Re: [Qemu-arm] [Qemu-devel] [RFC 5/6] hw/arm/virt: support kvm_type property

[Auger Eric]

Hi Thomas,

On 07/09/2018 07:26 PM, Thomas Huth wrote:
> On 03.07.2018 14:31, Auger Eric wrote:
>> Hi Drew,
>>
>> On 07/03/2018 01:55 PM, Andrew Jones wrote:
>>> On Wed, Jun 20, 2018 at 03:07:32PM +0200, Eric Auger wrote:
>>>> The kvm-type property currently is used to pass
>>>> a user parameter to KVM_CREATE_VM. This matches
>>>> the way KVM/ARM expects to pass the max_vm_phys_shift
>>>> parameter.
>>>>
>>>> This patch adds the support or the kvm-type property in
>>>> machvirt and also implements the machine class kvm_type()
>>>> callback so that it either returns the kvm-type value
>>>> provided by the user or returns the max_vm_phys_shift
>>>> exposed by KVM.
>>>>
>>>> for instance, the usespace can use the following option to
>>>> instantiate a 42b IPA guest: -machine kvm-type=42
>>>
>>> 'kvm-type' is a very generic name. It looks like you're creating a KVM
>>> VM of type 42 (which I assume is the ultimate KVM VM that answers the
>>> meaning to Life, the Universe, and Everything), but it's not obvious
>>> how it relates to physical address bits. Why not call this property
>>> something like 'min_vm_phys_shift'? Notice the 'min' in the name,
>>> because this is where the user is stating what the minimum number of
>>> physical address bits required for the VM is. IIUC, if KVM supports
>>> more, then it shouldn't be a problem.
>>
>> Well I agree with you that using kvm-type=42 is not very nice.
>>
>> On the other hand the current kernel API to pass the VM GPA address size
>> is though the KVM_CREATE_VM kvm_type argument.
>>
>> in accel/kvm/kvm-all.c there is all the infrastructure to fetch the
>> generic machine kvm-type machine option and decode it into type, which
>> is passed to KVM_CREATE_VM.
>>
>> "
>>     kvm_type = qemu_opt_get(qemu_get_machine_opts(), "kvm-type");
>>     if (mc->kvm_type) {
>>         type = mc->kvm_type(ms, kvm_type);
>>     } else if (kvm_type) {
>>         ret = -EINVAL;
>>         fprintf(stderr, "Invalid argument kvm-type=%s\n", kvm_type);
>>         goto err;
>>     }
>>
>>     do {
>>         ret = kvm_ioctl(s, KVM_CREATE_VM, type);
>>     } while (ret == -EINTR);
>> "
>>
>> This infrastructure already is used in hw/ppc/spapr.c
> 
> FWIW: The ppc code uses "kvm-type" to select the KVM implementation in
> the kernel, since there are two implementations: kvm-pr (which is a
> trap-and-emulate implementation) and kvm-hv (which is a
> hardware-accelerated implementation). If you now introduce kvm-type for
> ARM, too, but with a completely different meaning, I think that could
> rather be confusing for the users...?

Agreed. The kernel user API still is under discussion and from a qemu
perspective I will use either another machine option or a cpu option to
set the intermediate physical address size.

Thanks

Eric
> 
>  Thomas
>