Re: [PATCH v3 05/11] KVM: page track: introduce kvm_page_track_{add,remove}_page

[Xiao Guangrong <guangrong.xiao@linux.intel.com>]

On 02/19/2016 07:37 PM, Paolo Bonzini wrote:
>
>
> On 14/02/2016 12:31, Xiao Guangrong wrote:
>> +	/* does tracking count wrap? */
>> +	WARN_ON((count > 0) && (val + count < val));
>
> This doesn't work, because "val + count" is an int.

val is 'unsigned short val' and count is 'short', so
'val + count' is not int...

>
>> +	/* the last tracker has already gone? */
>> +	WARN_ON((count < 0) && (val < !count));
>
> Also, here any underflow should warn.
>
> You can actually use the fact that val + count is an int like this:
>
>      WARN_ON(val + count < 0 || val + count > USHRT_MAX)
>

It looks nice, i will change the type of val to int to simplify the
code.

> and also please return if the warning fires.
>

Okay.

>> +void kvm_page_track_add_page(struct kvm *kvm, gfn_t gfn,
>> +			     enum kvm_page_track_mode mode)
>> +{
>> +	struct kvm_memslots *slots;
>> +	struct kvm_memory_slot *slot;
>> +	int i;
>> +
>> +	for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
>> +		slots = __kvm_memslots(kvm, i);
>> +
>> +		slot = __gfn_to_memslot(slots, gfn);
>> +		if (!slot)
>> +			continue;
>> +
>> +		spin_lock(&kvm->mmu_lock);
>> +		kvm_slot_page_track_add_page_nolock(kvm, slot, gfn, mode);
>> +		spin_unlock(&kvm->mmu_lock);
>> +	}
>> +}
>
> I don't think it is right to walk all address spaces.  The good news is

Then we can not track the page in SMM mode, but i think it is not a big
problem as SMM is invisible to OS (it is expected to not hurting OS) and
current shadow page in normal spaces can not reflect the changes happend
in SMM either. So it is okay to only take normal space into account.

> that you're not using kvm_page_track_{add,remove}_page at all as far as
> I can see, so you can just remove them.

kvm_page_track_{add,remove}_page, which hides the mmu specifics (e.g. slot,
mmu-lock, etc.) and are exported as APIs for other users, are just the
small wrappers of kvm_slot_page_track_{add,remove}_page_nolock and the
nolock functions are used in the later patch.

If you think it is not a good time to export these APIs, i am okay to export
_nolock functions only in the next version.

>
> Also, when you will need it, I think it's better to move the
> spin_lock/spin_unlock pair outside the for loop.  With this change,
> perhaps it's better to leave it to the caller completely---but I cannot
> say until I see the caller.

I will remove page tracking in SMM address space, so this is no loop in
the next version. ;)

>
> In the meanwhile, please leave out _nolock from the other functions' name.

I just wanted to warn the user that these functions are not safe as they
are not protected by mmu-lock. I will remove these hints if you dislike them.