Re: [PATCH v5 2/6] ARM/sysctl: Expose the supported guest GIC modes in physinfo

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 11-Sep-26 14:47, Julian Vetter wrote:
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> In preparation to simplify the domain creation logic surrounding GIC
> version.
> 
> On a GICv3 host, also report support for GICv2-compatible guests when
> the hardware's vGICv2 compatibility mode is enabled, rather than just
> the native GIC version.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Julian Vetter <julian.vetter@xxxxxxxxxx>
> ---
> Changes in v5:
> - Add vgic_v2_hw_enabled() to new vgic implementation.
> ---
>  xen/arch/arm/include/asm/vgic.h |  6 ++++++
>  xen/arch/arm/sysctl.c           | 34 +++++++++++++++++++++++++++++++++
>  xen/arch/arm/vgic-v2.c          |  5 +++++
>  xen/arch/arm/vgic/vgic-v2.c     |  5 +++++
>  xen/include/public/sysctl.h     |  2 ++
>  5 files changed, 52 insertions(+)
> 
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index 6f9ab1c98c..26c53aaf3c 100644
> --- a/xen/arch/arm/include/asm/vgic.h
> +++ b/xen/arch/arm/include/asm/vgic.h
> @@ -433,6 +433,12 @@ unsigned int vgic_max_vcpus(unsigned int 
> domctl_vgic_version);
>  void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
>                        paddr_t vbase, uint32_t aliased_offset);
>  
> +#ifdef CONFIG_VGICV2
> +bool vgic_v2_hw_enabled(void);
> +#else
> +static inline bool vgic_v2_hw_enabled(void) { return false; }
> +#endif
> +
>  #ifdef CONFIG_GICV3
>  struct rdist_region;
>  void vgic_v3_setup_hw(paddr_t dbase,
> diff --git a/xen/arch/arm/sysctl.c b/xen/arch/arm/sysctl.c
> index 32cab4feff..8411deb7e2 100644
> --- a/xen/arch/arm/sysctl.c
> +++ b/xen/arch/arm/sysctl.c
> @@ -12,7 +12,11 @@
>  #include <xen/dt-overlay.h>
>  #include <xen/errno.h>
>  #include <xen/hypercall.h>
> +
>  #include <asm/arm64/sve.h>
> +#include <asm/gic.h>
> +#include <asm/vgic.h>
> +
>  #include <public/sysctl.h>
>  
>  void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
> @@ -21,6 +25,36 @@ void arch_do_physinfo(struct xen_sysctl_physinfo *pi)
>  
>      pi->arch_capabilities |= MASK_INSR(sve_encode_vl(get_sys_vl_len()),
>                                         XEN_SYSCTL_PHYSCAP_ARM_SVE_MASK);
> +
> +    /*
> +     * The GIC version(s) we're happy creating guests with. Right now for
> +     * simplicity it is tied to the active hardware version, but this will
> +     * cease to be the case if/when the compatibility modes are enabled.
You carry on this comment but as we are now also informing about the legacy
GICv2 mode (which was not there in v1) I no longer understand what it denotes
and why we need it. I think we can drop this.

> +     */
> +    switch ( gic_hw_version() )
> +    {
> +    case GIC_V2:
> +        pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_ARM_GIC_V2;
> +        break;
> +
> +    case GIC_V3:
> +        pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_ARM_GIC_V3;
> +
> +        /* GICv3 may additionally support GICv2-compatible guests. */
> +        if ( vgic_v2_hw_enabled() )
> +            pi->arch_capabilities |= XEN_SYSCTL_PHYSCAP_ARM_GIC_V2;
> +        break;
> +
> +    case GIC_INVALID:
Change to default please to make MISRA C R16.4 happy.

> +        /*
> +         * Running a control domain without having the GIC sorted yet?
> +         * Something's broken, but there's nothing we can do about it here.
> +         */
> +        ASSERT_UNREACHABLE();
> +        printk_once(XENLOG_ERR "Unrecognised GIC version %d\n",
> +                    gic_hw_version());
Please swap the printk with ASSERT.

With these remarks addressed:
Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal