On 19.12.2016 01:48, David Gibson wrote:
> On Fri, Dec 16, 2016 at 01:44:57PM +0100, Thomas Huth wrote:
>> On 15.12.2016 06:53, David Gibson wrote:
>>> The KVM_PPC_ALLOCATE_HTAB ioctl() is used to set the size of hashed page
>>> table (HPT) that userspace expects a guest VM to have, and is also used to
>>> clear that HPT when necessary (e.g. guest reboot).
>>>
>>> At present, once the ioctl() is called for the first time, the HPT size can
>>> never be changed thereafter - it will be cleared but always sized as from
>>> the first call.
>>>
>>> With upcoming HPT resize implementation, we're going to need to allow
>>> userspace to resize the HPT at reset (to change it back to the default size
>>> if the guest changed it).
>>>
>>> So, we need to allow this ioctl() to change the HPT size.
>>>
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
[...]
>>> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> index 68bb228..8e5ac2f 100644
>>> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
>>> @@ -104,10 +104,22 @@ void kvmppc_set_hpt(struct kvm *kvm, struct kvm_hpt_info *info)
>>>  		info->virt, (long)info->order, kvm->arch.lpid);
>>>  }
>>>  
>>> -long kvmppc_alloc_reset_hpt(struct kvm *kvm, u32 *htab_orderp)
>>> +void kvmppc_free_hpt(struct kvm_hpt_info *info)
>>> +{
>>> +	vfree(info->rev);
>>> +	if (info->cma)
>>> +		kvm_free_hpt_cma(virt_to_page(info->virt),
>>> +				 1 << (info->order - PAGE_SHIFT));
>>> +	else
>>> +		free_pages(info->virt, info->order - PAGE_SHIFT);
>>> +	info->virt = 0;
>>> +	info->order = 0;
>>> +}
>>
>> Why do you need to move kvmppc_free_hpt() around? Seems like unecessary
>> code churn to me?
> 
> Previously, kvmppc_free_hpt() wasn't needed in
> kvmppc_alloc_reset_hpt(), now it is.  So we need to move it above that
> function.  I could move it in the previous patch, but that would
> obscure what the actual changes are to it, so it seemed better to do
> it here.

kvmppc_free_hpt() is not a static function, there is a prototype in a
header for this somewhere, so as far as I can see, it should also work
without moving this function?

[...]
>>> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
>>> index 71c5adb..957e473 100644
>>> --- a/arch/powerpc/kvm/book3s_hv.c
>>> +++ b/arch/powerpc/kvm/book3s_hv.c
>>> @@ -3600,12 +3600,9 @@ static long kvm_arch_vm_ioctl_hv(struct file *filp,
>>>  		r = -EFAULT;
>>>  		if (get_user(htab_order, (u32 __user *)argp))
>>>  			break;
>>> -		r = kvmppc_alloc_reset_hpt(kvm, &htab_order);
>>> +		r = kvmppc_alloc_reset_hpt(kvm, htab_order);
>>>  		if (r)
>>>  			break;
>>> -		r = -EFAULT;
>>> -		if (put_user(htab_order, (u32 __user *)argp))
>>> -			break;
>>
>> Now that htab_order is not changed anymore by the kernel, I'm pretty
>> sure you need some checks on the value here before calling
>> kvmppc_alloc_reset_hpt(), e.g. return an error code if htab_order <
>> PPC_MIN_HPT_ORDER.
> 
> Right.  I've done that by putting the checks into
> kvmppc_allocate_hpt() in the earlier patch.
> 
>> And, I'm not sure if I got that right, but in former times, the
>> htab_order from the userspace application was just a suggestion, and now
>> it's mandatory, right? So if an old userspace used a very high value
>> here (or even something smaller than PPC_MIN_HPT_ORDER like 0), the
>> kernel fixed this up and the userspace could run happily with the fixed
>> value afterwards. But since this value from userspace if mandatory now,
>> such an userspace application is broken now. So maybe it's better to
>> introduce a new ioctl for this new behavior instead, to avoid breaking
>> old userspace applications?
> 
> A long time ago it was just a hint.  However, that behaviour was
> already changed in 572abd5 "KVM: PPC: Book3S HV: Don't fall back to
> smaller HPT size in allocation ioctl".  This is important: without
> that we could get a different HPT size on the two ends of a migration,
> which broke things nastily.

OK, makes sense, especially if the userspace provided an order. But if I
get that patch right, it was still possible to call with order == 0 to
get the automatic sizing? Do we need to preserve that behavior for some
very old userspace applications?

 Thomas