[RFC,5/6] hw/arm/virt: support kvm_type property
diff mbox series

Message ID 1529500053-21704-6-git-send-email-eric.auger@redhat.com
State New
Headers show
Series
  • KVM/ARM: Dynamic and larger GPA size
Related show

Commit Message

Auger Eric June 20, 2018, 1:07 p.m. UTC
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

Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 hw/arm/virt.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/hw/arm/virt.h |  1 +
 2 files changed, 45 insertions(+)

Comments

Dr. David Alan Gilbert June 20, 2018, 5:15 p.m. UTC | #1
* Eric Auger (eric.auger@redhat.com) 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

Without saying if this is better or worse, it is different from x86,
where we have the number of physical address bits as a -cpu parameter
rather than a machine parameter, e.g.

   qemu-system-x86_64 -M pc,accel=kvm -cpu SandyBridge,phys-bits=36
or
   qemu-system-x86_64 -M pc,accel=kvm -cpu SandyBridge,host-phys-bits=true

our machine types can override the default value though.

One other complication (that I don't know if it applies to ARM) is that
TCG only supports phys-bits=40, so we refuse a TCG run with an
explicitly set phys-bits!=40.

Dave

> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/arm/virt.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 45 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index dd92ab9..1700556 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1585,6 +1585,21 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
>      }
>  }
>  
> +static char *virt_get_kvm_type(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return g_strdup(vms->kvm_type);
> +}
> +
> +static void virt_set_kvm_type(Object *obj, const char *value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    g_free(vms->kvm_type);
> +    vms->kvm_type = g_strdup(value);
> +}
> +
>  static CpuInstanceProperties
>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>  {
> @@ -1646,6 +1661,31 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>      return NULL;
>  }
>  
> +static int virt_kvm_type(MachineState *ms, const char *type_str)
> +{
> +    int max_vm_phys_shift, ret = 0;
> +    uint64_t type;
> +
> +    if (!type_str) {
> +        max_vm_phys_shift = kvm_get_max_vm_phys_shift(ms);
> +        if (max_vm_phys_shift < 0) {
> +            goto out;
> +        }
> +    } else {
> +        type = g_ascii_strtoll(type_str, NULL, 0);
> +        type &= 0xFF;
> +        max_vm_phys_shift = (int)type;
> +        if (max_vm_phys_shift < 40 || max_vm_phys_shift > 52) {
> +            warn_report("valid kvm-type type values are within [40, 52]:"
> +                        " option is ignored and VM is created with 40b IPA");
> +            goto out;
> +        }
> +    }
> +    ret = max_vm_phys_shift;
> +out:
> +    return ret;
> +}
> +
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -1668,6 +1708,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> +    mc->kvm_type = virt_kvm_type;
>      assert(!mc->get_hotplug_handler);
>      mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
>      hc->plug = virt_machine_device_plug_cb;
> @@ -1756,6 +1797,9 @@ static void virt_3_0_instance_init(Object *obj)
>                                      "Valid values are none and smmuv3",
>                                      NULL);
>  
> +    object_property_add_str(obj, "kvm-type",
> +                            virt_get_kvm_type, virt_set_kvm_type, NULL);
> +
>      vms->memmap = a15memmap;
>      vms->irqmap = a15irqmap;
>  }
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 4ac7ef6..2674ce7 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -118,6 +118,7 @@ typedef struct {
>      uint32_t msi_phandle;
>      uint32_t iommu_phandle;
>      int psci_conduit;
> +    char *kvm_type;
>  } VirtMachineState;
>  
>  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
> -- 
> 2.5.5
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Auger Eric June 21, 2018, 10:05 a.m. UTC | #2
Hi David,

On 06/20/2018 07:15 PM, Dr. David Alan Gilbert wrote:
> * Eric Auger (eric.auger@redhat.com) 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
> 
> Without saying if this is better or worse, it is different from x86,
> where we have the number of physical address bits as a -cpu parameter
> rather than a machine parameter, e.g.
> 
>    qemu-system-x86_64 -M pc,accel=kvm -cpu SandyBridge,phys-bits=36
> or
>    qemu-system-x86_64 -M pc,accel=kvm -cpu SandyBridge,host-phys-bits=true
> 
> our machine types can override the default value though.

My understanding is this phys-bits is used to refine the model of the
CPU. For a given CPU, it is implementation defined how much physical and
virtual bits are supported. So it can vary. The guest then can query the
properties of the CPU using CPUID and when setting eax=0x80000008, it
then retrieves the max physical and virtual address space supported by
this CPU.

On ARM there is such capability register, named
ID_AA64MMFR0_EL1 where PARange field indicates the implemented physical
address size. No equivalent for the virtual range.

arm/internals.h implements arm_pamax(ARMCPU *cpu)
It decodes cpu->id_aa64mmfr0. This later is hardcoded for A57, A53, ...
So as far as I understand, we don't have a way to refine this reg
through cmd line, as we have on x86. But if we were to support this
feature, then phys_bits would be cpu->id_aa64mmfr0.parange.

However what we are trying to achieve in this series is different. We
don't want to refine the vCPU itself. We want to query and force KVM to
support a given intermediate physical address (IPA) range. This means
configure KVM to support a given stage 2 MMU configuration. As a
correlate this means supporting the corresponding GPA range. KVM's
capability to support a given IPA range depends on
- physical CPU ID_AA64MMFR0_EL1.PARange
- host kernel config
- host kernel page table limitations

So the kernel API proposed in [1] allows to query the max IPA KVM
support (through a /dev/kvm iotcl) and this series then does a
KVM_CREATE_VM with a type argument that matches the returned value. So
to me this rather looks as a VM attribute, which from a qemu perspective
would turn out to be a machine option. Then whether we should directly
use the kvm_type option is arguable. on spapr kvm_type typically encodes
the HV or PR. I understand this selects different kvm modules. Here we
want to use a kvm supporting up to 48b IPA for instance.

> 
> One other complication (that I don't know if it applies to ARM) is that
> TCG only supports phys-bits=40, so we refuse a TCG run with an
> explicitly set phys-bits!=40.

The new capability to support selectable GPA range only applies to
accelerated mode. I did not check if kvm-type option can be used in TCG
mode ;-)

Thanks

Eric
> 
> Dave
> 
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/arm/virt.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
>>  include/hw/arm/virt.h |  1 +
>>  2 files changed, 45 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index dd92ab9..1700556 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -1585,6 +1585,21 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
>>      }
>>  }
>>  
>> +static char *virt_get_kvm_type(Object *obj, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    return g_strdup(vms->kvm_type);
>> +}
>> +
>> +static void virt_set_kvm_type(Object *obj, const char *value, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    g_free(vms->kvm_type);
>> +    vms->kvm_type = g_strdup(value);
>> +}
>> +
>>  static CpuInstanceProperties
>>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>>  {
>> @@ -1646,6 +1661,31 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>>      return NULL;
>>  }
>>  
>> +static int virt_kvm_type(MachineState *ms, const char *type_str)
>> +{
>> +    int max_vm_phys_shift, ret = 0;
>> +    uint64_t type;
>> +
>> +    if (!type_str) {
>> +        max_vm_phys_shift = kvm_get_max_vm_phys_shift(ms);
>> +        if (max_vm_phys_shift < 0) {
>> +            goto out;
>> +        }
>> +    } else {
>> +        type = g_ascii_strtoll(type_str, NULL, 0);
>> +        type &= 0xFF;
>> +        max_vm_phys_shift = (int)type;
>> +        if (max_vm_phys_shift < 40 || max_vm_phys_shift > 52) {
>> +            warn_report("valid kvm-type type values are within [40, 52]:"
>> +                        " option is ignored and VM is created with 40b IPA");
>> +            goto out;
>> +        }
>> +    }
>> +    ret = max_vm_phys_shift;
>> +out:
>> +    return ret;
>> +}
>> +
>>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>>  {
>>      MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -1668,6 +1708,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>>      mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
>> +    mc->kvm_type = virt_kvm_type;
>>      assert(!mc->get_hotplug_handler);
>>      mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
>>      hc->plug = virt_machine_device_plug_cb;
>> @@ -1756,6 +1797,9 @@ static void virt_3_0_instance_init(Object *obj)
>>                                      "Valid values are none and smmuv3",
>>                                      NULL);
>>  
>> +    object_property_add_str(obj, "kvm-type",
>> +                            virt_get_kvm_type, virt_set_kvm_type, NULL);
>> +
>>      vms->memmap = a15memmap;
>>      vms->irqmap = a15irqmap;
>>  }
>> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
>> index 4ac7ef6..2674ce7 100644
>> --- a/include/hw/arm/virt.h
>> +++ b/include/hw/arm/virt.h
>> @@ -118,6 +118,7 @@ typedef struct {
>>      uint32_t msi_phandle;
>>      uint32_t iommu_phandle;
>>      int psci_conduit;
>> +    char *kvm_type;
>>  } VirtMachineState;
>>  
>>  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
>> -- 
>> 2.5.5
>>
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
>
Dr. David Alan Gilbert June 27, 2018, 12:01 p.m. UTC | #3
* Auger Eric (eric.auger@redhat.com) wrote:
> Hi David,
> 
> On 06/20/2018 07:15 PM, Dr. David Alan Gilbert wrote:
> > * Eric Auger (eric.auger@redhat.com) 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
> > 
> > Without saying if this is better or worse, it is different from x86,
> > where we have the number of physical address bits as a -cpu parameter
> > rather than a machine parameter, e.g.
> > 
> >    qemu-system-x86_64 -M pc,accel=kvm -cpu SandyBridge,phys-bits=36
> > or
> >    qemu-system-x86_64 -M pc,accel=kvm -cpu SandyBridge,host-phys-bits=true
> > 
> > our machine types can override the default value though.
> 
> My understanding is this phys-bits is used to refine the model of the
> CPU. For a given CPU, it is implementation defined how much physical and
> virtual bits are supported. So it can vary. The guest then can query the
> properties of the CPU using CPUID and when setting eax=0x80000008, it
> then retrieves the max physical and virtual address space supported by
> this CPU.
> 
> On ARM there is such capability register, named
> ID_AA64MMFR0_EL1 where PARange field indicates the implemented physical
> address size. No equivalent for the virtual range.
> 
> arm/internals.h implements arm_pamax(ARMCPU *cpu)
> It decodes cpu->id_aa64mmfr0. This later is hardcoded for A57, A53, ...
> So as far as I understand, we don't have a way to refine this reg
> through cmd line, as we have on x86. But if we were to support this
> feature, then phys_bits would be cpu->id_aa64mmfr0.parange.
> 
> However what we are trying to achieve in this series is different. We
> don't want to refine the vCPU itself. We want to query and force KVM to
> support a given intermediate physical address (IPA) range. This means
> configure KVM to support a given stage 2 MMU configuration. As a
> correlate this means supporting the corresponding GPA range. KVM's
> capability to support a given IPA range depends on
> - physical CPU ID_AA64MMFR0_EL1.PARange
> - host kernel config
> - host kernel page table limitations
> 
> So the kernel API proposed in [1] allows to query the max IPA KVM
> support (through a /dev/kvm iotcl) and this series then does a
> KVM_CREATE_VM with a type argument that matches the returned value. So
> to me this rather looks as a VM attribute, which from a qemu perspective
> would turn out to be a machine option. Then whether we should directly
> use the kvm_type option is arguable. on spapr kvm_type typically encodes
> the HV or PR. I understand this selects different kvm modules. Here we
> want to use a kvm supporting up to 48b IPA for instance.

OK, my only worry is that management layers are going to see this
differently for different architectures, where it feels like it's
basically the same thing.

Note on x86, while I don't think there's a host kernel restriction on
phys-bits/GPA, there is the restriction of the host CPU, so if setting
it to anything other than host-phys-bits there does need to be some
probing somewhere.

> > 
> > One other complication (that I don't know if it applies to ARM) is that
> > TCG only supports phys-bits=40, so we refuse a TCG run with an
> > explicitly set phys-bits!=40.
> 
> The new capability to support selectable GPA range only applies to
> accelerated mode. I did not check if kvm-type option can be used in TCG
> mode ;-)

Yes, I think all I did was add an error check to warn/fail if you try
and set it wrongly.

Dave

> Thanks
> 
> Eric
> > 
> > Dave
> > 
> >> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> >> ---
> >>  hw/arm/virt.c         | 44 ++++++++++++++++++++++++++++++++++++++++++++
> >>  include/hw/arm/virt.h |  1 +
> >>  2 files changed, 45 insertions(+)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index dd92ab9..1700556 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -1585,6 +1585,21 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
> >>      }
> >>  }
> >>  
> >> +static char *virt_get_kvm_type(Object *obj, Error **errp)
> >> +{
> >> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> >> +
> >> +    return g_strdup(vms->kvm_type);
> >> +}
> >> +
> >> +static void virt_set_kvm_type(Object *obj, const char *value, Error **errp)
> >> +{
> >> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> >> +
> >> +    g_free(vms->kvm_type);
> >> +    vms->kvm_type = g_strdup(value);
> >> +}
> >> +
> >>  static CpuInstanceProperties
> >>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
> >>  {
> >> @@ -1646,6 +1661,31 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
> >>      return NULL;
> >>  }
> >>  
> >> +static int virt_kvm_type(MachineState *ms, const char *type_str)
> >> +{
> >> +    int max_vm_phys_shift, ret = 0;
> >> +    uint64_t type;
> >> +
> >> +    if (!type_str) {
> >> +        max_vm_phys_shift = kvm_get_max_vm_phys_shift(ms);
> >> +        if (max_vm_phys_shift < 0) {
> >> +            goto out;
> >> +        }
> >> +    } else {
> >> +        type = g_ascii_strtoll(type_str, NULL, 0);
> >> +        type &= 0xFF;
> >> +        max_vm_phys_shift = (int)type;
> >> +        if (max_vm_phys_shift < 40 || max_vm_phys_shift > 52) {
> >> +            warn_report("valid kvm-type type values are within [40, 52]:"
> >> +                        " option is ignored and VM is created with 40b IPA");
> >> +            goto out;
> >> +        }
> >> +    }
> >> +    ret = max_vm_phys_shift;
> >> +out:
> >> +    return ret;
> >> +}
> >> +
> >>  static void virt_machine_class_init(ObjectClass *oc, void *data)
> >>  {
> >>      MachineClass *mc = MACHINE_CLASS(oc);
> >> @@ -1668,6 +1708,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
> >>      mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
> >>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
> >>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> >> +    mc->kvm_type = virt_kvm_type;
> >>      assert(!mc->get_hotplug_handler);
> >>      mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
> >>      hc->plug = virt_machine_device_plug_cb;
> >> @@ -1756,6 +1797,9 @@ static void virt_3_0_instance_init(Object *obj)
> >>                                      "Valid values are none and smmuv3",
> >>                                      NULL);
> >>  
> >> +    object_property_add_str(obj, "kvm-type",
> >> +                            virt_get_kvm_type, virt_set_kvm_type, NULL);
> >> +
> >>      vms->memmap = a15memmap;
> >>      vms->irqmap = a15irqmap;
> >>  }
> >> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> >> index 4ac7ef6..2674ce7 100644
> >> --- a/include/hw/arm/virt.h
> >> +++ b/include/hw/arm/virt.h
> >> @@ -118,6 +118,7 @@ typedef struct {
> >>      uint32_t msi_phandle;
> >>      uint32_t iommu_phandle;
> >>      int psci_conduit;
> >> +    char *kvm_type;
> >>  } VirtMachineState;
> >>  
> >>  #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")
> >> -- 
> >> 2.5.5
> >>
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Andrew Jones July 3, 2018, 11:55 a.m. UTC | #4
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.

Thanks,
drew
Auger Eric July 3, 2018, 12:31 p.m. UTC | #5
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.

Thanks

Eric


> 
> Thanks,
> drew
>
Andrew Jones July 3, 2018, 12:47 p.m. UTC | #6
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
> > 
>
Suzuki K Poulose July 3, 2018, 1:20 p.m. UTC | #7
On 03/07/18 13:47, Andrew Jones wrote:
>> 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 for spotting, I will update the documentation.

Suzuki
Thomas Huth July 9, 2018, 5:26 p.m. UTC | #8
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...?

 Thomas
Auger Eric July 10, 2018, 10:07 a.m. UTC | #9
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
>

Patch
diff mbox series

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index dd92ab9..1700556 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1585,6 +1585,21 @@  static void virt_set_iommu(Object *obj, const char *value, Error **errp)
     }
 }
 
+static char *virt_get_kvm_type(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return g_strdup(vms->kvm_type);
+}
+
+static void virt_set_kvm_type(Object *obj, const char *value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    g_free(vms->kvm_type);
+    vms->kvm_type = g_strdup(value);
+}
+
 static CpuInstanceProperties
 virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
 {
@@ -1646,6 +1661,31 @@  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
     return NULL;
 }
 
+static int virt_kvm_type(MachineState *ms, const char *type_str)
+{
+    int max_vm_phys_shift, ret = 0;
+    uint64_t type;
+
+    if (!type_str) {
+        max_vm_phys_shift = kvm_get_max_vm_phys_shift(ms);
+        if (max_vm_phys_shift < 0) {
+            goto out;
+        }
+    } else {
+        type = g_ascii_strtoll(type_str, NULL, 0);
+        type &= 0xFF;
+        max_vm_phys_shift = (int)type;
+        if (max_vm_phys_shift < 40 || max_vm_phys_shift > 52) {
+            warn_report("valid kvm-type type values are within [40, 52]:"
+                        " option is ignored and VM is created with 40b IPA");
+            goto out;
+        }
+    }
+    ret = max_vm_phys_shift;
+out:
+    return ret;
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1668,6 +1708,7 @@  static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
     mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+    mc->kvm_type = virt_kvm_type;
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
     hc->plug = virt_machine_device_plug_cb;
@@ -1756,6 +1797,9 @@  static void virt_3_0_instance_init(Object *obj)
                                     "Valid values are none and smmuv3",
                                     NULL);
 
+    object_property_add_str(obj, "kvm-type",
+                            virt_get_kvm_type, virt_set_kvm_type, NULL);
+
     vms->memmap = a15memmap;
     vms->irqmap = a15irqmap;
 }
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 4ac7ef6..2674ce7 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -118,6 +118,7 @@  typedef struct {
     uint32_t msi_phandle;
     uint32_t iommu_phandle;
     int psci_conduit;
+    char *kvm_type;
 } VirtMachineState;
 
 #define TYPE_VIRT_MACHINE   MACHINE_TYPE_NAME("virt")