[PATCH 0/4] hw/arm/virt: Improve virt_kvm_type()

[PATCH 0/4] hw/arm/virt: Improve virt_kvm_type()

[Gavin Shan]

hw/arm/virt.c::virt_kvm_type() can be improved due to the facts as
below, when I inspect the code to review Salil's (RFCv3) vCPU hotplug
series.

  * virt_kvm_type() and MachineClass::kvm_type() are needed only when
    CONFIG_KVM is enabled.

  * kvm_arm_get_max_vm_ipa_size() and kvm_arch_get_default_type() are
    generally interchangeable with the exception that type zero is
    corresponding to IPA size (bits) 40. So it's unecessary to keep
    both APIs.

Lets clean it up so that kvm_arm_get_max_vm_ipa_size() is eventually
removed. kvm_arch_get_default_type() is used to get the KVM type and
IPA size (bits).

Gavin Shan (4):
  hw/arm/virt: hide virt_kvm_type() on !CONFIG_KVM
  hw/arm/virt: Avoid multiple lines of comments in virt_kvm_type()
  hw/arm/virt: Use kvm_arch_get_default_type()
  target/arm/kvm: Remove kvm_arm_get_max_vm_ipa_size()

 hw/arm/virt.c        | 22 +++++++++++-----------
 target/arm/kvm.c     | 13 ++-----------
 target/arm/kvm_arm.h | 15 ---------------
 3 files changed, 13 insertions(+), 37 deletions(-)

-- 
2.45.2

[PATCH 1/4] hw/arm/virt: hide virt_kvm_type() on !CONFIG_KVM

[Gavin Shan]

virt_kvm_type() and mc->kvm_type() are only needed when CONFIG_KVM
is enabled. It's reasonable to hide them when CONFIG_KVM is disabled.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 719e83e6a1..83be57db37 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2991,6 +2991,7 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
  * for arm64 kvm_type [7-0] encodes the requested number of bits
  * in the IPA address space
  */
+#ifdef CONFIG_KVM
 static int virt_kvm_type(MachineState *ms, const char *type_str)
 {
     VirtMachineState *vms = VIRT_MACHINE(ms);
@@ -3025,6 +3026,7 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
      */
     return fixed_ipa ? 0 : requested_pa_size;
 }
+#endif /* CONFIG_KVM */
 
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
@@ -3084,7 +3086,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
 #endif
     mc->valid_cpu_types = valid_cpu_types;
     mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
+#ifdef CONFIG_KVM
     mc->kvm_type = virt_kvm_type;
+#endif
     assert(!mc->get_hotplug_handler);
     mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
     hc->pre_plug = virt_machine_device_pre_plug_cb;
-- 
2.45.2

Re: [PATCH 1/4] hw/arm/virt: hide virt_kvm_type() on !CONFIG_KVM

[Peter Maydell]

On Fri, 9 Aug 2024 at 04:52, Gavin Shan <gshan@redhat.com> wrote:
>
> virt_kvm_type() and mc->kvm_type() are only needed when CONFIG_KVM
> is enabled. It's reasonable to hide them when CONFIG_KVM is disabled.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 719e83e6a1..83be57db37 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2991,6 +2991,7 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>   * for arm64 kvm_type [7-0] encodes the requested number of bits
>   * in the IPA address space
>   */
> +#ifdef CONFIG_KVM
>  static int virt_kvm_type(MachineState *ms, const char *type_str)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(ms);
> @@ -3025,6 +3026,7 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
>       */
>      return fixed_ipa ? 0 : requested_pa_size;
>  }
> +#endif /* CONFIG_KVM */
>
>  static void virt_machine_class_init(ObjectClass *oc, void *data)
>  {
> @@ -3084,7 +3086,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>  #endif
>      mc->valid_cpu_types = valid_cpu_types;
>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> +#ifdef CONFIG_KVM
>      mc->kvm_type = virt_kvm_type;
> +#endif
>      assert(!mc->get_hotplug_handler);
>      mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
>      hc->pre_plug = virt_machine_device_pre_plug_cb;

This adds extra ifdefs to the source code -- is there any
reason why we need them? If nothing goes wrong, I'd
prefer not to clutter the source with thme.

thanks
-- PMM

Re: [PATCH 1/4] hw/arm/virt: hide virt_kvm_type() on !CONFIG_KVM

[Gavin Shan]

On 8/9/24 7:00 PM, Peter Maydell wrote:
> On Fri, 9 Aug 2024 at 04:52, Gavin Shan <gshan@redhat.com> wrote:
>>
>> virt_kvm_type() and mc->kvm_type() are only needed when CONFIG_KVM
>> is enabled. It's reasonable to hide them when CONFIG_KVM is disabled.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>>   hw/arm/virt.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 719e83e6a1..83be57db37 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -2991,6 +2991,7 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>>    * for arm64 kvm_type [7-0] encodes the requested number of bits
>>    * in the IPA address space
>>    */
>> +#ifdef CONFIG_KVM
>>   static int virt_kvm_type(MachineState *ms, const char *type_str)
>>   {
>>       VirtMachineState *vms = VIRT_MACHINE(ms);
>> @@ -3025,6 +3026,7 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
>>        */
>>       return fixed_ipa ? 0 : requested_pa_size;
>>   }
>> +#endif /* CONFIG_KVM */
>>
>>   static void virt_machine_class_init(ObjectClass *oc, void *data)
>>   {
>> @@ -3084,7 +3086,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>>   #endif
>>       mc->valid_cpu_types = valid_cpu_types;
>>       mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
>> +#ifdef CONFIG_KVM
>>       mc->kvm_type = virt_kvm_type;
>> +#endif
>>       assert(!mc->get_hotplug_handler);
>>       mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
>>       hc->pre_plug = virt_machine_device_pre_plug_cb;
> 
> This adds extra ifdefs to the source code -- is there any
> reason why we need them? If nothing goes wrong, I'd
> prefer not to clutter the source with thme.
> 

There is nothing wrong in current code. Actually, it's preparatory work
to replace kvm_arm_get_max_vm_ipa_size() with kvm_arch_get_default_type()
in mc->kvm_type() in PATCH[3/4]. kvm_arch_get_default_type() is visible
only when CONFIG_KVM is enabled.

Thanks,
Gavin

[PATCH 2/4] hw/arm/virt: Avoid multiple lines of comments in virt_kvm_type()

[Gavin Shan]

The comment needn't to span multiple lines and can be merged to
one line perfectly.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 83be57db37..09b7a158a9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -3005,9 +3005,7 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
 
     requested_pa_size = 64 - clz64(vms->highest_gpa);
 
-    /*
-     * KVM requires the IPA size to be at least 32 bits.
-     */
+    /* KVM requires the IPA size to be at least 32 bits */
     if (requested_pa_size < 32) {
         requested_pa_size = 32;
     }
-- 
2.45.2

[PATCH 3/4] hw/arm/virt: Use kvm_arch_get_default_type()

[Gavin Shan]

kvm_arch_get_default_type() and kvm_arm_get_max_vm_ipa_size() are
interchangeable since the type is equivalent to IPA size (bits)
with one exception that IPA size (bits) is 40 when the type is zero.

Replace kvm_arm_get_max_vm_ipa_size() with kvm_arch_get_default_type().
After this, kvm_arm_get_max_vm_ipa_size() needn't to be a public API
any more.

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 hw/arm/virt.c        | 14 ++++++--------
 target/arm/kvm.c     |  2 +-
 target/arm/kvm_arm.h | 15 ---------------
 3 files changed, 7 insertions(+), 24 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 09b7a158a9..f35857aa95 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2995,10 +2995,12 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
 static int virt_kvm_type(MachineState *ms, const char *type_str)
 {
     VirtMachineState *vms = VIRT_MACHINE(ms);
-    int max_vm_pa_size, requested_pa_size;
+    int type, max_vm_pa_size, requested_pa_size;
     bool fixed_ipa;
 
-    max_vm_pa_size = kvm_arm_get_max_vm_ipa_size(ms, &fixed_ipa);
+    /* The IPA size is 40 bits when the type is zero */
+    type = kvm_arch_get_default_type(ms);
+    max_vm_pa_size = (type == 0) ? 40 : type;
 
     /* we freeze the memory map to compute the highest gpa */
     virt_set_memmap(vms, max_vm_pa_size);
@@ -3017,12 +3019,8 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
                      requested_pa_size, max_vm_pa_size);
         return -1;
     }
-    /*
-     * We return the requested PA log size, unless KVM only supports
-     * the implicit legacy 40b IPA setting, in which case the kvm_type
-     * must be 0.
-     */
-    return fixed_ipa ? 0 : requested_pa_size;
+
+    return type;
 }
 #endif /* CONFIG_KVM */
 
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 849e2e21b3..65893c9c12 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -526,7 +526,7 @@ bool kvm_arm_pmu_supported(void)
     return kvm_check_extension(kvm_state, KVM_CAP_ARM_PMU_V3);
 }
 
-int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
+static int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
 {
     KVMState *s = KVM_STATE(ms->accelerator);
     int ret;
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index cfaa0d9bc7..fd919d4738 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -188,16 +188,6 @@ bool kvm_arm_pmu_supported(void);
  */
 bool kvm_arm_sve_supported(void);
 
-/**
- * kvm_arm_get_max_vm_ipa_size:
- * @ms: Machine state handle
- * @fixed_ipa: True when the IPA limit is fixed at 40. This is the case
- * for legacy KVM.
- *
- * Returns the number of bits in the IPA address space supported by KVM
- */
-int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa);
-
 int kvm_arm_vgic_probe(void);
 
 void kvm_arm_pmu_init(ARMCPU *cpu);
@@ -248,11 +238,6 @@ static inline void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
     g_assert_not_reached();
 }
 
-static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
-{
-    g_assert_not_reached();
-}
-
 static inline int kvm_arm_vgic_probe(void)
 {
     g_assert_not_reached();
-- 
2.45.2

Re: [PATCH 3/4] hw/arm/virt: Use kvm_arch_get_default_type()

[Peter Maydell]

On Fri, 9 Aug 2024 at 04:52, Gavin Shan <gshan@redhat.com> wrote:
>
> kvm_arch_get_default_type() and kvm_arm_get_max_vm_ipa_size() are
> interchangeable since the type is equivalent to IPA size (bits)
> with one exception that IPA size (bits) is 40 when the type is zero.

Well, sort of, but they're conceptually different.

kvm_arch_get_default_type() is the API for "give me the value
I need to pass to kvm_ioctl(s, KVM_CREATE_VM, type)"; it's
architecture independent and different architectures do
different things. In the future Arm might need to do something
other than "just pass in the IPA size".

kvm_arm_get_max_vm_ipa_size() does exactly what it says on the
tin: it is an Arm specific function that returns the maximum
supported IPA size.

I would prefer not to conflate the two.

thanks
-- PMM

Re: [PATCH 3/4] hw/arm/virt: Use kvm_arch_get_default_type()

[Gavin Shan]

On 8/9/24 6:59 PM, Peter Maydell wrote:
> On Fri, 9 Aug 2024 at 04:52, Gavin Shan <gshan@redhat.com> wrote:
>>
>> kvm_arch_get_default_type() and kvm_arm_get_max_vm_ipa_size() are
>> interchangeable since the type is equivalent to IPA size (bits)
>> with one exception that IPA size (bits) is 40 when the type is zero.
> 
> Well, sort of, but they're conceptually different.
> 
> kvm_arch_get_default_type() is the API for "give me the value
> I need to pass to kvm_ioctl(s, KVM_CREATE_VM, type)"; it's
> architecture independent and different architectures do
> different things. In the future Arm might need to do something
> other than "just pass in the IPA size".
> 
> kvm_arm_get_max_vm_ipa_size() does exactly what it says on the
> tin: it is an Arm specific function that returns the maximum
> supported IPA size.
> 
> I would prefer not to conflate the two.
> 

Agreed. Thanks for the explanation. Lets keep them separate and ignore
this series.

By the way, mc->kvm_type() can be used if one machine (platform) needs
to do more things than kvm_arch_get_default_type(), similar to what we're
doing on 'virt' machine currently.

Thanks,
Gavin

Re: [PATCH 3/4] hw/arm/virt: Use kvm_arch_get_default_type()

[Gavin Shan]

On 8/9/24 1:51 PM, Gavin Shan wrote:
> kvm_arch_get_default_type() and kvm_arm_get_max_vm_ipa_size() are
> interchangeable since the type is equivalent to IPA size (bits)
> with one exception that IPA size (bits) is 40 when the type is zero.
> 
> Replace kvm_arm_get_max_vm_ipa_size() with kvm_arch_get_default_type().
> After this, kvm_arm_get_max_vm_ipa_size() needn't to be a public API
> any more.
> 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>   hw/arm/virt.c        | 14 ++++++--------
>   target/arm/kvm.c     |  2 +-
>   target/arm/kvm_arm.h | 15 ---------------
>   3 files changed, 7 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 09b7a158a9..f35857aa95 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2995,10 +2995,12 @@ static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>   static int virt_kvm_type(MachineState *ms, const char *type_str)
>   {
>       VirtMachineState *vms = VIRT_MACHINE(ms);
> -    int max_vm_pa_size, requested_pa_size;
> +    int type, max_vm_pa_size, requested_pa_size;
>       bool fixed_ipa;
>   
> -    max_vm_pa_size = kvm_arm_get_max_vm_ipa_size(ms, &fixed_ipa);
> +    /* The IPA size is 40 bits when the type is zero */
> +    type = kvm_arch_get_default_type(ms);
> +    max_vm_pa_size = (type == 0) ? 40 : type;
>   
>       /* we freeze the memory map to compute the highest gpa */
>       virt_set_memmap(vms, max_vm_pa_size);
> @@ -3017,12 +3019,8 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
>                        requested_pa_size, max_vm_pa_size);
>           return -1;
>       }
> -    /*
> -     * We return the requested PA log size, unless KVM only supports
> -     * the implicit legacy 40b IPA setting, in which case the kvm_type
> -     * must be 0.
> -     */
> -    return fixed_ipa ? 0 : requested_pa_size;
> +
> +    return type;
>   }
>   #endif /* CONFIG_KVM */
>   

This actually needs to be something like below. It's incorrect to ignore
@requested_pa_size here.

         return (type == 0) ? 0 : requested_pa_size;

> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 849e2e21b3..65893c9c12 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -526,7 +526,7 @@ bool kvm_arm_pmu_supported(void)
>       return kvm_check_extension(kvm_state, KVM_CAP_ARM_PMU_V3);
>   }
>   
> -int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
> +static int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
>   {
>       KVMState *s = KVM_STATE(ms->accelerator);
>       int ret;
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index cfaa0d9bc7..fd919d4738 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -188,16 +188,6 @@ bool kvm_arm_pmu_supported(void);
>    */
>   bool kvm_arm_sve_supported(void);
>   
> -/**
> - * kvm_arm_get_max_vm_ipa_size:
> - * @ms: Machine state handle
> - * @fixed_ipa: True when the IPA limit is fixed at 40. This is the case
> - * for legacy KVM.
> - *
> - * Returns the number of bits in the IPA address space supported by KVM
> - */
> -int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa);
> -
>   int kvm_arm_vgic_probe(void);
>   
>   void kvm_arm_pmu_init(ARMCPU *cpu);
> @@ -248,11 +238,6 @@ static inline void kvm_arm_add_vcpu_properties(ARMCPU *cpu)
>       g_assert_not_reached();
>   }
>   
> -static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
> -{
> -    g_assert_not_reached();
> -}
> -
>   static inline int kvm_arm_vgic_probe(void)
>   {
>       g_assert_not_reached();

Thanks,
Gavin

[PATCH 4/4] target/arm/kvm: Remove kvm_arm_get_max_vm_ipa_size()

[Gavin Shan]

Remove kvm_arm_get_max_vm_ipa_size() after its logics are moved
to its only caller kvm_arch_get_default_type().

Signed-off-by: Gavin Shan <gshan@redhat.com>
---
 target/arm/kvm.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 65893c9c12..5d9e66d82d 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -526,22 +526,13 @@ bool kvm_arm_pmu_supported(void)
     return kvm_check_extension(kvm_state, KVM_CAP_ARM_PMU_V3);
 }
 
-static int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
+int kvm_arch_get_default_type(MachineState *ms)
 {
     KVMState *s = KVM_STATE(ms->accelerator);
     int ret;
 
     ret = kvm_check_extension(s, KVM_CAP_ARM_VM_IPA_SIZE);
-    *fixed_ipa = ret <= 0;
-
-    return ret > 0 ? ret : 40;
-}
-
-int kvm_arch_get_default_type(MachineState *ms)
-{
-    bool fixed_ipa;
-    int size = kvm_arm_get_max_vm_ipa_size(ms, &fixed_ipa);
-    return fixed_ipa ? 0 : size;
+    return (ret <= 0) ? 0 : ret;
 }
 
 int kvm_arch_init(MachineState *ms, KVMState *s)
-- 
2.45.2