| Message ID | 20240809035134.699830-4-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/virt: Improve virt_kvm_type() | expand |
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
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
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
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();
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(-)