Re: [RFC PATCH RESEND 4/4] hw/arm/virt: Add Target Implementation CPU support

[Eric Auger <eric.auger@redhat.com>]

Hi Shameer,
On 8/1/25 9:47 AM, Shameer Kolothum wrote:
> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>
> Add the impl-cpu property to virt machine. This will enable
> user to specify the target implementation CPUs as the example
> below:
>
> ...
> -M impl-cpu.0.midr=1,impl-cpu.0.revidr=1,impl-cpu.0.aidr=1,\
> impl-cpu.1.midr=2,impl-cpu.1.revidr=2,impl-cpu.1.aidr=2 \
>
> Make use of helper functions to check the KVM support for target
> Impl CPUs and if supported set the user specified target CPUs.

I think you need to document the new virt options in
docs/system/arm/virt.rst

>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  hw/arm/virt.c         | 66 +++++++++++++++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  3 ++
>  2 files changed, 69 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index a96452f17a..72a0cd3ea8 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -71,6 +71,7 @@
>  #include "hw/firmware/smbios.h"
>  #include "qapi/visitor.h"
>  #include "qapi/qapi-visit-common.h"
> +#include "qapi/qapi-visit-machine.h"
>  #include "qobject/qlist.h"
>  #include "standard-headers/linux/input.h"
>  #include "hw/arm/smmuv3.h"
> @@ -2232,6 +2233,20 @@ static void machvirt_init(MachineState *machine)
>          exit(1);
>      }
>  
> +    if (vms->target_cpus_num) {
> +        if (!kvm_enabled()) {
> +            error_report("Target Impl CPU requested, but not supported "
> +                         "without  KVM");
> +            exit(1);
> +        }
> +
> +        if (!kvm_arm_target_impl_cpus_supported()) {
> +            error_report("Target Impl CPU requested, but not supported by KVM");
> +            exit(1);
> +        }
> +        kvm_arm_set_target_impl_cpus(vms->target_cpus_num, vms->target_cpus);
> +    }
> +
>      create_fdt(vms);
>  
>      assert(possible_cpus->len == max_cpus);
> @@ -2666,6 +2681,45 @@ static void virt_set_oem_table_id(Object *obj, const char *value,
>      strncpy(vms->oem_table_id, value, 8);
>  }
>  
> +static void virt_set_target_impl_cpus(Object *obj, Visitor *v,
> +                                       const char *name, void *opaque,
> +                                       Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +    ArmTargetImplCPUList *list = NULL;
> +    ArmTargetImplCPUList *iter = NULL;
> +    ArmTargetImplCPU *target_cpus;
> +    uint64_t target_num = 0;
> +    int i;
> +
> +    visit_type_ArmTargetImplCPUList(v, name, &list, errp);
> +    if (!list) {
> +        return;
> +    }
> +
> +    for (iter = list; iter; iter = iter->next) {
> +        target_num++;
> +    }
> +
> +    target_cpus = g_new0(ArmTargetImplCPU, target_num);
> +    for (i = 0, iter = list; iter; iter = iter->next, i++) {
> +        target_cpus[i].midr = iter->value->midr;
> +        target_cpus[i].revidr = iter->value->revidr;
> +        target_cpus[i].aidr = iter->value->aidr;
> +    }
Can't you avoid having both an array and a list. Can you use the list
all the way?
> +    vms->target_cpus_num = target_num;
> +    vms->target_cpus = target_cpus;
> +    vms->target_cpus_list = list;
> +}
> +
> +static void virt_get_target_impl_cpus(Object *obj, Visitor *v, const char *name,
> +                             void *opaque, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +    ArmTargetImplCPUList **list = &vms->target_cpus_list;
> +
> +    visit_type_ArmTargetImplCPUList(v, name, list, errp);
> +}
>  
>  bool virt_is_acpi_enabled(VirtMachineState *vms)
>  {
> @@ -3326,6 +3380,18 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>                                            "Override the default value of field OEM Table ID "
>                                            "in ACPI table header."
>                                            "The string may be up to 8 bytes in size");
> +    object_class_property_add(oc, "impl-cpu", "ArmTargetImplCPU",
> +                              virt_get_target_impl_cpus,
> +                              virt_set_target_impl_cpus,
> +                              NULL, NULL);
> +    object_class_property_set_description(oc, "impl-cpu",
> +                                          "Describe target implementation CPU in the format: "
> +                                          "impl-cpu.0.midr=1,"
> +                                          "impl-cpu.0.revidr=1,"
> +                                          "impl-cpu.0.aidr=1,"
> +                                          "impl-cpu.1.midr=2,"
> +                                          "impl-cpu.1.revidr=2,"
> +                                          "impl-cpu.1.aidr=2");
>  
>  }
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index c8e94e6aed..cf462fcb37 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -180,6 +180,9 @@ struct VirtMachineState {
>      char *oem_id;
>      char *oem_table_id;
>      bool ns_el2_virt_timer_irq;
> +    uint64_t target_cpus_num;
> +    ArmTargetImplCPU *target_cpus;
> +    ArmTargetImplCPUList *target_cpus_list;
>  };
>  
>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
Thanks

Eric