On 05/14/2018 02:55 PM, Liran Alon wrote: >>>>> >>>>> +#define KVM_MMU_ROOT_CURRENT BIT(0) >>>>> +#define KVM_MMU_ROOT_PREVIOUS BIT(1) >>>>> +#define KVM_MMU_ROOTS_ALL (KVM_MMU_ROOT_CURRENT | >>>>> KVM_MMU_ROOT_PREVIOUS) >>>>> + >>>> I think it is a bit weird that this patch introduce "uint >>>> roots_to_free" as flags >>>> while it is really used as a bool: Either you free both prev & >> active >>>> root or you free just active root. >>>> Therefore it makes more sense at this point for parameter to be >> "bool >>>> free_prev_root". >> You are correct that a bool free_prev_root is sufficient as far as >> this >> patch alone is concerned. That is actually how I wrote it initially. >> However, it would not be sufficient for patch 10, which does need to >> selectively free the prev_root only without freeing the current root. > I agree but this is not how patches should be made in my opinion. > I think every patch should make sense in it's own. > It makes sense that at this patch it will be a bool and that a later patch will then modify this > to flags when needed. Ok. I can change it to a bool in this patch and then change back to the flags in the later patch. Thanks, Junaid
