[PATCH v5 0/6] accel/kvm: Specify default IPA size for arm64

[PATCH v5 0/6] accel/kvm: Specify default IPA size for arm64

[Akihiko Odaki]

Some Arm systems such as Apple Silicon Macs have IPA size smaller than the
default used by KVM. Introduce our own default IPA size that fits on such a
system.

When reviewing this series, Philippe Mathieu-Daudé found the error handling
around KVM type decision logic is flawed so I added a few patches for fixing
the error handling path.

V4 -> V5: Fixed KVM type error handling
V3 -> V4: Removed an inclusion of kvm_mips.h that is no longer needed.
V2 -> V3: Changed to use the maximum IPA size as the default.
V1 -> V2: Introduced an arch hook

Akihiko Odaki (6):
  kvm: Introduce kvm_arch_get_default_type hook
  accel/kvm: Specify default IPA size for arm64
  mips: Report an error when KVM_VM_MIPS_VZ is unavailable
  accel/kvm: Use negative KVM type for error propagation
  accel/kvm: Free as when an error occurred
  accel/kvm: Make kvm_dirty_ring_reaper_init() void

 include/sysemu/kvm.h     |  2 ++
 target/mips/kvm_mips.h   |  9 ---------
 accel/kvm/kvm-all.c      | 19 +++++++++++--------
 hw/arm/virt.c            |  2 +-
 hw/mips/loongson3_virt.c |  2 --
 hw/ppc/spapr.c           |  2 +-
 target/arm/kvm.c         |  7 +++++++
 target/i386/kvm/kvm.c    |  5 +++++
 target/mips/kvm.c        |  3 ++-
 target/ppc/kvm.c         |  5 +++++
 target/riscv/kvm.c       |  5 +++++
 target/s390x/kvm/kvm.c   |  5 +++++
 12 files changed, 44 insertions(+), 22 deletions(-)

-- 
2.41.0

[PATCH v5 1/6] kvm: Introduce kvm_arch_get_default_type hook

[Akihiko Odaki]

kvm_arch_get_default_type() returns the default KVM type. This hook is
particularly useful to derive a KVM type that is valid for "none"
machine model, which is used by libvirt to probe the availability of
KVM.

For MIPS, the existing mips_kvm_type() is reused. This function ensures
the availability of VZ which is mandatory to use KVM on the current
QEMU.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 include/sysemu/kvm.h     | 2 ++
 target/mips/kvm_mips.h   | 9 ---------
 accel/kvm/kvm-all.c      | 4 +++-
 hw/mips/loongson3_virt.c | 2 --
 target/arm/kvm.c         | 5 +++++
 target/i386/kvm/kvm.c    | 5 +++++
 target/mips/kvm.c        | 2 +-
 target/ppc/kvm.c         | 5 +++++
 target/riscv/kvm.c       | 5 +++++
 target/s390x/kvm/kvm.c   | 5 +++++
 10 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index 115f0cca79..ccaf55caf7 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -369,6 +369,8 @@ int kvm_arch_get_registers(CPUState *cpu);
 
 int kvm_arch_put_registers(CPUState *cpu, int level);
 
+int kvm_arch_get_default_type(MachineState *ms);
+
 int kvm_arch_init(MachineState *ms, KVMState *s);
 
 int kvm_arch_init_vcpu(CPUState *cpu);
diff --git a/target/mips/kvm_mips.h b/target/mips/kvm_mips.h
index 171d53dbe1..c711269d0a 100644
--- a/target/mips/kvm_mips.h
+++ b/target/mips/kvm_mips.h
@@ -25,13 +25,4 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu);
 int kvm_mips_set_interrupt(MIPSCPU *cpu, int irq, int level);
 int kvm_mips_set_ipi_interrupt(MIPSCPU *cpu, int irq, int level);
 
-#ifdef CONFIG_KVM
-int mips_kvm_type(MachineState *machine, const char *vm_type);
-#else
-static inline int mips_kvm_type(MachineState *machine, const char *vm_type)
-{
-    return 0;
-}
-#endif
-
 #endif /* KVM_MIPS_H */
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 373d876c05..d591b5079c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2458,7 +2458,7 @@ static int kvm_init(MachineState *ms)
     KVMState *s;
     const KVMCapabilityInfo *missing_cap;
     int ret;
-    int type = 0;
+    int type;
     uint64_t dirty_log_manual_caps;
 
     qemu_mutex_init(&kml_slots_lock);
@@ -2523,6 +2523,8 @@ static int kvm_init(MachineState *ms)
         type = mc->kvm_type(ms, kvm_type);
     } else if (mc->kvm_type) {
         type = mc->kvm_type(ms, NULL);
+    } else {
+        type = kvm_arch_get_default_type(ms);
     }
 
     do {
diff --git a/hw/mips/loongson3_virt.c b/hw/mips/loongson3_virt.c
index 4018b8c1d3..a2c56f7a21 100644
--- a/hw/mips/loongson3_virt.c
+++ b/hw/mips/loongson3_virt.c
@@ -29,7 +29,6 @@
 #include "qemu/datadir.h"
 #include "qapi/error.h"
 #include "elf.h"
-#include "kvm_mips.h"
 #include "hw/char/serial.h"
 #include "hw/intc/loongson_liointc.h"
 #include "hw/mips/mips.h"
@@ -612,7 +611,6 @@ static void loongson3v_machine_class_init(ObjectClass *oc, void *data)
     mc->max_cpus = LOONGSON_MAX_VCPUS;
     mc->default_ram_id = "loongson3.highram";
     mc->default_ram_size = 1600 * MiB;
-    mc->kvm_type = mips_kvm_type;
     mc->minimum_page_bits = 14;
     mc->default_nic = "virtio-net-pci";
 }
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index b4c7654f49..40f577bfd5 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -247,6 +247,11 @@ int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
     return ret > 0 ? ret : 40;
 }
 
+int kvm_arch_get_default_type(MachineState *ms)
+{
+    return 0;
+}
+
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     int ret = 0;
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index ebfaf3d24c..b45ce20fd8 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -2556,6 +2556,11 @@ static void register_smram_listener(Notifier *n, void *unused)
                                  &smram_address_space, 1, "kvm-smram");
 }
 
+int kvm_arch_get_default_type(MachineState *ms)
+{
+    return 0;
+}
+
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     uint64_t identity_base = 0xfffbc000;
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index c14e8f550f..e98aad01bd 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -1266,7 +1266,7 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
     abort();
 }
 
-int mips_kvm_type(MachineState *machine, const char *vm_type)
+int kvm_arch_get_default_type(MachineState *machine)
 {
 #if defined(KVM_CAP_MIPS_VZ)
     int r;
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index a8a935e267..dc1182cd37 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -108,6 +108,11 @@ static int kvm_ppc_register_host_cpu_type(void);
 static void kvmppc_get_cpu_characteristics(KVMState *s);
 static int kvmppc_get_dec_bits(void);
 
+int kvm_arch_get_default_type(MachineState *ms)
+{
+    return 0;
+}
+
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     cap_interrupt_unset = kvm_check_extension(s, KVM_CAP_PPC_UNSET_IRQ);
diff --git a/target/riscv/kvm.c b/target/riscv/kvm.c
index 9d8a8982f9..4266dce092 100644
--- a/target/riscv/kvm.c
+++ b/target/riscv/kvm.c
@@ -907,6 +907,11 @@ int kvm_arch_add_msi_route_post(struct kvm_irq_routing_entry *route,
     return 0;
 }
 
+int kvm_arch_get_default_type(MachineState *ms)
+{
+    return 0;
+}
+
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     return 0;
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index a9e5880349..9117fab6e8 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -340,6 +340,11 @@ static void ccw_machine_class_foreach(ObjectClass *oc, void *opaque)
     mc->default_cpu_type = S390_CPU_TYPE_NAME("host");
 }
 
+int kvm_arch_get_default_type(MachineState *ms)
+{
+    return 0;
+}
+
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     object_class_foreach(ccw_machine_class_foreach, TYPE_S390_CCW_MACHINE,
-- 
2.41.0

Re: [PATCH v5 1/6] kvm: Introduce kvm_arch_get_default_type hook

[Peter Maydell]

On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> kvm_arch_get_default_type() returns the default KVM type. This hook is
> particularly useful to derive a KVM type that is valid for "none"
> machine model, which is used by libvirt to probe the availability of
> KVM.
>
> For MIPS, the existing mips_kvm_type() is reused. This function ensures
> the availability of VZ which is mandatory to use KVM on the current
> QEMU.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  include/sysemu/kvm.h     | 2 ++
>  target/mips/kvm_mips.h   | 9 ---------
>  accel/kvm/kvm-all.c      | 4 +++-
>  hw/mips/loongson3_virt.c | 2 --
>  target/arm/kvm.c         | 5 +++++
>  target/i386/kvm/kvm.c    | 5 +++++
>  target/mips/kvm.c        | 2 +-
>  target/ppc/kvm.c         | 5 +++++
>  target/riscv/kvm.c       | 5 +++++
>  target/s390x/kvm/kvm.c   | 5 +++++
>  10 files changed, 31 insertions(+), 13 deletions(-)
>
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index 115f0cca79..ccaf55caf7 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -369,6 +369,8 @@ int kvm_arch_get_registers(CPUState *cpu);
>
>  int kvm_arch_put_registers(CPUState *cpu, int level);
>
> +int kvm_arch_get_default_type(MachineState *ms);
> +

New global functions should have a doc comment that explains
what they do, what their API is, etc. For instance, is
this allowed to return an error, and if so, how ?

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v5 1/6] kvm: Introduce kvm_arch_get_default_type hook

[Peter Maydell]

On Fri, 4 Aug 2023 at 18:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> > kvm_arch_get_default_type() returns the default KVM type. This hook is
> > particularly useful to derive a KVM type that is valid for "none"
> > machine model, which is used by libvirt to probe the availability of
> > KVM.
> >
> > For MIPS, the existing mips_kvm_type() is reused. This function ensures
> > the availability of VZ which is mandatory to use KVM on the current
> > QEMU.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >  include/sysemu/kvm.h     | 2 ++
> >  target/mips/kvm_mips.h   | 9 ---------
> >  accel/kvm/kvm-all.c      | 4 +++-
> >  hw/mips/loongson3_virt.c | 2 --
> >  target/arm/kvm.c         | 5 +++++
> >  target/i386/kvm/kvm.c    | 5 +++++
> >  target/mips/kvm.c        | 2 +-
> >  target/ppc/kvm.c         | 5 +++++
> >  target/riscv/kvm.c       | 5 +++++
> >  target/s390x/kvm/kvm.c   | 5 +++++
> >  10 files changed, 31 insertions(+), 13 deletions(-)
> >
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index 115f0cca79..ccaf55caf7 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -369,6 +369,8 @@ int kvm_arch_get_registers(CPUState *cpu);
> >
> >  int kvm_arch_put_registers(CPUState *cpu, int level);
> >
> > +int kvm_arch_get_default_type(MachineState *ms);
> > +
>
> New global functions should have a doc comment that explains
> what they do, what their API is, etc. For instance, is
> this allowed to return an error, and if so, how ?

Looks like this was the only issue with this patchset. So
I propose to take this into my target-arm queue for 8.2,
with the following doc comment added:

/**
 * kvm_arch_get_default_type: Return default KVM type
 * @ms: MachineState of the VM being created
 *
 * Return the default type argument to use in the
 * KVM_CREATE_VM ioctl when creating the VM. This will
 * only be used when the machine model did not specify a
 * type to use via the MachineClass::kvm_type method.
 *
 * Returns: type to use, or a negative value on error.
 */

unless anybody wants more time for review or for this
series to go into the tree some other way.

thanks
-- PMM

Re: [PATCH v5 1/6] kvm: Introduce kvm_arch_get_default_type hook

[Philippe Mathieu-Daudé]

On 4/8/23 19:38, Peter Maydell wrote:
> On Fri, 4 Aug 2023 at 18:26, Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>
>>> kvm_arch_get_default_type() returns the default KVM type. This hook is
>>> particularly useful to derive a KVM type that is valid for "none"
>>> machine model, which is used by libvirt to probe the availability of
>>> KVM.
>>>
>>> For MIPS, the existing mips_kvm_type() is reused. This function ensures
>>> the availability of VZ which is mandatory to use KVM on the current
>>> QEMU.
>>>
>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>> ---
>>>   include/sysemu/kvm.h     | 2 ++
>>>   target/mips/kvm_mips.h   | 9 ---------
>>>   accel/kvm/kvm-all.c      | 4 +++-
>>>   hw/mips/loongson3_virt.c | 2 --
>>>   target/arm/kvm.c         | 5 +++++
>>>   target/i386/kvm/kvm.c    | 5 +++++
>>>   target/mips/kvm.c        | 2 +-
>>>   target/ppc/kvm.c         | 5 +++++
>>>   target/riscv/kvm.c       | 5 +++++
>>>   target/s390x/kvm/kvm.c   | 5 +++++
>>>   10 files changed, 31 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
>>> index 115f0cca79..ccaf55caf7 100644
>>> --- a/include/sysemu/kvm.h
>>> +++ b/include/sysemu/kvm.h
>>> @@ -369,6 +369,8 @@ int kvm_arch_get_registers(CPUState *cpu);
>>>
>>>   int kvm_arch_put_registers(CPUState *cpu, int level);
>>>
>>> +int kvm_arch_get_default_type(MachineState *ms);
>>> +
>>
>> New global functions should have a doc comment that explains
>> what they do, what their API is, etc. For instance, is
>> this allowed to return an error, and if so, how ?
> 
> Looks like this was the only issue with this patchset. So
> I propose to take this into my target-arm queue for 8.2,
> with the following doc comment added:
> 
> /**
>   * kvm_arch_get_default_type: Return default KVM type
>   * @ms: MachineState of the VM being created
>   *
>   * Return the default type argument to use in the
>   * KVM_CREATE_VM ioctl when creating the VM. This will
>   * only be used when the machine model did not specify a
>   * type to use via the MachineClass::kvm_type method.
>   *
>   * Returns: type to use, or a negative value on error.
>   */

Thank you.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v5 2/6] accel/kvm: Specify default IPA size for arm64

[Akihiko Odaki]

Before this change, the default KVM type, which is used for non-virt
machine models, was 0.

The kernel documentation says:
> On arm64, the physical address size for a VM (IPA Size limit) is
> limited to 40bits by default. The limit can be configured if the host
> supports the extension KVM_CAP_ARM_VM_IPA_SIZE. When supported, use
> KVM_VM_TYPE_ARM_IPA_SIZE(IPA_Bits) to set the size in the machine type
> identifier, where IPA_Bits is the maximum width of any physical
> address used by the VM. The IPA_Bits is encoded in bits[7-0] of the
> machine type identifier.
>
> e.g, to configure a guest to use 48bit physical address size::
>
>     vm_fd = ioctl(dev_fd, KVM_CREATE_VM, KVM_VM_TYPE_ARM_IPA_SIZE(48));
>
> The requested size (IPA_Bits) must be:
>
>  ==   =========================================================
>   0   Implies default size, 40bits (for backward compatibility)
>   N   Implies N bits, where N is a positive integer such that,
>       32 <= N <= Host_IPA_Limit
>  ==   =========================================================

> Host_IPA_Limit is the maximum possible value for IPA_Bits on the host
> and is dependent on the CPU capability and the kernel configuration.
> The limit can be retrieved using KVM_CAP_ARM_VM_IPA_SIZE of the
> KVM_CHECK_EXTENSION ioctl() at run-time.
>
> Creation of the VM will fail if the requested IPA size (whether it is
> implicit or explicit) is unsupported on the host.
https://docs.kernel.org/virt/kvm/api.html#kvm-create-vm

So if Host_IPA_Limit < 40, specifying 0 as the type will fail. This
actually confused libvirt, which uses "none" machine model to probe the
KVM availability, on M2 MacBook Air.

Fix this by using Host_IPA_Limit as the default type when
KVM_CAP_ARM_VM_IPA_SIZE is available.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/arm/kvm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 40f577bfd5..23aeb09949 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -249,7 +249,9 @@ int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool *fixed_ipa)
 
 int kvm_arch_get_default_type(MachineState *ms)
 {
-    return 0;
+    bool fixed_ipa;
+    int size = kvm_arm_get_max_vm_ipa_size(ms, &fixed_ipa);
+    return fixed_ipa ? 0 : size;
 }
 
 int kvm_arch_init(MachineState *ms, KVMState *s)
-- 
2.41.0

Re: [PATCH v5 2/6] accel/kvm: Specify default IPA size for arm64

[Peter Maydell]

On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Before this change, the default KVM type, which is used for non-virt
> machine models, was 0.
>
> The kernel documentation says:
> > On arm64, the physical address size for a VM (IPA Size limit) is
> > limited to 40bits by default. The limit can be configured if the host
> > supports the extension KVM_CAP_ARM_VM_IPA_SIZE. When supported, use
> > KVM_VM_TYPE_ARM_IPA_SIZE(IPA_Bits) to set the size in the machine type
> > identifier, where IPA_Bits is the maximum width of any physical
> > address used by the VM. The IPA_Bits is encoded in bits[7-0] of the
> > machine type identifier.
> >
> > e.g, to configure a guest to use 48bit physical address size::
> >
> >     vm_fd = ioctl(dev_fd, KVM_CREATE_VM, KVM_VM_TYPE_ARM_IPA_SIZE(48));
> >
> > The requested size (IPA_Bits) must be:
> >
> >  ==   =========================================================
> >   0   Implies default size, 40bits (for backward compatibility)
> >   N   Implies N bits, where N is a positive integer such that,
> >       32 <= N <= Host_IPA_Limit
> >  ==   =========================================================
>
> > Host_IPA_Limit is the maximum possible value for IPA_Bits on the host
> > and is dependent on the CPU capability and the kernel configuration.
> > The limit can be retrieved using KVM_CAP_ARM_VM_IPA_SIZE of the
> > KVM_CHECK_EXTENSION ioctl() at run-time.
> >
> > Creation of the VM will fail if the requested IPA size (whether it is
> > implicit or explicit) is unsupported on the host.
> https://docs.kernel.org/virt/kvm/api.html#kvm-create-vm
>
> So if Host_IPA_Limit < 40, specifying 0 as the type will fail. This
> actually confused libvirt, which uses "none" machine model to probe the
> KVM availability, on M2 MacBook Air.
>
> Fix this by using Host_IPA_Limit as the default type when
> KVM_CAP_ARM_VM_IPA_SIZE is available.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v5 3/6] mips: Report an error when KVM_VM_MIPS_VZ is unavailable

[Akihiko Odaki]

On MIPS, QEMU requires KVM_VM_MIPS_VZ type for KVM. Report an error in
such a case as other architectures do when an error occurred during KVM
type decision.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 target/mips/kvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index e98aad01bd..e22e24ed97 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -1278,6 +1278,7 @@ int kvm_arch_get_default_type(MachineState *machine)
     }
 #endif
 
+    error_report("KVM_VM_MIPS_VZ type is not available");
     return -1;
 }
 
-- 
2.41.0

Re: [PATCH v5 3/6] mips: Report an error when KVM_VM_MIPS_VZ is unavailable

[Peter Maydell]

On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On MIPS, QEMU requires KVM_VM_MIPS_VZ type for KVM. Report an error in
> such a case as other architectures do when an error occurred during KVM
> type decision.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  target/mips/kvm.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> index e98aad01bd..e22e24ed97 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -1278,6 +1278,7 @@ int kvm_arch_get_default_type(MachineState *machine)
>      }
>  #endif
>
> +    error_report("KVM_VM_MIPS_VZ type is not available");
>      return -1;
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v5 3/6] mips: Report an error when KVM_VM_MIPS_VZ is unavailable

[Philippe Mathieu-Daudé]

On 27/7/23 09:31, Akihiko Odaki wrote:
> On MIPS, QEMU requires KVM_VM_MIPS_VZ type for KVM. Report an error in
> such a case as other architectures do when an error occurred during KVM
> type decision.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   target/mips/kvm.c | 1 +
>   1 file changed, 1 insertion(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v5 4/6] accel/kvm: Use negative KVM type for error propagation

[Akihiko Odaki]

On MIPS, kvm_arch_get_default_type() returns a negative value when an
error occurred so handle the case. Also, let other machines return
negative values when errors occur and declare returning a negative
value as the correct way to propagate an error that happened when
determining KVM type.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 accel/kvm/kvm-all.c | 5 +++++
 hw/arm/virt.c       | 2 +-
 hw/ppc/spapr.c      | 2 +-
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d591b5079c..94a62efa3c 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2527,6 +2527,11 @@ static int kvm_init(MachineState *ms)
         type = kvm_arch_get_default_type(ms);
     }
 
+    if (type < 0) {
+        ret = -EINVAL;
+        goto err;
+    }
+
     do {
         ret = kvm_ioctl(s, KVM_CREATE_VM, type);
     } while (ret == -EINTR);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8a4c663735..161f3ffbf7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2977,7 +2977,7 @@ static int virt_kvm_type(MachineState *ms, const char *type_str)
                      "require an IPA range (%d bits) larger than "
                      "the one supported by the host (%d bits)",
                      requested_pa_size, max_vm_pa_size);
-        exit(1);
+        return -1;
     }
     /*
      * We return the requested PA log size, unless KVM only supports
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 54dbfd7fe9..1b522e8e40 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3104,7 +3104,7 @@ static int spapr_kvm_type(MachineState *machine, const char *vm_type)
     }
 
     error_report("Unknown kvm-type specified '%s'", vm_type);
-    exit(1);
+    return -1;
 }
 
 /*
-- 
2.41.0

Re: [PATCH v5 4/6] accel/kvm: Use negative KVM type for error propagation

[Peter Maydell]

On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On MIPS, kvm_arch_get_default_type() returns a negative value when an
> error occurred so handle the case. Also, let other machines return
> negative values when errors occur and declare returning a negative
> value as the correct way to propagate an error that happened when
> determining KVM type.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  accel/kvm/kvm-all.c | 5 +++++
>  hw/arm/virt.c       | 2 +-
>  hw/ppc/spapr.c      | 2 +-
>  3 files changed, 7 insertions(+), 2 deletions(-)

I might have put this earlier in the series, but we get to
the same place in the end whichever way around we do it.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v5 4/6] accel/kvm: Use negative KVM type for error propagation

[Philippe Mathieu-Daudé]

On 27/7/23 09:31, Akihiko Odaki wrote:
> On MIPS, kvm_arch_get_default_type() returns a negative value when an
> error occurred so handle the case. Also, let other machines return
> negative values when errors occur and declare returning a negative
> value as the correct way to propagate an error that happened when
> determining KVM type.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   accel/kvm/kvm-all.c | 5 +++++
>   hw/arm/virt.c       | 2 +-
>   hw/ppc/spapr.c      | 2 +-
>   3 files changed, 7 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v5 5/6] accel/kvm: Free as when an error occurred

[Akihiko Odaki]

An error may occur after s->as is allocated, for example while
determining KVM type.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 accel/kvm/kvm-all.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 94a62efa3c..4591669d78 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2765,6 +2765,7 @@ err:
     if (s->fd != -1) {
         close(s->fd);
     }
+    g_free(s->as);
     g_free(s->memory_listener.slots);
 
     return ret;
-- 
2.41.0

Re: [PATCH v5 5/6] accel/kvm: Free as when an error occurred

[Peter Maydell]

On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> An error may occur after s->as is allocated, for example while
> determining KVM type.

That's about the one example you don't want to cite, because
it makes it sound like this is only a problem because
of a bug in the previous patch. In fact we already have
lots of 'goto err' paths after the allocation of s->as,
such as the one when kvm_ioctl(KVM_CREATE_VM) fails.

> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  accel/kvm/kvm-all.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index 94a62efa3c..4591669d78 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -2765,6 +2765,7 @@ err:
>      if (s->fd != -1) {
>          close(s->fd);
>      }
> +    g_free(s->as);
>      g_free(s->memory_listener.slots);
>
>      return ret;

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

[PATCH v5 6/6] accel/kvm: Make kvm_dirty_ring_reaper_init() void

[Akihiko Odaki]

The returned value was always zero and had no meaning.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 accel/kvm/kvm-all.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index 4591669d78..a4a1b4e05d 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1454,15 +1454,13 @@ static void *kvm_dirty_ring_reaper_thread(void *data)
     return NULL;
 }
 
-static int kvm_dirty_ring_reaper_init(KVMState *s)
+static void kvm_dirty_ring_reaper_init(KVMState *s)
 {
     struct KVMDirtyRingReaper *r = &s->reaper;
 
     qemu_thread_create(&r->reaper_thr, "kvm-reaper",
                        kvm_dirty_ring_reaper_thread,
                        s, QEMU_THREAD_JOINABLE);
-
-    return 0;
 }
 
 static int kvm_dirty_ring_init(KVMState *s)
@@ -2744,10 +2742,7 @@ static int kvm_init(MachineState *ms)
     }
 
     if (s->kvm_dirty_ring_size) {
-        ret = kvm_dirty_ring_reaper_init(s);
-        if (ret) {
-            goto err;
-        }
+        kvm_dirty_ring_reaper_init(s);
     }
 
     if (kvm_check_extension(kvm_state, KVM_CAP_BINARY_STATS_FD)) {
-- 
2.41.0

Re: [PATCH v5 6/6] accel/kvm: Make kvm_dirty_ring_reaper_init() void

[Peter Maydell]

On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> The returned value was always zero and had no meaning.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v5 6/6] accel/kvm: Make kvm_dirty_ring_reaper_init() void

[Philippe Mathieu-Daudé]

On 27/7/23 09:31, Akihiko Odaki wrote:
> The returned value was always zero and had no meaning.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   accel/kvm/kvm-all.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v5 0/6] accel/kvm: Specify default IPA size for arm64

[Peter Maydell]

On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Some Arm systems such as Apple Silicon Macs have IPA size smaller than the
> default used by KVM. Introduce our own default IPA size that fits on such a
> system.
>
> When reviewing this series, Philippe Mathieu-Daudé found the error handling
> around KVM type decision logic is flawed so I added a few patches for fixing
> the error handling path.
>
> V4 -> V5: Fixed KVM type error handling
> V3 -> V4: Removed an inclusion of kvm_mips.h that is no longer needed.
> V2 -> V3: Changed to use the maximum IPA size as the default.
> V1 -> V2: Introduced an arch hook

Applied to target-arm-for-8.2 with an extra doc comment in patch 1;
thanks.

-- PMM

Re: [PATCH v5 0/6] accel/kvm: Specify default IPA size for arm64

[Peter Maydell]

On Fri, 4 Aug 2023 at 18:41, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 27 Jul 2023 at 08:31, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> > Some Arm systems such as Apple Silicon Macs have IPA size smaller than the
> > default used by KVM. Introduce our own default IPA size that fits on such a
> > system.
> >
> > When reviewing this series, Philippe Mathieu-Daudé found the error handling
> > around KVM type decision logic is flawed so I added a few patches for fixing
> > the error handling path.
> >
> > V4 -> V5: Fixed KVM type error handling
> > V3 -> V4: Removed an inclusion of kvm_mips.h that is no longer needed.
> > V2 -> V3: Changed to use the maximum IPA size as the default.
> > V1 -> V2: Introduced an arch hook
>
> Applied to target-arm-for-8.2 with an extra doc comment in patch 1;
> thanks.

I also figured it would be good to tag the first 2 patches
for qemu-stable, so I'll do that as well.

thanks
-- PMM