Re: [PATCH 1/2] hw/arm/virt: Consolidate GIC finalize logic

[Alexander Graf]

Hey Zengui,

On 21.12.22 04:35, Zenghui Yu wrote:
> On 2022/12/21 7:04, Alexander Graf wrote:
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index c7dd59d7f1..365d19f7a3 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -109,12 +109,12 @@ typedef enum VirtMSIControllerType {
> >  } VirtMSIControllerType;
> >
> >  typedef enum VirtGICType {
> > -    VIRT_GIC_VERSION_MAX,
> > -    VIRT_GIC_VERSION_HOST,
> > -    VIRT_GIC_VERSION_2,
> > -    VIRT_GIC_VERSION_3,
> > -    VIRT_GIC_VERSION_4,
> > -    VIRT_GIC_VERSION_NOSEL,
> > +    VIRT_GIC_VERSION_MAX = 0,
> > +    VIRT_GIC_VERSION_HOST = 1,
> > +    VIRT_GIC_VERSION_NOSEL = 2,
> > +    VIRT_GIC_VERSION_2 = (1 << 2),
> > +    VIRT_GIC_VERSION_3 = (1 << 3),
> > +    VIRT_GIC_VERSION_4 = (1 << 4),
>
> This would break the ACPI case. When building the MADT, we currently
> write the raw vms->gic_version value into "GIC version" field of the
> GICD structure. This happens to work because VIRT_GIC_VERSION_x == x (by
> luck, I think). We may need to fix build_madt() before this change.


Ouch, thanks a lot for the catch! I don't think it's by luck - the 
versions are put very deliberately at a place where they equal the gic 
number. But I agree that it's missing a comment - I'll add one for 
clarification and make sure the defines looks explicit in v2.


Alex