[PATCH v2] ARM/vgic: Clean up vgic_v{2,3}_setup_hw()

[PATCH v2] ARM/vgic: Clean up vgic_v{2,3}_setup_hw()

[Michal Orzel]

From: Andrew Cooper <andrew.cooper3@citrix.com>

vgic_v{2,3}_setup_hw()'s callers are __init, so they should be too.
vgic_v{2,3}_hw and gic_v2_hw_data are written once during init and
unmodified thereafter, so make them __ro_after_init.  Reposition
'bool enabled' in these structures to fit in the tail padding, removing
8 bytes from their size when paddr_t is 8B.

While at it, drop dead vgic_v3_setup_hw() dummy implementation
from vgic/vgic.c. GICV3 depends on !NEW_VGIC.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
---
Changes in v2 (Michal):
 - take Andrew's v1 patch and extend the changes to vGICv3 and new vGICv2
---
 xen/arch/arm/vgic-v2.c      |  8 ++++----
 xen/arch/arm/vgic-v3.c      | 11 +++++------
 xen/arch/arm/vgic/vgic-v2.c |  8 ++++----
 xen/arch/arm/vgic/vgic.c    | 11 -----------
 4 files changed, 13 insertions(+), 25 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 642407fd5b05..3fa8cdeeab14 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -25,7 +25,6 @@
 #include <asm/vreg.h>
 
 static struct {
-    bool enabled;
     /* Distributor interface address */
     paddr_t dbase;
     /* CPU interface address & size */
@@ -36,10 +35,11 @@ static struct {
 
     /* Offset to add to get an 8kB contiguous region if GIC is aliased */
     uint32_t aliased_offset;
-} vgic_v2_hw;
+    bool enabled;
+} vgic_v2_hw __ro_after_init;
 
-void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
-                      paddr_t vbase, uint32_t aliased_offset)
+void __init vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
+                             paddr_t vbase, uint32_t aliased_offset)
 {
     vgic_v2_hw.enabled = true;
     vgic_v2_hw.dbase = dbase;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index c01cc596d593..16e9d0cbad03 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -44,19 +44,18 @@
 #define VGICD_CTLR_DEFAULT  (GICD_CTLR_ARE_NS)
 
 static struct {
-    bool enabled;
     /* Distributor interface address */
     paddr_t dbase;
     /* Re-distributor regions */
     unsigned int nr_rdist_regions;
     const struct rdist_region *regions;
     unsigned int intid_bits;  /* Number of interrupt ID bits */
-} vgic_v3_hw;
+    bool enabled;
+} vgic_v3_hw __ro_after_init;
 
-void vgic_v3_setup_hw(paddr_t dbase,
-                      unsigned int nr_rdist_regions,
-                      const struct rdist_region *regions,
-                      unsigned int intid_bits)
+void __init vgic_v3_setup_hw(paddr_t dbase, unsigned int nr_rdist_regions,
+                             const struct rdist_region *regions,
+                             unsigned int intid_bits)
 {
     vgic_v3_hw.enabled = true;
     vgic_v3_hw.dbase = dbase;
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index 6a558089c522..06fa36545355 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -24,7 +24,6 @@
 #include "vgic.h"
 
 static struct {
-    bool enabled;
     paddr_t dbase;          /* Distributor interface address */
     paddr_t cbase;          /* CPU interface address & size */
     paddr_t csize;
@@ -32,10 +31,11 @@ static struct {
 
     /* Offset to add to get an 8kB contiguous region if GIC is aliased */
     uint32_t aliased_offset;
-} gic_v2_hw_data;
+    bool enabled;
+} gic_v2_hw_data __ro_after_init;
 
-void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
-                      paddr_t vbase, uint32_t aliased_offset)
+void __init vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
+                             paddr_t vbase, uint32_t aliased_offset)
 {
     gic_v2_hw_data.enabled = true;
     gic_v2_hw_data.dbase = dbase;
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index b2c0e1873ace..ba029b8a3bbf 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -964,17 +964,6 @@ unsigned int vgic_max_vcpus(unsigned int domctl_vgic_version)
     }
 }
 
-#ifdef CONFIG_GICV3
-/* Dummy implementation to allow building without actual vGICv3 support. */
-void vgic_v3_setup_hw(paddr_t dbase,
-                      unsigned int nr_rdist_regions,
-                      const struct rdist_region *regions,
-                      unsigned int intid_bits)
-{
-    panic("New VGIC implementation does not yet support GICv3\n");
-}
-#endif
-
 /*
  * Local variables:
  * mode: C
-- 
2.43.0

Re: [PATCH v2] ARM/vgic: Clean up vgic_v{2,3}_setup_hw()

[Julien Grall]

Hi Michal,

On 28/08/2026 08:04, Michal Orzel wrote:
> From: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> vgic_v{2,3}_setup_hw()'s callers are __init, so they should be too.
> vgic_v{2,3}_hw and gic_v2_hw_data are written once during init and
> unmodified thereafter, so make them __ro_after_init.  Reposition
> 'bool enabled' in these structures to fit in the tail padding, removing
> 8 bytes from their size when paddr_t is 8B.
> 
> While at it, drop dead vgic_v3_setup_hw() dummy implementation
> from vgic/vgic.c. GICV3 depends on !NEW_VGIC.

I am not sure about this one. There are logics in the new vGIC which are 
GICv3 specific so technically not reachable. However, I would argue they 
should not be remove as the eventual goal as always been to move to a 
different GIC (our current vGIC is not spec compliant). For this 
specific change, it is easy to re-add so ...

> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Michal Orzel <michal.orzel@amd.com>

Reviewed-by: Julien Grall <julien@xen.org>

Cheers,

-- 
Julien Grall