[PATCH v2 0/4] Move alloc/free_vcpu_struct() to common code

[PATCH v2 0/4] Move alloc/free_vcpu_struct() to common code

[Oleksii Kurochko]

As it was suggested in [1] it would be better to allocate one page for struct
vcpu for all arch-es. To do that it is needed to align Arm code to allocate
one page (as there is a case(when CONFIG_NEW_VGIC=y) when Arm64 will require
to allocate two pages). As a result, the following patches for Arm have been
introduced:
 - [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu
 - [PATCH v2 2/4] xen/arm: drop MAX_PAGES_PER_VCPU

This patches are dependency for:
 - [PATCH v2 3/4] xen: move alloc/free_vcpu_struct() to common code

Also, as a part of this patch series another clean up is done which makes
{alloc,free}_domain_struct() static.

[1] https://lore.kernel.org/xen-devel/f8a9be3a-a0c6-496a-806f-40760dca5aee@suse.com/T/#m275dfcbdccef0461fa9a8acef072403f18091768

---
Changes in v2:
 - Introduce new patches for Arm:
     - [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu
     - [PATCH v2 2/4] xen/arm: drop MAX_PAGES_PER_VCPU
    to allocate one page for struct vcpu in common code for all the arch-es.
 - Introduce patch to clean up xen/domain.h a little bit:
     - [PATCH v2 4/4] xen/common: make {alloc,free}_domain_struct() static
 - Address the comments from v1:
     - [PATCH v2 3/4] xen: move alloc/free_vcpu_struct() to common code
---

Oleksii Kurochko (4):
  xen/arm: optimize the size of struct vcpu
  xen/arm: drop MAX_PAGES_PER_VCPU
  xen: move alloc/free_vcpu_struct() to common code
  xen/common: make {alloc,free}_domain_struct() static

 xen/arch/arm/domain.c             | 32 --------------
 xen/arch/arm/gic-vgic.c           | 48 ++++++++++-----------
 xen/arch/arm/include/asm/domain.h |  2 +-
 xen/arch/arm/vgic-v3.c            | 34 +++++++--------
 xen/arch/arm/vgic.c               | 72 +++++++++++++++++--------------
 xen/arch/arm/vgic/vgic-init.c     | 10 ++++-
 xen/arch/arm/vgic/vgic-v2.c       |  4 +-
 xen/arch/arm/vgic/vgic.c          | 50 ++++++++++-----------
 xen/arch/ppc/stubs.c              | 10 -----
 xen/arch/riscv/stubs.c            | 10 -----
 xen/arch/x86/domain.c             | 17 +-------
 xen/arch/x86/include/asm/domain.h |  3 ++
 xen/common/domain.c               | 26 ++++++++++-
 xen/include/xen/domain.h          |  8 ----
 14 files changed, 145 insertions(+), 181 deletions(-)

-- 
2.52.0

[PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Oleksii Kurochko]

When CONFIG_NEW_VGIC=y and CONFIG_ARM_64=y, the size of struct vcpu
exceeds one page, which requires allocating two pages and led to the
introduction of MAX_PAGES_PER_VCPU.

To remove the need for MAX_PAGES_PER_VCPU in a follow-up patch, the vgic
member of struct arch_vcpu is changed to a pointer to struct vgic_cpu.
As a result, the size of struct vcpu for Arm64 is reduced to 2048 bytes,
compared to 3840 bytes (without these changes and with CONFIG_ARM_64=y)
and 4736 bytes (without these changes and with both CONFIG_ARM_64=y and
CONFIG_NEW_VGIC=y).

Since the vgic member is now a pointer, vcpu_vgic_init() and
vcpu_vgic_free() are updated to allocate and free the struct vgic_cpu
instance dynamically.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - New patch.
---
 xen/arch/arm/gic-vgic.c           | 48 ++++++++++-----------
 xen/arch/arm/include/asm/domain.h |  2 +-
 xen/arch/arm/vgic-v3.c            | 34 +++++++--------
 xen/arch/arm/vgic.c               | 72 +++++++++++++++++--------------
 xen/arch/arm/vgic/vgic-init.c     | 10 ++++-
 xen/arch/arm/vgic/vgic-v2.c       |  4 +-
 xen/arch/arm/vgic/vgic.c          | 50 ++++++++++-----------
 7 files changed, 116 insertions(+), 104 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index ea48c5375a..482b77c986 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -42,12 +42,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
 {
     struct pending_irq *iter;
 
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&v->arch.vgic->lock));
 
     if ( !list_empty(&n->lr_queue) )
         return;
 
-    list_for_each_entry ( iter, &v->arch.vgic.lr_pending, lr_queue )
+    list_for_each_entry ( iter, &v->arch.vgic->lr_pending, lr_queue )
     {
         if ( iter->priority > n->priority )
         {
@@ -55,12 +55,12 @@ static inline void gic_add_to_lr_pending(struct vcpu *v, struct pending_irq *n)
             return;
         }
     }
-    list_add_tail(&n->lr_queue, &v->arch.vgic.lr_pending);
+    list_add_tail(&n->lr_queue, &v->arch.vgic->lr_pending);
 }
 
 void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p)
 {
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&v->arch.vgic->lock));
 
     list_del_init(&p->lr_queue);
 }
@@ -73,7 +73,7 @@ void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
     if ( unlikely(!n) )
         return;
 
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&v->arch.vgic->lock));
 
     /* Don't try to update the LR if the interrupt is disabled */
     if ( !test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
@@ -104,7 +104,7 @@ static unsigned int gic_find_unused_lr(struct vcpu *v,
 {
     uint64_t *lr_mask = &this_cpu(lr_mask);
 
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&v->arch.vgic->lock));
 
     if ( unlikely(test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status)) )
     {
@@ -130,13 +130,13 @@ void gic_raise_guest_irq(struct vcpu *v, unsigned int virtual_irq,
     unsigned int nr_lrs = gic_get_nr_lrs();
     struct pending_irq *p = irq_to_pending(v, virtual_irq);
 
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&v->arch.vgic->lock));
 
     if ( unlikely(!p) )
         /* An unmapped LPI does not need to be raised. */
         return;
 
-    if ( v == current && list_empty(&v->arch.vgic.lr_pending) )
+    if ( v == current && list_empty(&v->arch.vgic->lr_pending) )
     {
         i = gic_find_unused_lr(v, p, 0);
 
@@ -156,7 +156,7 @@ static void gic_update_one_lr(struct vcpu *v, int i)
     int irq;
     struct gic_lr lr_val;
 
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&v->arch.vgic->lock));
     ASSERT(!local_irq_is_enabled());
 
     gic_hw_ops->read_lr(i, &lr_val);
@@ -253,7 +253,7 @@ void vgic_sync_from_lrs(struct vcpu *v)
 
     gic_hw_ops->update_hcr_status(GICH_HCR_UIE, false);
 
-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+    spin_lock_irqsave(&v->arch.vgic->lock, flags);
 
     while ((i = find_next_bit((const unsigned long *) &this_cpu(lr_mask),
                               nr_lrs, i)) < nr_lrs ) {
@@ -261,7 +261,7 @@ void vgic_sync_from_lrs(struct vcpu *v)
         i++;
     }
 
-    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+    spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
 }
 
 static void gic_restore_pending_irqs(struct vcpu *v)
@@ -274,13 +274,13 @@ static void gic_restore_pending_irqs(struct vcpu *v)
 
     ASSERT(!local_irq_is_enabled());
 
-    spin_lock(&v->arch.vgic.lock);
+    spin_lock(&v->arch.vgic->lock);
 
-    if ( list_empty(&v->arch.vgic.lr_pending) )
+    if ( list_empty(&v->arch.vgic->lr_pending) )
         goto out;
 
-    inflight_r = &v->arch.vgic.inflight_irqs;
-    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
+    inflight_r = &v->arch.vgic->inflight_irqs;
+    list_for_each_entry_safe ( p, t, &v->arch.vgic->lr_pending, lr_queue )
     {
         lr = gic_find_unused_lr(v, p, lr);
         if ( lr >= nr_lrs )
@@ -318,17 +318,17 @@ found:
     }
 
 out:
-    spin_unlock(&v->arch.vgic.lock);
+    spin_unlock(&v->arch.vgic->lock);
 }
 
 void gic_clear_pending_irqs(struct vcpu *v)
 {
     struct pending_irq *p, *t;
 
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&v->arch.vgic->lock));
 
     v->arch.lr_mask = 0;
-    list_for_each_entry_safe ( p, t, &v->arch.vgic.lr_pending, lr_queue )
+    list_for_each_entry_safe ( p, t, &v->arch.vgic->lr_pending, lr_queue )
         gic_remove_from_lr_pending(v, p);
 }
 
@@ -357,14 +357,14 @@ int vgic_vcpu_pending_irq(struct vcpu *v)
     mask_priority = gic_hw_ops->read_vmcr_priority();
     active_priority = find_first_bit(&apr, 32);
 
-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+    spin_lock_irqsave(&v->arch.vgic->lock, flags);
 
     /* TODO: We order the guest irqs by priority, but we don't change
      * the priority of host irqs. */
 
     /* find the first enabled non-active irq, the queue is already
      * ordered by priority */
-    list_for_each_entry( p, &v->arch.vgic.inflight_irqs, inflight )
+    list_for_each_entry( p, &v->arch.vgic->inflight_irqs, inflight )
     {
         if ( GIC_PRI_TO_GUEST(p->priority) >= mask_priority )
             goto out;
@@ -378,7 +378,7 @@ int vgic_vcpu_pending_irq(struct vcpu *v)
     }
 
 out:
-    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+    spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
     return rc;
 }
 
@@ -388,7 +388,7 @@ void vgic_sync_to_lrs(void)
 
     gic_restore_pending_irqs(current);
 
-    if ( !list_empty(&current->arch.vgic.lr_pending) && lr_all_full() )
+    if ( !list_empty(&current->arch.vgic->lr_pending) && lr_all_full() )
         gic_hw_ops->update_hcr_status(GICH_HCR_UIE, true);
 }
 
@@ -396,10 +396,10 @@ void gic_dump_vgic_info(struct vcpu *v)
 {
     struct pending_irq *p;
 
-    list_for_each_entry ( p, &v->arch.vgic.inflight_irqs, inflight )
+    list_for_each_entry ( p, &v->arch.vgic->inflight_irqs, inflight )
         printk("Inflight irq=%u lr=%u\n", p->irq, p->lr);
 
-    list_for_each_entry( p, &v->arch.vgic.lr_pending, lr_queue )
+    list_for_each_entry( p, &v->arch.vgic->lr_pending, lr_queue )
         printk("Pending irq=%d\n", p->irq);
 }
 
diff --git a/xen/arch/arm/include/asm/domain.h b/xen/arch/arm/include/asm/domain.h
index 758ad807e4..6cfa793828 100644
--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -230,7 +230,7 @@ struct arch_vcpu
     union gic_state_data gic;
     uint64_t lr_mask;
 
-    struct vgic_cpu vgic;
+    struct vgic_cpu *vgic;
 
     /* Timer registers  */
     register_t cntkctl;
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 77aab5c3c2..a9bb7e8906 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -162,10 +162,10 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
             goto read_as_zero_32;
         if ( dabt.size != DABT_WORD ) goto bad_width;
 
-        spin_lock_irqsave(&v->arch.vgic.lock, flags);
-        *r = vreg_reg32_extract(!!(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED),
+        spin_lock_irqsave(&v->arch.vgic->lock, flags);
+        *r = vreg_reg32_extract(!!(v->arch.vgic->flags & VGIC_V3_LPIS_ENABLED),
                                 info);
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
         return 1;
     }
 
@@ -195,7 +195,7 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
         /* We use the VCPU ID as the redistributor ID in bits[23:8] */
         typer |= v->vcpu_id << GICR_TYPER_PROC_NUM_SHIFT;
 
-        if ( v->arch.vgic.flags & VGIC_V3_RDIST_LAST )
+        if ( v->arch.vgic->flags & VGIC_V3_RDIST_LAST )
             typer |= GICR_TYPER_LAST;
 
         if ( v->domain->arch.vgic.has_its )
@@ -249,7 +249,7 @@ static int __vgic_v3_rdistr_rd_mmio_read(struct vcpu *v, mmio_info_t *info,
             goto read_as_zero_64;
         if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
 
-        val = read_atomic(&v->arch.vgic.rdist_pendbase);
+        val = read_atomic(&v->arch.vgic->rdist_pendbase);
         val &= ~GICR_PENDBASER_PTZ;      /* WO, reads as 0 */
         *r = vreg_reg64_extract(val, info);
         return 1;
@@ -467,7 +467,7 @@ static void vgic_vcpu_enable_lpis(struct vcpu *v)
         smp_mb();
     }
 
-    v->arch.vgic.flags |= VGIC_V3_LPIS_ENABLED;
+    v->arch.vgic->flags |= VGIC_V3_LPIS_ENABLED;
 }
 
 static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
@@ -488,14 +488,14 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
         if ( dabt.size != DABT_WORD ) goto bad_width;
 
         vgic_lock(v);                   /* protects rdists_enabled */
-        spin_lock_irqsave(&v->arch.vgic.lock, flags);
+        spin_lock_irqsave(&v->arch.vgic->lock, flags);
 
         /* LPIs can only be enabled once, but never disabled again. */
         if ( (r & GICR_CTLR_ENABLE_LPIS) &&
-             !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
+             !(v->arch.vgic->flags & VGIC_V3_LPIS_ENABLED) )
             vgic_vcpu_enable_lpis(v);
 
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
         vgic_unlock(v);
 
         return 1;
@@ -565,18 +565,18 @@ static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
             goto write_ignore_64;
         if ( !vgic_reg64_check_access(dabt) ) goto bad_width;
 
-        spin_lock_irqsave(&v->arch.vgic.lock, flags);
+        spin_lock_irqsave(&v->arch.vgic->lock, flags);
 
         /* Writing PENDBASER with LPIs enabled is UNPREDICTABLE. */
-        if ( !(v->arch.vgic.flags & VGIC_V3_LPIS_ENABLED) )
+        if ( !(v->arch.vgic->flags & VGIC_V3_LPIS_ENABLED) )
         {
-            reg = read_atomic(&v->arch.vgic.rdist_pendbase);
+            reg = read_atomic(&v->arch.vgic->rdist_pendbase);
             vreg_reg64_update(&reg, r, info);
             reg = sanitize_pendbaser(reg);
-            write_atomic(&v->arch.vgic.rdist_pendbase, reg);
+            write_atomic(&v->arch.vgic->rdist_pendbase, reg);
         }
 
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
 
         return 1;
     }
@@ -1115,7 +1115,7 @@ static struct vcpu *get_vcpu_from_rdist(struct domain *d,
 
     v = d->vcpu[vcpu_id];
 
-    *offset = gpa - v->arch.vgic.rdist_base;
+    *offset = gpa - v->arch.vgic->rdist_base;
 
     return v;
 }
@@ -1745,7 +1745,7 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
         return -EINVAL;
     }
 
-    v->arch.vgic.rdist_base = rdist_base;
+    v->arch.vgic->rdist_base = rdist_base;
 
     /*
      * If the redistributor is the last one of the
@@ -1756,7 +1756,7 @@ static int vgic_v3_vcpu_init(struct vcpu *v)
     last_cpu = (region->size / GICV3_GICR_SIZE) + region->first_cpu - 1;
 
     if ( v->vcpu_id == last_cpu || (v->vcpu_id == (d->max_vcpus - 1)) )
-        v->arch.vgic.flags |= VGIC_V3_RDIST_LAST;
+        v->arch.vgic->flags |= VGIC_V3_RDIST_LAST;
 
     return 0;
 }
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 3ebdf9953f..8b17871b86 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -84,7 +84,7 @@ static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
                                                   unsigned int rank)
 {
     if ( rank == 0 )
-        return v->arch.vgic.private_irqs;
+        return v->arch.vgic->private_irqs;
     else if ( rank <= DOMAIN_NR_RANKS(v->domain) )
         return &v->domain->arch.vgic.shared_irqs[rank - 1];
     else if ( is_valid_espi_rank(v->domain, rank) )
@@ -370,29 +370,35 @@ int vcpu_vgic_init(struct vcpu *v)
 {
     int i;
 
-    v->arch.vgic.private_irqs = xzalloc(struct vgic_irq_rank);
-    if ( v->arch.vgic.private_irqs == NULL )
+    v->arch.vgic = xzalloc(struct vgic_cpu);
+    if ( v->arch.vgic == NULL )
+        return -ENOMEM;
+
+    v->arch.vgic->private_irqs = xzalloc(struct vgic_irq_rank);
+    if ( v->arch.vgic->private_irqs == NULL )
       return -ENOMEM;
 
     /* SGIs/PPIs are always routed to this VCPU */
-    vgic_rank_init(v->arch.vgic.private_irqs, 0, v->vcpu_id);
+    vgic_rank_init(v->arch.vgic->private_irqs, 0, v->vcpu_id);
 
     v->domain->arch.vgic.handler->vcpu_init(v);
 
-    memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
+    memset(&v->arch.vgic->pending_irqs, 0, sizeof(v->arch.vgic->pending_irqs));
     for (i = 0; i < 32; i++)
-        vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i);
+        vgic_init_pending_irq(&v->arch.vgic->pending_irqs[i], i);
 
-    INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
-    INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
-    spin_lock_init(&v->arch.vgic.lock);
+    INIT_LIST_HEAD(&v->arch.vgic->inflight_irqs);
+    INIT_LIST_HEAD(&v->arch.vgic->lr_pending);
+    spin_lock_init(&v->arch.vgic->lock);
 
     return 0;
 }
 
 int vcpu_vgic_free(struct vcpu *v)
 {
-    xfree(v->arch.vgic.private_irqs);
+    xfree(v->arch.vgic->private_irqs);
+    xfree(v->arch.vgic);
+
     return 0;
 }
 
@@ -423,14 +429,14 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     /* This will never be called for an LPI, as we don't migrate them. */
     ASSERT(!is_lpi(irq));
 
-    spin_lock_irqsave(&old->arch.vgic.lock, flags);
+    spin_lock_irqsave(&old->arch.vgic->lock, flags);
 
     p = irq_to_pending(old, irq);
 
     /* nothing to do for virtual interrupts */
     if ( p->desc == NULL )
     {
-        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&old->arch.vgic->lock, flags);
         return true;
     }
 
@@ -438,7 +444,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     if ( test_bit(GIC_IRQ_GUEST_MIGRATING, &p->status) )
     {
         gprintk(XENLOG_WARNING, "irq %u migration failed: requested while in progress\n", irq);
-        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&old->arch.vgic->lock, flags);
         return false;
     }
 
@@ -447,7 +453,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     if ( list_empty(&p->inflight) )
     {
         irq_set_affinity(p->desc, cpumask_of(new->processor));
-        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&old->arch.vgic->lock, flags);
         return true;
     }
     /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
@@ -455,7 +461,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     {
         vgic_remove_irq_from_queues(old, p);
         irq_set_affinity(p->desc, cpumask_of(new->processor));
-        spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&old->arch.vgic->lock, flags);
         vgic_inject_irq(new->domain, new, irq, true);
         return true;
     }
@@ -464,7 +470,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
     if ( !list_empty(&p->inflight) )
         set_bit(GIC_IRQ_GUEST_MIGRATING, &p->status);
 
-    spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
+    spin_unlock_irqrestore(&old->arch.vgic->lock, flags);
     return true;
 }
 
@@ -516,12 +522,12 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, unsigned int n)
         irq = i + (32 * n);
         v_target = vgic_get_target_vcpu(v, irq);
 
-        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+        spin_lock_irqsave(&v_target->arch.vgic->lock, flags);
         p = irq_to_pending(v_target, irq);
         clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
         gic_remove_from_lr_pending(v_target, p);
         desc = p->desc;
-        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&v_target->arch.vgic->lock, flags);
 
         if ( desc != NULL )
         {
@@ -567,12 +573,12 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, unsigned int n)
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
         v_target = vgic_get_target_vcpu(v, irq);
-        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+        spin_lock_irqsave(&v_target->arch.vgic->lock, flags);
         p = irq_to_pending(v_target, irq);
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
         if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
             gic_raise_guest_irq(v_target, irq, p->priority);
-        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&v_target->arch.vgic->lock, flags);
         if ( p->desc != NULL )
         {
             irq_set_affinity(p->desc, cpumask_of(v_target->processor));
@@ -701,7 +707,7 @@ struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
     /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
      * are used for SPIs; the rests are used for per cpu irqs */
     if ( irq < 32 )
-        n = &v->arch.vgic.pending_irqs[irq];
+        n = &v->arch.vgic->pending_irqs[irq];
     else if ( is_lpi(irq) )
         n = v->domain->arch.vgic.handler->lpi_to_pending(v->domain, irq);
     else
@@ -734,16 +740,16 @@ void vgic_clear_pending_irqs(struct vcpu *v)
     struct pending_irq *p, *t;
     unsigned long flags;
 
-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
-    list_for_each_entry_safe ( p, t, &v->arch.vgic.inflight_irqs, inflight )
+    spin_lock_irqsave(&v->arch.vgic->lock, flags);
+    list_for_each_entry_safe ( p, t, &v->arch.vgic->inflight_irqs, inflight )
         list_del_init(&p->inflight);
     gic_clear_pending_irqs(v);
-    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+    spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
 }
 
 void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
 {
-    ASSERT(spin_is_locked(&v->arch.vgic.lock));
+    ASSERT(spin_is_locked(&v->arch.vgic->lock));
 
     clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
     list_del_init(&p->inflight);
@@ -772,20 +778,20 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
         v = vgic_get_target_vcpu(d->vcpu[0], virq);
     };
 
-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+    spin_lock_irqsave(&v->arch.vgic->lock, flags);
 
     n = irq_to_pending(v, virq);
     /* If an LPI has been removed, there is nothing to inject here. */
     if ( unlikely(!n) )
     {
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
         return;
     }
 
     /* vcpu offline */
     if ( test_bit(_VPF_down, &v->pause_flags) )
     {
-        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
         return;
     }
 
@@ -804,7 +810,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
     if ( test_bit(GIC_IRQ_GUEST_ENABLED, &n->status) )
         gic_raise_guest_irq(v, virq, priority);
 
-    list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
+    list_for_each_entry ( iter, &v->arch.vgic->inflight_irqs, inflight )
     {
         if ( iter->priority > priority )
         {
@@ -812,9 +818,9 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v, unsigned int virq,
             goto out;
         }
     }
-    list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
+    list_add_tail(&n->inflight, &v->arch.vgic->inflight_irqs);
 out:
-    spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
+    spin_unlock_irqrestore(&v->arch.vgic->lock, flags);
 
     /* we have a new higher priority irq, inject it into the guest */
     vcpu_kick(v);
@@ -924,7 +930,7 @@ void vgic_check_inflight_irqs_pending(struct vcpu *v, unsigned int rank, uint32_
 
         v_target = vgic_get_target_vcpu(v, irq);
 
-        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+        spin_lock_irqsave(&v_target->arch.vgic->lock, flags);
 
         p = irq_to_pending(v_target, irq);
 
@@ -933,7 +939,7 @@ void vgic_check_inflight_irqs_pending(struct vcpu *v, unsigned int rank, uint32_
                    "%pv trying to clear pending interrupt %u.\n",
                    v, irq);
 
-        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+        spin_unlock_irqrestore(&v_target->arch.vgic->lock, flags);
     }
 }
 
diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
index f8d7d3a226..67f297797f 100644
--- a/xen/arch/arm/vgic/vgic-init.c
+++ b/xen/arch/arm/vgic/vgic-init.c
@@ -57,7 +57,7 @@
  */
 static void vgic_vcpu_early_init(struct vcpu *vcpu)
 {
-    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
+    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
     unsigned int i;
 
     INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
@@ -202,6 +202,10 @@ int vcpu_vgic_init(struct vcpu *v)
 {
     int ret = 0;
 
+    v->arch.vgic = xzalloc(struct vgic_cpu);
+    if ( v->arch.vgic == NULL )
+        return -ENOMEM;
+
     vgic_vcpu_early_init(v);
 
     if ( gic_hw_version() == GIC_V2 )
@@ -241,10 +245,12 @@ void domain_vgic_free(struct domain *d)
 
 int vcpu_vgic_free(struct vcpu *v)
 {
-    struct vgic_cpu *vgic_cpu = &v->arch.vgic;
+    struct vgic_cpu *vgic_cpu = v->arch.vgic;
 
     INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
 
+    xfree(vgic_cpu);
+
     return 0;
 }
 
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index 6a558089c5..e64d681dd2 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -56,8 +56,8 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
  */
 void vgic_v2_fold_lr_state(struct vcpu *vcpu)
 {
-    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
-    unsigned int used_lrs = vcpu->arch.vgic.used_lrs;
+    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
+    unsigned int used_lrs = vcpu->arch.vgic->used_lrs;
     unsigned long flags;
     unsigned int lr;
 
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index b2c0e1873a..146bd124c3 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -40,8 +40,8 @@
  * When taking more than one ap_list_lock at the same time, always take the
  * lowest numbered VCPU's ap_list_lock first, so:
  *   vcpuX->vcpu_id < vcpuY->vcpu_id:
- *     spin_lock(vcpuX->arch.vgic.ap_list_lock);
- *     spin_lock(vcpuY->arch.vgic.ap_list_lock);
+ *     spin_lock(vcpuX->arch.vgic->ap_list_lock);
+ *     spin_lock(vcpuY->arch.vgic->ap_list_lock);
  *
  * Since the VGIC must support injecting virtual interrupts from ISRs, we have
  * to use the spin_lock_irqsave/spin_unlock_irqrestore versions of outer
@@ -102,7 +102,7 @@ struct vgic_irq *vgic_get_irq(struct domain *d, struct vcpu *vcpu,
 {
     /* SGIs and PPIs */
     if ( intid <= VGIC_MAX_PRIVATE )
-        return &vcpu->arch.vgic.private_irqs[intid];
+        return &vcpu->arch.vgic->private_irqs[intid];
 
     /* SPIs */
     if ( intid <= VGIC_MAX_SPI )
@@ -245,7 +245,7 @@ out:
 /* Must be called with the ap_list_lock held */
 static void vgic_sort_ap_list(struct vcpu *vcpu)
 {
-    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
+    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
 
     ASSERT(spin_is_locked(&vgic_cpu->ap_list_lock));
 
@@ -323,7 +323,7 @@ retry:
 
     /* someone can do stuff here, which we re-check below */
 
-    spin_lock_irqsave(&vcpu->arch.vgic.ap_list_lock, flags);
+    spin_lock_irqsave(&vcpu->arch.vgic->ap_list_lock, flags);
     spin_lock(&irq->irq_lock);
 
     /*
@@ -341,7 +341,7 @@ retry:
     if ( unlikely(irq->vcpu || vcpu != vgic_target_oracle(irq)) )
     {
         spin_unlock(&irq->irq_lock);
-        spin_unlock_irqrestore(&vcpu->arch.vgic.ap_list_lock, flags);
+        spin_unlock_irqrestore(&vcpu->arch.vgic->ap_list_lock, flags);
 
         spin_lock_irqsave(&irq->irq_lock, flags);
         goto retry;
@@ -352,11 +352,11 @@ retry:
      * now in the ap_list.
      */
     vgic_get_irq_kref(irq);
-    list_add_tail(&irq->ap_list, &vcpu->arch.vgic.ap_list_head);
+    list_add_tail(&irq->ap_list, &vcpu->arch.vgic->ap_list_head);
     irq->vcpu = vcpu;
 
     spin_unlock(&irq->irq_lock);
-    spin_unlock_irqrestore(&vcpu->arch.vgic.ap_list_lock, flags);
+    spin_unlock_irqrestore(&vcpu->arch.vgic->ap_list_lock, flags);
 
     vcpu_kick(vcpu);
 
@@ -422,7 +422,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *vcpu, unsigned int intid,
  */
 static void vgic_prune_ap_list(struct vcpu *vcpu)
 {
-    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
+    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
     struct vgic_irq *irq, *tmp;
     unsigned long flags;
 
@@ -487,8 +487,8 @@ retry:
             vcpuB = vcpu;
         }
 
-        spin_lock_irqsave(&vcpuA->arch.vgic.ap_list_lock, flags);
-        spin_lock(&vcpuB->arch.vgic.ap_list_lock);
+        spin_lock_irqsave(&vcpuA->arch.vgic->ap_list_lock, flags);
+        spin_lock(&vcpuB->arch.vgic->ap_list_lock);
         spin_lock(&irq->irq_lock);
 
         /*
@@ -502,7 +502,7 @@ retry:
          */
         if ( target_vcpu == vgic_target_oracle(irq) )
         {
-            struct vgic_cpu *new_cpu = &target_vcpu->arch.vgic;
+            struct vgic_cpu *new_cpu = target_vcpu->arch.vgic;
 
             list_del(&irq->ap_list);
             irq->vcpu = target_vcpu;
@@ -510,8 +510,8 @@ retry:
         }
 
         spin_unlock(&irq->irq_lock);
-        spin_unlock(&vcpuB->arch.vgic.ap_list_lock);
-        spin_unlock_irqrestore(&vcpuA->arch.vgic.ap_list_lock, flags);
+        spin_unlock(&vcpuB->arch.vgic->ap_list_lock);
+        spin_unlock_irqrestore(&vcpuA->arch.vgic->ap_list_lock, flags);
         goto retry;
     }
 
@@ -542,7 +542,7 @@ static void vgic_set_underflow(struct vcpu *vcpu)
 /* Requires the ap_list_lock to be held. */
 static int compute_ap_list_depth(struct vcpu *vcpu)
 {
-    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
+    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
     struct vgic_irq *irq;
     int count = 0;
 
@@ -557,7 +557,7 @@ static int compute_ap_list_depth(struct vcpu *vcpu)
 /* Requires the VCPU's ap_list_lock to be held. */
 static void vgic_flush_lr_state(struct vcpu *vcpu)
 {
-    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
+    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
     struct vgic_irq *irq;
     int count = 0;
 
@@ -583,7 +583,7 @@ static void vgic_flush_lr_state(struct vcpu *vcpu)
         }
     }
 
-    vcpu->arch.vgic.used_lrs = count;
+    vcpu->arch.vgic->used_lrs = count;
 }
 
 /**
@@ -600,7 +600,7 @@ static void vgic_flush_lr_state(struct vcpu *vcpu)
 void vgic_sync_from_lrs(struct vcpu *vcpu)
 {
     /* An empty ap_list_head implies used_lrs == 0 */
-    if ( list_empty(&vcpu->arch.vgic.ap_list_head) )
+    if ( list_empty(&vcpu->arch.vgic->ap_list_head) )
         return;
 
     vgic_fold_lr_state(vcpu);
@@ -628,14 +628,14 @@ void vgic_sync_to_lrs(void)
      * and introducing additional synchronization mechanism doesn't change
      * this.
      */
-    if ( list_empty(&current->arch.vgic.ap_list_head) )
+    if ( list_empty(&current->arch.vgic->ap_list_head) )
         return;
 
     ASSERT(!local_irq_is_enabled());
 
-    spin_lock(&current->arch.vgic.ap_list_lock);
+    spin_lock(&current->arch.vgic->ap_list_lock);
     vgic_flush_lr_state(current);
-    spin_unlock(&current->arch.vgic.ap_list_lock);
+    spin_unlock(&current->arch.vgic->ap_list_lock);
 
     gic_hw_ops->update_hcr_status(GICH_HCR_EN, 1);
 }
@@ -652,7 +652,7 @@ void vgic_sync_to_lrs(void)
  */
 int vgic_vcpu_pending_irq(struct vcpu *vcpu)
 {
-    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
+    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
     struct vgic_irq *irq;
     unsigned long flags;
     int ret = 0;
@@ -761,11 +761,11 @@ void vgic_free_virq(struct domain *d, unsigned int virq)
 
 void gic_dump_vgic_info(struct vcpu *v)
 {
-    struct vgic_cpu *vgic_cpu = &v->arch.vgic;
+    struct vgic_cpu *vgic_cpu = v->arch.vgic;
     struct vgic_irq *irq;
     unsigned long flags;
 
-    spin_lock_irqsave(&v->arch.vgic.ap_list_lock, flags);
+    spin_lock_irqsave(&v->arch.vgic->ap_list_lock, flags);
 
     if ( !list_empty(&vgic_cpu->ap_list_head) )
         printk("   active or pending interrupts queued:\n");
@@ -781,7 +781,7 @@ void gic_dump_vgic_info(struct vcpu *v)
         spin_unlock(&irq->irq_lock);
     }
 
-    spin_unlock_irqrestore(&v->arch.vgic.ap_list_lock, flags);
+    spin_unlock_irqrestore(&v->arch.vgic->ap_list_lock, flags);
 }
 
 void vgic_clear_pending_irqs(struct vcpu *v)
-- 
2.52.0

[PATCH v2 2/4] xen/arm: drop MAX_PAGES_PER_VCPU

[Oleksii Kurochko]

Now that the size of struct vcpu is smaller than PAGE_SIZE,
MAX_PAGES_PER_VCPU is no longer needed and is removed.

This change also updates {alloc,free}_vcpu_struct().

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - New patch.
---
 xen/arch/arm/domain.c | 25 +++++--------------------
 1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 47973f99d9..e566023340 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -473,36 +473,21 @@ void dump_pageframe_info(struct domain *d)
 
 }
 
-/*
- * The new VGIC has a bigger per-IRQ structure, so we need more than one
- * page on ARM64. Cowardly increase the limit in this case.
- */
-#if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
-#define MAX_PAGES_PER_VCPU  2
-#else
-#define MAX_PAGES_PER_VCPU  1
-#endif
-
 struct vcpu *alloc_vcpu_struct(const struct domain *d)
 {
     struct vcpu *v;
 
-    BUILD_BUG_ON(sizeof(*v) > MAX_PAGES_PER_VCPU * PAGE_SIZE);
-    v = alloc_xenheap_pages(get_order_from_bytes(sizeof(*v)), 0);
-    if ( v != NULL )
-    {
-        unsigned int i;
-
-        for ( i = 0; i < DIV_ROUND_UP(sizeof(*v), PAGE_SIZE); i++ )
-            clear_page((void *)v + i * PAGE_SIZE);
-    }
+    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
+    v = alloc_xenheap_pages(0, 0);
+    if ( v )
+        clear_page(v);
 
     return v;
 }
 
 void free_vcpu_struct(struct vcpu *v)
 {
-    free_xenheap_pages(v, get_order_from_bytes(sizeof(*v)));
+    free_xenheap_page(v);
 }
 
 int arch_vcpu_create(struct vcpu *v)
-- 
2.52.0

[PATCH v2 3/4] xen: move alloc/free_vcpu_struct() to common code

[Oleksii Kurochko]

alloc_vcpu_struct() and free_vcpu_struct() contain little
architecture-specific logic and are suitable for sharing across
architectures. Move both helpers to common code.

To support the remaining architectural differences, introduce
arch_vcpu_struct_memflags(), allowing architectures to override the
memory flags passed to alloc_xenheap_pages(). This is currently needed
by x86, which may require MEMF_bits(32) for HVM guests using shadow
paging.

The ARM implementation of alloc/free_vcpu_struct() is removed and
replaced by the common version. Stub implementations are also dropped
from PPC and RISC-V.

Finally, make alloc_vcpu_struct() and free_vcpu_struct() static to
common/domain.c, as they are no longer used outside common code.

No functional changes.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - Rework alloc/free_vcpu_struct() to work with only one page.
 - Return back the comment about the restriction inside x86's
   arch_vcpu_struct_memflags().
 - Drop MAX_PAGES_PER_VCPU.
---
 xen/arch/arm/domain.c             | 17 -----------------
 xen/arch/ppc/stubs.c              | 10 ----------
 xen/arch/riscv/stubs.c            | 10 ----------
 xen/arch/x86/domain.c             | 17 ++---------------
 xen/arch/x86/include/asm/domain.h |  3 +++
 xen/common/domain.c               | 20 ++++++++++++++++++++
 xen/include/xen/domain.h          |  4 ----
 7 files changed, 25 insertions(+), 56 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e566023340..507df807ed 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -473,23 +473,6 @@ void dump_pageframe_info(struct domain *d)
 
 }
 
-struct vcpu *alloc_vcpu_struct(const struct domain *d)
-{
-    struct vcpu *v;
-
-    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
-    v = alloc_xenheap_pages(0, 0);
-    if ( v )
-        clear_page(v);
-
-    return v;
-}
-
-void free_vcpu_struct(struct vcpu *v)
-{
-    free_xenheap_page(v);
-}
-
 int arch_vcpu_create(struct vcpu *v)
 {
     int rc = 0;
diff --git a/xen/arch/ppc/stubs.c b/xen/arch/ppc/stubs.c
index 9953ea1c6c..f7f6e7ed97 100644
--- a/xen/arch/ppc/stubs.c
+++ b/xen/arch/ppc/stubs.c
@@ -152,11 +152,6 @@ void dump_pageframe_info(struct domain *d)
     BUG_ON("unimplemented");
 }
 
-void free_vcpu_struct(struct vcpu *v)
-{
-    BUG_ON("unimplemented");
-}
-
 int arch_vcpu_create(struct vcpu *v)
 {
     BUG_ON("unimplemented");
@@ -264,11 +259,6 @@ void vcpu_kick(struct vcpu *v)
     BUG_ON("unimplemented");
 }
 
-struct vcpu *alloc_vcpu_struct(const struct domain *d)
-{
-    BUG_ON("unimplemented");
-}
-
 unsigned long
 hypercall_create_continuation(unsigned int op, const char *format, ...)
 {
diff --git a/xen/arch/riscv/stubs.c b/xen/arch/riscv/stubs.c
index fe7d85ee1d..579c4215c8 100644
--- a/xen/arch/riscv/stubs.c
+++ b/xen/arch/riscv/stubs.c
@@ -126,11 +126,6 @@ void dump_pageframe_info(struct domain *d)
     BUG_ON("unimplemented");
 }
 
-void free_vcpu_struct(struct vcpu *v)
-{
-    BUG_ON("unimplemented");
-}
-
 int arch_vcpu_create(struct vcpu *v)
 {
     BUG_ON("unimplemented");
@@ -238,11 +233,6 @@ void vcpu_kick(struct vcpu *v)
     BUG_ON("unimplemented");
 }
 
-struct vcpu *alloc_vcpu_struct(const struct domain *d)
-{
-    BUG_ON("unimplemented");
-}
-
 unsigned long
 hypercall_create_continuation(unsigned int op, const char *format, ...)
 {
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 7632d5e2d6..68c7503eda 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -493,28 +493,15 @@ unsigned int arch_domain_struct_memflags(void)
     return MEMF_bits(bits);
 }
 
-struct vcpu *alloc_vcpu_struct(const struct domain *d)
+unsigned int arch_vcpu_struct_memflags(const struct domain *d)
 {
-    struct vcpu *v;
     /*
      * This structure contains embedded PAE PDPTEs, used when an HVM guest
      * runs on shadow pagetables outside of 64-bit mode. In this case the CPU
      * may require that the shadow CR3 points below 4GB, and hence the whole
      * structure must satisfy this restriction. Thus we specify MEMF_bits(32).
      */
-    unsigned int memflags =
-        (is_hvm_domain(d) && paging_mode_shadow(d)) ? MEMF_bits(32) : 0;
-
-    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
-    v = alloc_xenheap_pages(0, memflags);
-    if ( v != NULL )
-        clear_page(v);
-    return v;
-}
-
-void free_vcpu_struct(struct vcpu *v)
-{
-    free_xenheap_page(v);
+    return (is_hvm_domain(d) && paging_mode_shadow(d)) ? MEMF_bits(32) : 0;
 }
 
 /* Initialise various registers to their architectural INIT/RESET state. */
diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
index 7e5cbd11a4..576f9202b4 100644
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -15,6 +15,9 @@
 unsigned int arch_domain_struct_memflags(void);
 #define arch_domain_struct_memflags arch_domain_struct_memflags
 
+unsigned int arch_vcpu_struct_memflags(const struct domain *d);
+#define arch_vcpu_struct_memflags arch_vcpu_struct_memflags
+
 #define has_32bit_shinfo(d)    ((d)->arch.has_32bit_shinfo)
 
 /*
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 93c71bc766..92fc0684fc 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -392,6 +392,26 @@ static int vcpu_teardown(struct vcpu *v)
     return 0;
 }
 
+static struct vcpu *alloc_vcpu_struct(const struct domain *d)
+{
+#ifndef arch_vcpu_struct_memflags
+# define arch_vcpu_struct_memflags(d) 0
+#endif
+    struct vcpu *v;
+
+    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
+    v = alloc_xenheap_pages(0, arch_vcpu_struct_memflags(d));
+    if ( v )
+        clear_page(v);
+
+    return v;
+}
+
+static void free_vcpu_struct(struct vcpu *v)
+{
+    free_xenheap_page(v);
+}
+
 /*
  * Destoy a vcpu once all references to it have been dropped.  Used either
  * from domain_destroy()'s RCU path, or from the vcpu_create() error path
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 8aab05ae93..644f5ac3f2 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -70,10 +70,6 @@ void domid_free(domid_t domid);
 struct domain *alloc_domain_struct(void);
 void free_domain_struct(struct domain *d);
 
-/* Allocate/free a VCPU structure. */
-struct vcpu *alloc_vcpu_struct(const struct domain *d);
-void free_vcpu_struct(struct vcpu *v);
-
 /* Allocate/free a PIRQ structure. */
 #ifndef alloc_pirq_struct
 struct pirq *alloc_pirq_struct(struct domain *d);
-- 
2.52.0

[PATCH v2 4/4] xen/common: make {alloc,free}_domain_struct() static

[Oleksii Kurochko]

As {alloc,free}_domain_struct() are used only within domain.c,
they can be declared static and their declarations removed
from xen/domain.h.

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in v2:
 - New patch.
---
 xen/common/domain.c      | 6 ++++--
 xen/include/xen/domain.h | 4 ----
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 92fc0684fc..7509dafd6f 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -690,6 +690,8 @@ static int domain_teardown(struct domain *d)
     return 0;
 }
 
+static void free_domain_struct(struct domain *d);
+
 /*
  * Destroy a domain once all references to it have been dropped.  Used either
  * from the RCU path, or from the domain_create() error path before the domain
@@ -819,7 +821,7 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
     return arch_sanitise_domain_config(config);
 }
 
-struct domain *alloc_domain_struct(void)
+static struct domain *alloc_domain_struct(void)
 {
 #ifndef arch_domain_struct_memflags
 # define arch_domain_struct_memflags() 0
@@ -835,7 +837,7 @@ struct domain *alloc_domain_struct(void)
     return d;
 }
 
-void free_domain_struct(struct domain *d)
+static void free_domain_struct(struct domain *d)
 {
     free_xenheap_page(d);
 }
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 644f5ac3f2..273717c31b 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -66,10 +66,6 @@ void domid_free(domid_t domid);
  * Arch-specifics.
  */
 
-/* Allocate/free a domain structure. */
-struct domain *alloc_domain_struct(void);
-void free_domain_struct(struct domain *d);
-
 /* Allocate/free a PIRQ structure. */
 #ifndef alloc_pirq_struct
 struct pirq *alloc_pirq_struct(struct domain *d);
-- 
2.52.0

Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Andrew Cooper]

On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 3ebdf9953f..8b17871b86 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -370,29 +370,35 @@ int vcpu_vgic_init(struct vcpu *v)
>  {
>      int i;
>  
> -    v->arch.vgic.private_irqs = xzalloc(struct vgic_irq_rank);
> -    if ( v->arch.vgic.private_irqs == NULL )
> +    v->arch.vgic = xzalloc(struct vgic_cpu);
> +    if ( v->arch.vgic == NULL )
> +        return -ENOMEM;
> +
> +    v->arch.vgic->private_irqs = xzalloc(struct vgic_irq_rank);
> +    if ( v->arch.vgic->private_irqs == NULL )
>        return -ENOMEM;

This error path needs to free v->arch.vgic.  (If we continue down this
route.  See below.)

>  
>      /* SGIs/PPIs are always routed to this VCPU */
> -    vgic_rank_init(v->arch.vgic.private_irqs, 0, v->vcpu_id);
> +    vgic_rank_init(v->arch.vgic->private_irqs, 0, v->vcpu_id);
>  
>      v->domain->arch.vgic.handler->vcpu_init(v);
>  
> -    memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
> +    memset(&v->arch.vgic->pending_irqs, 0, sizeof(v->arch.vgic->pending_irqs));
>      for (i = 0; i < 32; i++)
> -        vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i);
> +        vgic_init_pending_irq(&v->arch.vgic->pending_irqs[i], i);
>  
> -    INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
> -    INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
> -    spin_lock_init(&v->arch.vgic.lock);
> +    INIT_LIST_HEAD(&v->arch.vgic->inflight_irqs);
> +    INIT_LIST_HEAD(&v->arch.vgic->lr_pending);
> +    spin_lock_init(&v->arch.vgic->lock);
>  
>      return 0;
>  }
>  
>  int vcpu_vgic_free(struct vcpu *v)
>  {
> -    xfree(v->arch.vgic.private_irqs);
> +    xfree(v->arch.vgic->private_irqs);
> +    xfree(v->arch.vgic);
> +
>      return 0;
>  }

Free functions should be idempotent.  This was buggy before, even moreso
now.  It wants to be:

void vcpu_vgic_free(struct vcpu *v)
{
    if ( v->arch.vgic )
    {
        XFREE(v->arch.vgic->private_irqs);
        XFREE(v->arch.vgic);
    }
}

Given the type change, this probably wants splitting out into an earlier
patch.

Given the fact that the single caller doesn't even check the return
value, you're fixing a MISRA violation by making it void.

> diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
> index f8d7d3a226..67f297797f 100644
> --- a/xen/arch/arm/vgic/vgic-init.c
> +++ b/xen/arch/arm/vgic/vgic-init.c
> @@ -241,10 +245,12 @@ void domain_vgic_free(struct domain *d)
>  
>  int vcpu_vgic_free(struct vcpu *v)
>  {
> -    struct vgic_cpu *vgic_cpu = &v->arch.vgic;
> +    struct vgic_cpu *vgic_cpu = v->arch.vgic;
>  
>      INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
>  
> +    xfree(vgic_cpu);
> +
>      return 0;
>  }

Not in your part of the change, but this is bogus.  It's not remotely
safe to init the list head like that.

The list is either already empty, in which case it's a no-op, or it
corrupts the list.  It appears that the list mixes entries from other
vCPUs, and from the domain.

I think this is further proof that NEW_VGIC should be deleted
wholesale.  It's clearly not in a good state, and I get the impression
that a badly timed evtchn sent to a domain in the middle of being
cleaned up will cause Xen to trip over the corrupted list.

>  
> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> index 6a558089c5..e64d681dd2 100644
> --- a/xen/arch/arm/vgic/vgic-v2.c
> +++ b/xen/arch/arm/vgic/vgic-v2.c
> @@ -56,8 +56,8 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
>   */
>  void vgic_v2_fold_lr_state(struct vcpu *vcpu)
>  {
> -    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
> -    unsigned int used_lrs = vcpu->arch.vgic.used_lrs;
> +    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
> +    unsigned int used_lrs = vcpu->arch.vgic->used_lrs;

vgic_cpu->used_lrs.

Taking a step back, I think the patch could be much smaller if you only
made private_irqs in NEW_VGIC be a separate pointer, matching the "old"
VGIC code.  Or does that not save enough space in struct vCPU?

~Andrew

Re: [PATCH v2 2/4] xen/arm: drop MAX_PAGES_PER_VCPU

[Andrew Cooper]

On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 47973f99d9..e566023340 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -473,36 +473,21 @@ void dump_pageframe_info(struct domain *d)
>  
>  }
>  
> -/*
> - * The new VGIC has a bigger per-IRQ structure, so we need more than one
> - * page on ARM64. Cowardly increase the limit in this case.
> - */
> -#if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
> -#define MAX_PAGES_PER_VCPU  2
> -#else
> -#define MAX_PAGES_PER_VCPU  1
> -#endif
> -
>  struct vcpu *alloc_vcpu_struct(const struct domain *d)
>  {
>      struct vcpu *v;
>  
> -    BUILD_BUG_ON(sizeof(*v) > MAX_PAGES_PER_VCPU * PAGE_SIZE);
> -    v = alloc_xenheap_pages(get_order_from_bytes(sizeof(*v)), 0);
> -    if ( v != NULL )
> -    {
> -        unsigned int i;
> -
> -        for ( i = 0; i < DIV_ROUND_UP(sizeof(*v), PAGE_SIZE); i++ )
> -            clear_page((void *)v + i * PAGE_SIZE);
> -    }
> +    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
> +    v = alloc_xenheap_pages(0, 0);

I know this is only interim until the next patch, but
alloc_xenheap_page() to match the free function used.

Personally, I'd merge patches 2 and 3 together, because everything you
touch in this patch is deleted by the next one.

But, whatever the ARM maintainers prefer.

~Andrew

Re: [PATCH v2 4/4] xen/common: make {alloc,free}_domain_struct() static

[Andrew Cooper]

On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
> As {alloc,free}_domain_struct() are used only within domain.c,
> they can be declared static and their declarations removed
> from xen/domain.h.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> Changes in v2:
>  - New patch.
> ---
>  xen/common/domain.c      | 6 ++++--
>  xen/include/xen/domain.h | 4 ----
>  2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 92fc0684fc..7509dafd6f 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -690,6 +690,8 @@ static int domain_teardown(struct domain *d)
>      return 0;
>  }
>  
> +static void free_domain_struct(struct domain *d);
> +

Another option would be to move them both up here, and avoid the forward
declaration.  It's a bigger patch, but results in domain.c being
slightly less complex.

~Andrew

Re: [PATCH v2 3/4] xen: move alloc/free_vcpu_struct() to common code

[Andrew Cooper]

On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
> alloc_vcpu_struct() and free_vcpu_struct() contain little
> architecture-specific logic and are suitable for sharing across
> architectures. Move both helpers to common code.
>
> To support the remaining architectural differences, introduce
> arch_vcpu_struct_memflags(), allowing architectures to override the
> memory flags passed to alloc_xenheap_pages(). This is currently needed
> by x86, which may require MEMF_bits(32) for HVM guests using shadow
> paging.
>
> The ARM implementation of alloc/free_vcpu_struct() is removed and
> replaced by the common version. Stub implementations are also dropped
> from PPC and RISC-V.
>
> Finally, make alloc_vcpu_struct() and free_vcpu_struct() static to
> common/domain.c, as they are no longer used outside common code.
>
> No functional changes.
>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in v2:
>  - Rework alloc/free_vcpu_struct() to work with only one page.
>  - Return back the comment about the restriction inside x86's
>    arch_vcpu_struct_memflags().
>  - Drop MAX_PAGES_PER_VCPU.
> ---
>  xen/arch/arm/domain.c             | 17 -----------------
>  xen/arch/ppc/stubs.c              | 10 ----------
>  xen/arch/riscv/stubs.c            | 10 ----------
>  xen/arch/x86/domain.c             | 17 ++---------------
>  xen/arch/x86/include/asm/domain.h |  3 +++
>  xen/common/domain.c               | 20 ++++++++++++++++++++
>  xen/include/xen/domain.h          |  4 ----
>  7 files changed, 25 insertions(+), 56 deletions(-)

Much nicer.

> diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> index 7632d5e2d6..68c7503eda 100644
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -493,28 +493,15 @@ unsigned int arch_domain_struct_memflags(void)
>      return MEMF_bits(bits);
>  }
>  
> -struct vcpu *alloc_vcpu_struct(const struct domain *d)
> +unsigned int arch_vcpu_struct_memflags(const struct domain *d)
>  {
> -    struct vcpu *v;
>      /*
>       * This structure contains embedded PAE PDPTEs, used when an HVM guest
>       * runs on shadow pagetables outside of 64-bit mode. In this case the CPU
>       * may require that the shadow CR3 points below 4GB, and hence the whole
>       * structure must satisfy this restriction. Thus we specify MEMF_bits(32).
>       */
> -    unsigned int memflags =
> -        (is_hvm_domain(d) && paging_mode_shadow(d)) ? MEMF_bits(32) : 0;
> -
> -    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
> -    v = alloc_xenheap_pages(0, memflags);
> -    if ( v != NULL )
> -        clear_page(v);
> -    return v;
> -}
> -
> -void free_vcpu_struct(struct vcpu *v)
> -{
> -    free_xenheap_page(v);
> +    return (is_hvm_domain(d) && paging_mode_shadow(d)) ? MEMF_bits(32) : 0;
>  }
>  
>  /* Initialise various registers to their architectural INIT/RESET state. */
> diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h
> index 7e5cbd11a4..576f9202b4 100644
> --- a/xen/arch/x86/include/asm/domain.h
> +++ b/xen/arch/x86/include/asm/domain.h
> @@ -15,6 +15,9 @@
>  unsigned int arch_domain_struct_memflags(void);
>  #define arch_domain_struct_memflags arch_domain_struct_memflags
>  
> +unsigned int arch_vcpu_struct_memflags(const struct domain *d);
> +#define arch_vcpu_struct_memflags arch_vcpu_struct_memflags
> +

Given the single caller and simplicity, this would be better as a static
inline, except it can't yet for header reasons.  So, instead, I'd
suggest simply:

/*
 * $COMMENT
 */
#define arch_vcpu_struct_memflags(d) \
(is_hvm_domain(d) && paging_mode_shadow(d) ? MEMF_bits(32) : 0)

This form does double expand 'd'.  The more complex form would be to
match
https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=c37bcb35f928c81cbeea7c05f86b6779f8a2b8c4
with a local variable.

> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index 93c71bc766..92fc0684fc 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -392,6 +392,26 @@ static int vcpu_teardown(struct vcpu *v)
>      return 0;
>  }
>  
> +static struct vcpu *alloc_vcpu_struct(const struct domain *d)
> +{
> +#ifndef arch_vcpu_struct_memflags
> +# define arch_vcpu_struct_memflags(d) 0

((void)d, 0)

~Andrew

Re: [PATCH v2 3/4] xen: move alloc/free_vcpu_struct() to common code

[Jan Beulich]

On 18.12.2025 19:34, Andrew Cooper wrote:
> On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -392,6 +392,26 @@ static int vcpu_teardown(struct vcpu *v)
>>      return 0;
>>  }
>>  
>> +static struct vcpu *alloc_vcpu_struct(const struct domain *d)
>> +{
>> +#ifndef arch_vcpu_struct_memflags
>> +# define arch_vcpu_struct_memflags(d) 0
> 
> ((void)d, 0)

Nit: ((void)(d), 0)

Jan

Re: [PATCH v2 2/4] xen/arm: drop MAX_PAGES_PER_VCPU

[Orzel, Michal]

On 18/12/2025 19:19, Andrew Cooper wrote:
> On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 47973f99d9..e566023340 100644
>> --- a/xen/arch/arm/domain.c
>> +++ b/xen/arch/arm/domain.c
>> @@ -473,36 +473,21 @@ void dump_pageframe_info(struct domain *d)
>>  
>>  }
>>  
>> -/*
>> - * The new VGIC has a bigger per-IRQ structure, so we need more than one
>> - * page on ARM64. Cowardly increase the limit in this case.
>> - */
>> -#if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
>> -#define MAX_PAGES_PER_VCPU  2
>> -#else
>> -#define MAX_PAGES_PER_VCPU  1
>> -#endif
>> -
>>  struct vcpu *alloc_vcpu_struct(const struct domain *d)
>>  {
>>      struct vcpu *v;
>>  
>> -    BUILD_BUG_ON(sizeof(*v) > MAX_PAGES_PER_VCPU * PAGE_SIZE);
>> -    v = alloc_xenheap_pages(get_order_from_bytes(sizeof(*v)), 0);
>> -    if ( v != NULL )
>> -    {
>> -        unsigned int i;
>> -
>> -        for ( i = 0; i < DIV_ROUND_UP(sizeof(*v), PAGE_SIZE); i++ )
>> -            clear_page((void *)v + i * PAGE_SIZE);
>> -    }
>> +    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
>> +    v = alloc_xenheap_pages(0, 0);
> 
> I know this is only interim until the next patch, but
> alloc_xenheap_page() to match the free function used.
> 
> Personally, I'd merge patches 2 and 3 together, because everything you
> touch in this patch is deleted by the next one.
> 
> But, whatever the ARM maintainers prefer.
I'm in favor of Andrew's suggestion. There's no point in introducing something
in one patch and dropping it in the very next one.

~Michal

Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Oleksii Kurochko]

On 12/18/25 7:15 PM, Andrew Cooper wrote:
> On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 3ebdf9953f..8b17871b86 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -370,29 +370,35 @@ int vcpu_vgic_init(struct vcpu *v)
>>   {
>>       int i;
>>   
>> -    v->arch.vgic.private_irqs = xzalloc(struct vgic_irq_rank);
>> -    if ( v->arch.vgic.private_irqs == NULL )
>> +    v->arch.vgic = xzalloc(struct vgic_cpu);
>> +    if ( v->arch.vgic == NULL )
>> +        return -ENOMEM;
>> +
>> +    v->arch.vgic->private_irqs = xzalloc(struct vgic_irq_rank);
>> +    if ( v->arch.vgic->private_irqs == NULL )
>>         return -ENOMEM;
> This error path needs to free v->arch.vgic.  (If we continue down this
> route.  See below.)
>
>>   
>>       /* SGIs/PPIs are always routed to this VCPU */
>> -    vgic_rank_init(v->arch.vgic.private_irqs, 0, v->vcpu_id);
>> +    vgic_rank_init(v->arch.vgic->private_irqs, 0, v->vcpu_id);
>>   
>>       v->domain->arch.vgic.handler->vcpu_init(v);
>>   
>> -    memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
>> +    memset(&v->arch.vgic->pending_irqs, 0, sizeof(v->arch.vgic->pending_irqs));
>>       for (i = 0; i < 32; i++)
>> -        vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i);
>> +        vgic_init_pending_irq(&v->arch.vgic->pending_irqs[i], i);
>>   
>> -    INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
>> -    INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
>> -    spin_lock_init(&v->arch.vgic.lock);
>> +    INIT_LIST_HEAD(&v->arch.vgic->inflight_irqs);
>> +    INIT_LIST_HEAD(&v->arch.vgic->lr_pending);
>> +    spin_lock_init(&v->arch.vgic->lock);
>>   
>>       return 0;
>>   }
>>   
>>   int vcpu_vgic_free(struct vcpu *v)
>>   {
>> -    xfree(v->arch.vgic.private_irqs);
>> +    xfree(v->arch.vgic->private_irqs);
>> +    xfree(v->arch.vgic);
>> +
>>       return 0;
>>   }
> Free functions should be idempotent.  This was buggy before, even moreso
> now.  It wants to be:
>
> void vcpu_vgic_free(struct vcpu *v)
> {
>      if ( v->arch.vgic )
>      {
>          XFREE(v->arch.vgic->private_irqs);
>          XFREE(v->arch.vgic);
>      }
> }
>
> Given the type change, this probably wants splitting out into an earlier
> patch.
>
> Given the fact that the single caller doesn't even check the return
> value, you're fixing a MISRA violation by making it void.

Btw, IIUC, it could be also be something like:
   (void) vcpu_vgic_free(...)
and then we won't break any MISRA rule, right?

I will send suggested updates in the separate patch of this patch series.


>
>> diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
>> index f8d7d3a226..67f297797f 100644
>> --- a/xen/arch/arm/vgic/vgic-init.c
>> +++ b/xen/arch/arm/vgic/vgic-init.c
>> @@ -241,10 +245,12 @@ void domain_vgic_free(struct domain *d)
>>   
>>   int vcpu_vgic_free(struct vcpu *v)
>>   {
>> -    struct vgic_cpu *vgic_cpu = &v->arch.vgic;
>> +    struct vgic_cpu *vgic_cpu = v->arch.vgic;
>>   
>>       INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
>>   
>> +    xfree(vgic_cpu);
>> +
>>       return 0;
>>   }
> Not in your part of the change, but this is bogus.  It's not remotely
> safe to init the list head like that.
>
> The list is either already empty, in which case it's a no-op, or it
> corrupts the list.  It appears that the list mixes entries from other
> vCPUs, and from the domain.

I guess it should be handled by vgic_prune_ap_list() which is called
when we are entering hypervisor (enter_hypervisor_from_guest() ->
vgic_sync_from_lrs() -> vgic_prune_ap_list()).

Also, I would like not that this code is based on KVM which also has
the same INIT_LIST_HEAD():
   https://elixir.bootlin.com/linux/v6.18.1/source/arch/arm64/kvm/vgic/vgic-init.c#L467

I won't touch this code in the next patch series except if one of
Arm's maintainer will tell me so.

>
> I think this is further proof that NEW_VGIC should be deleted
> wholesale.  It's clearly not in a good state, and I get the impression
> that a badly timed evtchn sent to a domain in the middle of being
> cleaned up will cause Xen to trip over the corrupted list.
>
>>   
>> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
>> index 6a558089c5..e64d681dd2 100644
>> --- a/xen/arch/arm/vgic/vgic-v2.c
>> +++ b/xen/arch/arm/vgic/vgic-v2.c
>> @@ -56,8 +56,8 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t csize,
>>    */
>>   void vgic_v2_fold_lr_state(struct vcpu *vcpu)
>>   {
>> -    struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic;
>> -    unsigned int used_lrs = vcpu->arch.vgic.used_lrs;
>> +    struct vgic_cpu *vgic_cpu = vcpu->arch.vgic;
>> +    unsigned int used_lrs = vcpu->arch.vgic->used_lrs;
> vgic_cpu->used_lrs.
>
> Taking a step back, I think the patch could be much smaller if you only
> made private_irqs in NEW_VGIC be a separate pointer, matching the "old"
> VGIC code.  Or does that not save enough space in struct vCPU?

It seems like it would be also fine, the size of struct vcpu will be 2176.
And it also basically just require only:
   struct vgic_cpu {
   -    struct vgic_irq private_irqs[VGIC_NR_PRIVATE_IRQS];
   +    struct vgic_irq *private_irqs;
(of course with allocation and freeing of private_irqs).

Lets stick to this approach then.

Thanks.

~ Oleksii

Re: [PATCH v2 2/4] xen/arm: drop MAX_PAGES_PER_VCPU

[Oleksii Kurochko]

On 12/19/25 9:22 AM, Orzel, Michal wrote:
>
> On 18/12/2025 19:19, Andrew Cooper wrote:
>> On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index 47973f99d9..e566023340 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -473,36 +473,21 @@ void dump_pageframe_info(struct domain *d)
>>>   
>>>   }
>>>   
>>> -/*
>>> - * The new VGIC has a bigger per-IRQ structure, so we need more than one
>>> - * page on ARM64. Cowardly increase the limit in this case.
>>> - */
>>> -#if defined(CONFIG_NEW_VGIC) && defined(CONFIG_ARM_64)
>>> -#define MAX_PAGES_PER_VCPU  2
>>> -#else
>>> -#define MAX_PAGES_PER_VCPU  1
>>> -#endif
>>> -
>>>   struct vcpu *alloc_vcpu_struct(const struct domain *d)
>>>   {
>>>       struct vcpu *v;
>>>   
>>> -    BUILD_BUG_ON(sizeof(*v) > MAX_PAGES_PER_VCPU * PAGE_SIZE);
>>> -    v = alloc_xenheap_pages(get_order_from_bytes(sizeof(*v)), 0);
>>> -    if ( v != NULL )
>>> -    {
>>> -        unsigned int i;
>>> -
>>> -        for ( i = 0; i < DIV_ROUND_UP(sizeof(*v), PAGE_SIZE); i++ )
>>> -            clear_page((void *)v + i * PAGE_SIZE);
>>> -    }
>>> +    BUILD_BUG_ON(sizeof(*v) > PAGE_SIZE);
>>> +    v = alloc_xenheap_pages(0, 0);
>> I know this is only interim until the next patch, but
>> alloc_xenheap_page() to match the free function used.
>>
>> Personally, I'd merge patches 2 and 3 together, because everything you
>> touch in this patch is deleted by the next one.
>>
>> But, whatever the ARM maintainers prefer.
> I'm in favor of Andrew's suggestion. There's no point in introducing something
> in one patch and dropping it in the very next one.

Then I will merge patch 2 and 3.

~ Oleksii

Re: [PATCH v2 4/4] xen/common: make {alloc,free}_domain_struct() static

[Oleksii Kurochko]

On 12/18/25 7:21 PM, Andrew Cooper wrote:
> On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
>> As {alloc,free}_domain_struct() are used only within domain.c,
>> they can be declared static and their declarations removed
>> from xen/domain.h.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

>> ---
>> Changes in v2:
>>   - New patch.
>> ---
>>   xen/common/domain.c      | 6 ++++--
>>   xen/include/xen/domain.h | 4 ----
>>   2 files changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/common/domain.c b/xen/common/domain.c
>> index 92fc0684fc..7509dafd6f 100644
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -690,6 +690,8 @@ static int domain_teardown(struct domain *d)
>>       return 0;
>>   }
>>   
>> +static void free_domain_struct(struct domain *d);
>> +
> Another option would be to move them both up here, and avoid the forward
> declaration.  It's a bigger patch, but results in domain.c being
> slightly less complex.

Then I will move both|free_domain_struct()| and|alloc_domain_struct()| (to keep them
together) to the place where the forward declaration of|free_domain_struct()| is
introduced in this patch.

Does that work for you?

~ Oleksii

Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Oleksii Kurochko]

On 12/18/25 7:15 PM, Andrew Cooper wrote:
>>   int vcpu_vgic_free(struct vcpu *v)
>>   {
>> -    xfree(v->arch.vgic.private_irqs);
>> +    xfree(v->arch.vgic->private_irqs);
>> +    xfree(v->arch.vgic);
>> +
>>       return 0;
>>   }
> Free functions should be idempotent.  This was buggy before, even moreso
> now.

Was it really buggy before in terms of idempotent.

It seems like xfree() can handle the case when v->arch.vgic.private_irqs is NULL.
Should I still have:
  if ( v->arch.vgic->private_irqs )
     XFREE(v->arch.vgic->private_irqs);
?

~ Oleksii

Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Jan Beulich]

On 19.12.2025 13:42, Oleksii Kurochko wrote:
> 
> On 12/18/25 7:15 PM, Andrew Cooper wrote:
>> On 18/12/2025 5:28 pm, Oleksii Kurochko wrote:
>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>> index 3ebdf9953f..8b17871b86 100644
>>> --- a/xen/arch/arm/vgic.c
>>> +++ b/xen/arch/arm/vgic.c
>>> @@ -370,29 +370,35 @@ int vcpu_vgic_init(struct vcpu *v)
>>>   {
>>>       int i;
>>>   
>>> -    v->arch.vgic.private_irqs = xzalloc(struct vgic_irq_rank);
>>> -    if ( v->arch.vgic.private_irqs == NULL )
>>> +    v->arch.vgic = xzalloc(struct vgic_cpu);
>>> +    if ( v->arch.vgic == NULL )
>>> +        return -ENOMEM;
>>> +
>>> +    v->arch.vgic->private_irqs = xzalloc(struct vgic_irq_rank);
>>> +    if ( v->arch.vgic->private_irqs == NULL )
>>>         return -ENOMEM;
>> This error path needs to free v->arch.vgic.  (If we continue down this
>> route.  See below.)
>>
>>>   
>>>       /* SGIs/PPIs are always routed to this VCPU */
>>> -    vgic_rank_init(v->arch.vgic.private_irqs, 0, v->vcpu_id);
>>> +    vgic_rank_init(v->arch.vgic->private_irqs, 0, v->vcpu_id);
>>>   
>>>       v->domain->arch.vgic.handler->vcpu_init(v);
>>>   
>>> -    memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs));
>>> +    memset(&v->arch.vgic->pending_irqs, 0, sizeof(v->arch.vgic->pending_irqs));
>>>       for (i = 0; i < 32; i++)
>>> -        vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i);
>>> +        vgic_init_pending_irq(&v->arch.vgic->pending_irqs[i], i);
>>>   
>>> -    INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs);
>>> -    INIT_LIST_HEAD(&v->arch.vgic.lr_pending);
>>> -    spin_lock_init(&v->arch.vgic.lock);
>>> +    INIT_LIST_HEAD(&v->arch.vgic->inflight_irqs);
>>> +    INIT_LIST_HEAD(&v->arch.vgic->lr_pending);
>>> +    spin_lock_init(&v->arch.vgic->lock);
>>>   
>>>       return 0;
>>>   }
>>>   
>>>   int vcpu_vgic_free(struct vcpu *v)
>>>   {
>>> -    xfree(v->arch.vgic.private_irqs);
>>> +    xfree(v->arch.vgic->private_irqs);
>>> +    xfree(v->arch.vgic);
>>> +
>>>       return 0;
>>>   }
>> Free functions should be idempotent.  This was buggy before, even moreso
>> now.  It wants to be:
>>
>> void vcpu_vgic_free(struct vcpu *v)
>> {
>>      if ( v->arch.vgic )
>>      {
>>          XFREE(v->arch.vgic->private_irqs);
>>          XFREE(v->arch.vgic);
>>      }
>> }
>>
>> Given the type change, this probably wants splitting out into an earlier
>> patch.
>>
>> Given the fact that the single caller doesn't even check the return
>> value, you're fixing a MISRA violation by making it void.
> 
> Btw, IIUC, it could be also be something like:
>    (void) vcpu_vgic_free(...)
> and then we won't break any MISRA rule, right?

That would address one Misra concern, but there would then still be dead code.

Jan

Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Jan Beulich]

On 19.12.2025 14:32, Oleksii Kurochko wrote:
> 
> On 12/18/25 7:15 PM, Andrew Cooper wrote:
>>>   int vcpu_vgic_free(struct vcpu *v)
>>>   {
>>> -    xfree(v->arch.vgic.private_irqs);
>>> +    xfree(v->arch.vgic->private_irqs);
>>> +    xfree(v->arch.vgic);
>>> +
>>>       return 0;
>>>   }
>> Free functions should be idempotent.  This was buggy before, even moreso
>> now.
> 
> Was it really buggy before in terms of idempotent.
> 
> It seems like xfree() can handle the case when v->arch.vgic.private_irqs is NULL.
> Should I still have:
>   if ( v->arch.vgic->private_irqs )
>      XFREE(v->arch.vgic->private_irqs);
> ?

No, and iirc Andrew also didn't ask for this.

Jan

Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Julien Grall]

Hi Andrew,

On 18/12/2025 18:15, Andrew Cooper wrote:
>> diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
>> index f8d7d3a226..67f297797f 100644
>> --- a/xen/arch/arm/vgic/vgic-init.c
>> +++ b/xen/arch/arm/vgic/vgic-init.c
>> @@ -241,10 +245,12 @@ void domain_vgic_free(struct domain *d)
>>   
>>   int vcpu_vgic_free(struct vcpu *v)
>>   {
>> -    struct vgic_cpu *vgic_cpu = &v->arch.vgic;
>> +    struct vgic_cpu *vgic_cpu = v->arch.vgic;
>>   
>>       INIT_LIST_HEAD(&vgic_cpu->ap_list_head);
>>   
>> +    xfree(vgic_cpu);
>> +
>>       return 0;
>>   }
> 
> Not in your part of the change, but this is bogus.  It's not remotely
> safe to init the list head like that.
> 
> The list is either already empty, in which case it's a no-op, or it
> corrupts the list.  It appears that the list mixes entries from other
> vCPUs, and from the domain.
> 
> I think this is further proof that NEW_VGIC should be deleted
> wholesale.  It's clearly not in a good state, and I get the impression
> that a badly timed evtchn sent to a domain in the middle of being
> cleaned up will cause Xen to trip over the corrupted list.

I am probably missing something. What other issues are in the NEW_VGIC?

But note, if we delete the NEW_VGIC then we are back to a situation 
where the vGIC implementation we have is not even remotely spec 
compliant (no level support, not pending/active clear/set support) and 
from previous discussion it was not possible to rework the default vGIC 
implementation to make it compliant.

I was going to say that the NEW_VGIC is meant to be experimental. But I 
see, it can selected without EXPERT :(. Thankfully it is marked as not 
security supported at least in the Kconfig doc.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Oleksii Kurochko]

On 12/19/25 3:16 PM, Jan Beulich wrote:
> On 19.12.2025 14:32, Oleksii Kurochko wrote:
>> On 12/18/25 7:15 PM, Andrew Cooper wrote:
>>>>    int vcpu_vgic_free(struct vcpu *v)
>>>>    {
>>>> -    xfree(v->arch.vgic.private_irqs);
>>>> +    xfree(v->arch.vgic->private_irqs);
>>>> +    xfree(v->arch.vgic);
>>>> +
>>>>        return 0;
>>>>    }
>>> Free functions should be idempotent.  This was buggy before, even moreso
>>> now.
>> Was it really buggy before in terms of idempotent.
>>
>> It seems like xfree() can handle the case when v->arch.vgic.private_irqs is NULL.
>> Should I still have:
>>    if ( v->arch.vgic->private_irqs )
>>       XFREE(v->arch.vgic->private_irqs);
>> ?
> No, and iirc Andrew also didn't ask for this.

Andrew wrote:
```
Free functions should be idempotent.  This was buggy before, even moreso
now.  It wants to be:

void vcpu_vgic_free(struct vcpu *v)
{
      if ( v->arch.vgic )
      {
          XFREE(v->arch.vgic->private_irqs);
          XFREE(v->arch.vgic);
      }
}

Given the type change, this probably wants splitting out into an earlier
patch.

Given the fact that the single caller doesn't even check the return
value, you're fixing a MISRA violation by making it void.
```

It isn't clear what "This was buggy before" means. In my changes
it will be an issue if vcpu_vgic_free() would be called twice because
without "if ( v->arch.vgic )" we will have NULL pointer dereference
XFREE(v->arch.vgic->private_irqs) here. But considering that before
it was just:
   int vcpu_vgic_free(struct vcpu *v)
   {
	xfree(v->arch.vgic.private_irqs);
	return 0;
   }
In that case, it seems that calling|vcpu_vgic_free()| twice would not cause
anything serious to happen. Am I misunderstanding what was buggy in the original
code, or probably I'm misunderstanding what is idempotent?

~ Oleksii

Re: [PATCH v2 1/4] xen/arm: optimize the size of struct vcpu

[Oleksii Kurochko]

On 12/19/25 6:22 PM, Oleksii Kurochko wrote:
>
> On 12/19/25 3:16 PM, Jan Beulich wrote:
>> On 19.12.2025 14:32, Oleksii Kurochko wrote:
>>> On 12/18/25 7:15 PM, Andrew Cooper wrote:
>>>>>    int vcpu_vgic_free(struct vcpu *v)
>>>>>    {
>>>>> -    xfree(v->arch.vgic.private_irqs);
>>>>> +    xfree(v->arch.vgic->private_irqs);
>>>>> +    xfree(v->arch.vgic);
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>> Free functions should be idempotent.  This was buggy before, even 
>>>> moreso
>>>> now.
>>> Was it really buggy before in terms of idempotent.
>>>
>>> It seems like xfree() can handle the case when 
>>> v->arch.vgic.private_irqs is NULL.
>>> Should I still have:
>>>    if ( v->arch.vgic->private_irqs )
>>>       XFREE(v->arch.vgic->private_irqs);
>>> ?
>> No, and iirc Andrew also didn't ask for this.
>
> Andrew wrote:
> ```
> Free functions should be idempotent.  This was buggy before, even moreso
> now.  It wants to be:
>
> void vcpu_vgic_free(struct vcpu *v)
> {
>      if ( v->arch.vgic )
>      {
>          XFREE(v->arch.vgic->private_irqs);
>          XFREE(v->arch.vgic);
>      }
> }
>
> Given the type change, this probably wants splitting out into an earlier
> patch.
>
> Given the fact that the single caller doesn't even check the return
> value, you're fixing a MISRA violation by making it void.
> ```
>
> It isn't clear what "This was buggy before" means. In my changes
> it will be an issue if vcpu_vgic_free() would be called twice because
> without "if ( v->arch.vgic )" we will have NULL pointer dereference
> XFREE(v->arch.vgic->private_irqs) here. But considering that before
> it was just:
>   int vcpu_vgic_free(struct vcpu *v)
>   {
>     xfree(v->arch.vgic.private_irqs);
>     return 0;
>   }
> In that case, it seems that calling|vcpu_vgic_free()| twice would not 
> cause
> anything serious to happen. Am I misunderstanding what was buggy in 
> the original
> code, or probably I'm misunderstanding what is idempotent?

Oh, it is about that xfree() doesn't set private_irqs pointer to NULL. I thought it is
doing that inside xfree().

~ Oleksii