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

[Andrew Jones]

On Tue, Jul 03, 2018 at 02:31:02PM +0200, 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
> 
> Whould it be better if we would pass something like kvm-type=48bGPA?
> Otherwise I can decode another virt machine option (min_vm_phys_shift)
> in kvm_type callback.

Yes, this is what I'm thinking. I don't believe we have to expose the
details of the KVM API to the user through the QEMU command line. The
details are actually more complicated anyway, as the phys-shift is
only the lower 8-bits of KVM type[*], not the whole value.

Thanks,
drew

[*] Looks like Suzuki's series is missing the Documentation/virtual/kvm/api.txt
    update needed to specify that.

> 
> Thanks
> 
> Eric
> 
> 
> > 
> > Thanks,
> > drew
> > 
>