Re: [PATCH v16 14/14] hw/vmapple/vmapple: Add vmapple machine type

[Philippe Mathieu-Daudé]

On 27/12/24 21:12, Phil Dennis-Jordan wrote:

>      > diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
>      > index bcd1be63e3..6a4c4a7fa2 100644
>      > --- a/hw/vmapple/Kconfig
>      > +++ b/hw/vmapple/Kconfig
>      > @@ -10,3 +10,23 @@ config VMAPPLE_CFG
>      >   config VMAPPLE_VIRTIO_BLK
>      >       bool
>      >
>      > +config VMAPPLE
>      > +    bool
>      > +    depends on ARM
>      > +    depends on HVF
>      > +    default y if ARM
>      > +    imply PCI_DEVICES
>      > +    select ARM_GIC
>
>     Hmmm I'm getting ...:
>
>     qemu-system-aarch64: unknown type 'arm-gicv3'
>
>      > +    select PLATFORM_BUS
>      > +    select PCI_EXPRESS
>      > +    select PCI_EXPRESS_GENERIC_BRIDGE
>      > +    select PL011 # UART
>      > +    select PL031 # RTC
>      > +    select PL061 # GPIO
>      > +    select GPIO_PWR
>      > +    select PVPANIC_MMIO
>      > +    select VMAPPLE_AES
>      > +    select VMAPPLE_BDIF
>      > +    select VMAPPLE_CFG
>      > +    select MAC_PVG_MMIO
>      > +    select VMAPPLE_VIRTIO_BLK
>
>
>      > +static void create_gic(VMAppleMachineState *vms, MemoryRegion *mem)
>      > +{
>      > +    MachineState *ms = MACHINE(vms);
>      > +    /* We create a standalone GIC */
>      > +    SysBusDevice *gicbusdev;
>      > +    QList *redist_region_count;
>      > +    int i;
>      > +    unsigned int smp_cpus = ms->smp.cpus;
>      > +
>      > +    vms->gic = qdev_new(gicv3_class_name());
>
>     ... I suppose due to this call ^^^.
>
>     $ git grep arm-gicv3
>     hw/intc/arm_gicv3_kvm.c:45:#define TYPE_KVM_ARM_GICV3 "kvm-arm-gicv3"
>     include/hw/intc/arm_gicv3.h:18:#define TYPE_ARM_GICV3 "arm-gicv3"
>     $ git grep TYPE_ARM_GICV3
>     hw/intc/arm_gicv3.c:466:    .name = TYPE_ARM_GICV3,
>     $ git grep -FW arm_gicv3.c
>     hw/intc/meson.build=9=system_ss.add(when: 'CONFIG_ARM_GICV3_TCG',
>     if_true: files(
>     hw/intc/meson.build:10:  'arm_gicv3.c',
>     ...
>
>
> Ahhh, good catch! I suppose this is with —disable-tcg (or equivalent)

Yes, this is how I test HVF.

>     I think commit a8a5546798c ("hw/intc/arm_gicv3: Introduce
>     CONFIG_ARM_GIC_TCG Kconfig selector") is invalid as being
>     too restrictive.
>
>     I can go a bit further with these changes on top (ignoring
>     renaming ARM_GICV3_TCG -> ARM_GICV3):
>
>
>     -- >8 --
>     diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
>     index dd405bdb5d2..9e06c05b449 100644
>     --- a/hw/intc/Kconfig
>     +++ b/hw/intc/Kconfig
>     @@ -26 +26 @@ config ARM_GIC
>     -    select ARM_GICV3_TCG if TCG
>     +    select ARM_GICV3_TCG if TCG || HVF
>     @@ -32 +32 @@ config ARM_GICV3_TCG
>     -    depends on ARM_GIC && TCG
>     +    depends on ARM_GIC && (TCG || HVF)

Now implemented as [*]:
20241227202435.48055-1-philmd@linaro.org/">https://lore.kernel.org/qemu-devel/20241227202435.48055-1-philmd@linaro.org/

>     diff --git a/hw/vmapple/Kconfig b/hw/vmapple/Kconfig
>     index 6a4c4a7fa2e..374a89f6a8f 100644
>     --- a/hw/vmapple/Kconfig
>     +++ b/hw/vmapple/Kconfig
>     @@ -19 +19 @@ config VMAPPLE
>     -    select ARM_GIC
>     +    select ARM_GICV3_TCG
>
>
> Is this last part necessary/advisable? It would seem like the above 
> changes in hw/intc/Kconfig should make ARM_GIC work too?
> (The PVG dependency means we currently can’t support anything other than 
> macOS host systems and thus HVF or theoretically TCG anyway, but if QEMU 
> gained support for the HVF-provided GIC implementation, we’d need to 
> change this line again.)

Hmm indeed we can skip it, but vmapple machine enforces rev=3:

>      > +    qdev_prop_set_uint32(vms->gic, "revision", 3);

So directly selecting ARM_GICV3 sounds more explicit to me.
The diff is now (on top of [*]):

     -    select ARM_GIC
     +    select ARM_GICV3

WDYT?