[PATCH v2 0/4] P2M improvements for Arm

[PATCH v2 0/4] P2M improvements for Arm

[Henry Wang]

There are some clean-up/improvement work that can be done in the
Arm P2M code triggered by [1] and [2]. These were found at the 4.17
code freeze period so the issues were not fixed at that time.
Therefore do the follow-ups here.

Patch#1 addresses one comment in [1]. It was sent earlier and reviewed
once. Pick the updated version, i.e. "[PATCH v2] xen/arm: Reduce
redundant clear root pages when teardown p2m", to this series.

Patch#2 is a new patch based on v1 comments, this is a pre-requisite
patch for patch#3 where the deferring of GICv2 CPU interface mapping
should also be applied for new vGIC.

Patch#3 and #4 addresses the comment in [2] following the discussion
between two possible options.

[1] https://lore.kernel.org/xen-devel/a947e0b4-8f76-cea6-893f-abf30ff95e0d@xen.org/
[2] https://lore.kernel.org/xen-devel/e6643bfc-5bdf-f685-1b68-b28d341071c1@xen.org/

v1 -> v2:
1. Move in-code comment for p2m_force_tlb_flush_sync() on top of
   p2m_clear_root_pages().
2. Add a new patch as patch #2.
3. Correct style in in-code comment in patch #3.
4. Avoid open-coding gfn_eq() and gaddr_to_gfn(d->arch.vgic.cbase).
5. Apply same changes for the new vGICv2 implementation, update the
   commit message accordingly.
6. Add in-code comment in old GICv2's vgic_v2_domain_init() and
   new GICv2's vgic_v2_map_resources() to mention the mapping of the
   virtual CPU interface is deferred until first access.
7. Add reviewed-by and acked-by tags accordingly.

Henry Wang (4):
  xen/arm: Reduce redundant clear root pages when teardown p2m
  xen/arm: Rename vgic_cpu_base and vgic_dist_base for new vGIC
  xen/arm: Defer GICv2 CPU interface mapping until the first access
  xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

 xen/arch/arm/domain.c               |  8 ++++
 xen/arch/arm/include/asm/new_vgic.h | 10 +++--
 xen/arch/arm/include/asm/p2m.h      |  5 +--
 xen/arch/arm/include/asm/vgic.h     |  2 +
 xen/arch/arm/p2m.c                  | 60 ++++++++++-------------------
 xen/arch/arm/traps.c                | 18 +++++++--
 xen/arch/arm/vgic-v2.c              | 25 ++++--------
 xen/arch/arm/vgic/vgic-init.c       |  4 +-
 xen/arch/arm/vgic/vgic-v2.c         | 41 +++++++-------------
 9 files changed, 76 insertions(+), 97 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] xen/arm: Reduce redundant clear root pages when teardown p2m

[Henry Wang]

Currently, p2m for a domain will be teardown from two paths:
(1) The normal path when a domain is destroyed.
(2) The arch_domain_destroy() in the failure path of domain creation.

When tearing down p2m from (1), the part to clear and clean the root
is only needed to do once rather than for every call of p2m_teardown().
If the p2m teardown is from (2), the clear and clean of the root
is unnecessary because the domain is not scheduled.

Therefore, this patch introduces a helper `p2m_clear_root_pages()` to
do the clear and clean of the root, and move this logic outside of
p2m_teardown(). With this movement, the `page_list_empty(&p2m->pages)`
check can be dropped.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Acked-by: Julien Grall <jgrall@amazon.com>
---
v1 -> v2:
1. Move in-code comment for p2m_force_tlb_flush_sync() on top of
   p2m_clear_root_pages().
2. Add Reviewed-by tag from Michal and Acked-by tag from Julien.
---
 xen/arch/arm/domain.c          |  8 +++++++
 xen/arch/arm/include/asm/p2m.h |  1 +
 xen/arch/arm/p2m.c             | 40 +++++++++++++++++-----------------
 3 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 99577adb6c..b8a4a60570 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -959,6 +959,7 @@ enum {
     PROG_xen,
     PROG_page,
     PROG_mapping,
+    PROG_p2m_root,
     PROG_p2m,
     PROG_p2m_pool,
     PROG_done,
@@ -1021,6 +1022,13 @@ int domain_relinquish_resources(struct domain *d)
         if ( ret )
             return ret;
 
+    PROGRESS(p2m_root):
+        /*
+         * We are about to free the intermediate page-tables, so clear the
+         * root to prevent any walk to use them.
+         */
+        p2m_clear_root_pages(&d->arch.p2m);
+
     PROGRESS(p2m):
         ret = p2m_teardown(d, true);
         if ( ret )
diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index 91df922e1c..bf5183e53a 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -281,6 +281,7 @@ int p2m_set_entry(struct p2m_domain *p2m,
 
 bool p2m_resolve_translation_fault(struct domain *d, gfn_t gfn);
 
+void p2m_clear_root_pages(struct p2m_domain *p2m);
 void p2m_invalidate_root(struct p2m_domain *p2m);
 
 /*
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 948f199d84..f1ccd7efbd 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1314,6 +1314,26 @@ static void p2m_invalidate_table(struct p2m_domain *p2m, mfn_t mfn)
     p2m->need_flush = true;
 }
 
+/*
+ * The domain will not be scheduled anymore, so in theory we should
+ * not need to flush the TLBs. Do it for safety purpose.
+ * Note that all the devices have already been de-assigned. So we don't
+ * need to flush the IOMMU TLB here.
+ */
+void p2m_clear_root_pages(struct p2m_domain *p2m)
+{
+    unsigned int i;
+
+    p2m_write_lock(p2m);
+
+    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
+        clear_and_clean_page(p2m->root + i);
+
+    p2m_force_tlb_flush_sync(p2m);
+
+    p2m_write_unlock(p2m);
+}
+
 /*
  * Invalidate all entries in the root page-tables. This is
  * useful to get fault on entry and do an action.
@@ -1698,30 +1718,10 @@ int p2m_teardown(struct domain *d, bool allow_preemption)
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
     unsigned long count = 0;
     struct page_info *pg;
-    unsigned int i;
     int rc = 0;
 
-    if ( page_list_empty(&p2m->pages) )
-        return 0;
-
     p2m_write_lock(p2m);
 
-    /*
-     * We are about to free the intermediate page-tables, so clear the
-     * root to prevent any walk to use them.
-     */
-    for ( i = 0; i < P2M_ROOT_PAGES; i++ )
-        clear_and_clean_page(p2m->root + i);
-
-    /*
-     * The domain will not be scheduled anymore, so in theory we should
-     * not need to flush the TLBs. Do it for safety purpose.
-     *
-     * Note that all the devices have already been de-assigned. So we don't
-     * need to flush the IOMMU TLB here.
-     */
-    p2m_force_tlb_flush_sync(p2m);
-
     while ( (pg = page_list_remove_head(&p2m->pages)) )
     {
         p2m_free_page(p2m->domain, pg);
-- 
2.25.1

[PATCH v2 2/4] xen/arm: Rename vgic_cpu_base and vgic_dist_base for new vGIC

[Henry Wang]

In the follow-up patch from this series, the GICv2 CPU interface
mapping will be deferred until the first access in the stage-2
data abort trap handling code. Since the data abort trap handling
code is common for the current and the new vGIC implementation,
it is necessary to unify the variable names in struct vgic_dist
for these two implementations.

Therefore, this commit renames the vgic_cpu_base and vgic_dist_base
for new vGIC to cbase and dbase. So we can use the same code in
the data abort trap handling code for both vGIC implementations.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v1 -> v2:
1. New patch.
---
 xen/arch/arm/include/asm/new_vgic.h |  8 ++++----
 xen/arch/arm/vgic/vgic-init.c       |  4 ++--
 xen/arch/arm/vgic/vgic-v2.c         | 17 ++++++++---------
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/include/asm/new_vgic.h b/xen/arch/arm/include/asm/new_vgic.h
index b7fa9ab11a..18ed3f754a 100644
--- a/xen/arch/arm/include/asm/new_vgic.h
+++ b/xen/arch/arm/include/asm/new_vgic.h
@@ -115,11 +115,11 @@ struct vgic_dist {
     unsigned int        nr_spis;
 
     /* base addresses in guest physical address space: */
-    paddr_t             vgic_dist_base;     /* distributor */
+    paddr_t             dbase;     /* distributor */
     union
     {
         /* either a GICv2 CPU interface */
-        paddr_t         vgic_cpu_base;
+        paddr_t         cbase;
         /* or a number of GICv3 redistributor regions */
         struct
         {
@@ -188,12 +188,12 @@ struct vgic_cpu {
 
 static inline paddr_t vgic_cpu_base(const struct vgic_dist *vgic)
 {
-    return vgic->vgic_cpu_base;
+    return vgic->cbase;
 }
 
 static inline paddr_t vgic_dist_base(const struct vgic_dist *vgic)
 {
-    return vgic->vgic_dist_base;
+    return vgic->dbase;
 }
 
 #endif /* __ASM_ARM_NEW_VGIC_H */
diff --git a/xen/arch/arm/vgic/vgic-init.c b/xen/arch/arm/vgic/vgic-init.c
index 62ae553699..ea739d081e 100644
--- a/xen/arch/arm/vgic/vgic-init.c
+++ b/xen/arch/arm/vgic/vgic-init.c
@@ -112,8 +112,8 @@ int domain_vgic_register(struct domain *d, int *mmio_count)
         BUG();
     }
 
-    d->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
-    d->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
+    d->arch.vgic.dbase = VGIC_ADDR_UNDEF;
+    d->arch.vgic.cbase = VGIC_ADDR_UNDEF;
     d->arch.vgic.vgic_redist_base = VGIC_ADDR_UNDEF;
 
     return 0;
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index 1a99d3a8b4..07c8f8a005 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -272,7 +272,7 @@ int vgic_v2_map_resources(struct domain *d)
      */
     if ( is_hardware_domain(d) )
     {
-        d->arch.vgic.vgic_dist_base = gic_v2_hw_data.dbase;
+        d->arch.vgic.dbase = gic_v2_hw_data.dbase;
         /*
          * For the hardware domain, we always map the whole HW CPU
          * interface region in order to match the device tree (the "reg"
@@ -280,13 +280,13 @@ int vgic_v2_map_resources(struct domain *d)
          * Note that we assume the size of the CPU interface is always
          * aligned to PAGE_SIZE.
          */
-        d->arch.vgic.vgic_cpu_base = gic_v2_hw_data.cbase;
+        d->arch.vgic.cbase = gic_v2_hw_data.cbase;
         csize = gic_v2_hw_data.csize;
         vbase = gic_v2_hw_data.vbase;
     }
     else if ( is_domain_direct_mapped(d) )
     {
-        d->arch.vgic.vgic_dist_base = gic_v2_hw_data.dbase;
+        d->arch.vgic.dbase = gic_v2_hw_data.dbase;
         /*
          * For all the direct-mapped domain other than the hardware domain,
          * we only map a 8kB CPU interface but we make sure it is at a location
@@ -296,13 +296,13 @@ int vgic_v2_map_resources(struct domain *d)
          * address when the GIC is aliased to get a 8kB contiguous
          * region.
          */
-        d->arch.vgic.vgic_cpu_base = gic_v2_hw_data.cbase;
+        d->arch.vgic.cbase = gic_v2_hw_data.cbase;
         csize = GUEST_GICC_SIZE;
         vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
     }
     else
     {
-        d->arch.vgic.vgic_dist_base = GUEST_GICD_BASE;
+        d->arch.vgic.dbase = GUEST_GICD_BASE;
         /*
          * The CPU interface exposed to the guest is always 8kB. We may
          * need to add an offset to the virtual CPU interface base
@@ -310,14 +310,13 @@ int vgic_v2_map_resources(struct domain *d)
          * region.
          */
         BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
-        d->arch.vgic.vgic_cpu_base = GUEST_GICC_BASE;
+        d->arch.vgic.cbase = GUEST_GICC_BASE;
         csize = GUEST_GICC_SIZE;
         vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
     }
 
 
-    ret = vgic_register_dist_iodev(d, gaddr_to_gfn(dist->vgic_dist_base),
-                                   VGIC_V2);
+    ret = vgic_register_dist_iodev(d, gaddr_to_gfn(dist->dbase), VGIC_V2);
     if ( ret )
     {
         gdprintk(XENLOG_ERR, "Unable to register VGIC MMIO regions\n");
@@ -328,7 +327,7 @@ int vgic_v2_map_resources(struct domain *d)
      * Map the gic virtual cpu interface in the gic cpu interface
      * region of the guest.
      */
-    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.vgic_cpu_base),
+    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
                            csize / PAGE_SIZE, maddr_to_mfn(vbase));
     if ( ret )
     {
-- 
2.25.1

[PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until the first access

[Henry Wang]

Currently, the mapping of the GICv2 CPU interface is created in
arch_domain_create(). This causes some troubles in populating and
freeing of the domain P2M pages pool. For example, a default 16
P2M pages are required in p2m_init() to cope with the P2M mapping
of 8KB GICv2 CPU interface area, and these 16 P2M pages would cause
the complexity of P2M destroy in the failure path of
arch_domain_create().

As per discussion in [1], similarly as the MMIO access for ACPI, this
patch defers the GICv2 CPU interface mapping until the first MMIO
access. This is achieved by moving the GICv2 CPU interface mapping
code from vgic_v2_domain_init()/vgic_v2_map_resources() to the
stage-2 data abort trap handling code. The original CPU interface
size and virtual CPU interface base address is now saved in
`struct vgic_dist` instead of the local variable of
vgic_v2_domain_init()/vgic_v2_map_resources().

Take the opportunity to unify the way of data access using the
existing pointer to struct vgic_dist in vgic_v2_map_resources() for
new GICv2.

Since the hardware domain (Dom0) has an unlimited size P2M pool, the
gicv2_map_hwdom_extra_mappings() is also not touched by this patch.

[1] https://lore.kernel.org/xen-devel/e6643bfc-5bdf-f685-1b68-b28d341071c1@xen.org/

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
---
v1 -> v2:
1. Correct style in in-code comment.
2. Avoid open-coding gfn_eq() and gaddr_to_gfn(d->arch.vgic.cbase).
3. Apply same changes for the new vGICv2 implementation, update the
   commit message accordingly.
4. Add in-code comment in old GICv2's vgic_v2_domain_init() and
   new GICv2's vgic_v2_map_resources() to mention the mapping of the
   virtual CPU interface is deferred until first access.
---
 xen/arch/arm/include/asm/new_vgic.h |  2 ++
 xen/arch/arm/include/asm/vgic.h     |  2 ++
 xen/arch/arm/traps.c                | 18 +++++++++++---
 xen/arch/arm/vgic-v2.c              | 25 ++++++-------------
 xen/arch/arm/vgic/vgic-v2.c         | 38 ++++++++++-------------------
 5 files changed, 39 insertions(+), 46 deletions(-)

diff --git a/xen/arch/arm/include/asm/new_vgic.h b/xen/arch/arm/include/asm/new_vgic.h
index 18ed3f754a..1e76213893 100644
--- a/xen/arch/arm/include/asm/new_vgic.h
+++ b/xen/arch/arm/include/asm/new_vgic.h
@@ -127,6 +127,8 @@ struct vgic_dist {
             paddr_t     vgic_redist_free_offset;
         };
     };
+    paddr_t             csize; /* CPU interface size */
+    paddr_t             vbase; /* virtual CPU interface base address */
 
     /* distributor enabled */
     bool                enabled;
diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index 3d44868039..328fd46d1b 100644
--- a/xen/arch/arm/include/asm/vgic.h
+++ b/xen/arch/arm/include/asm/vgic.h
@@ -153,6 +153,8 @@ struct vgic_dist {
     /* Base address for guest GIC */
     paddr_t dbase; /* Distributor base address */
     paddr_t cbase; /* CPU interface base address */
+    paddr_t csize; /* CPU interface size */
+    paddr_t vbase; /* Virtual CPU interface base address */
 #ifdef CONFIG_GICV3
     /* GIC V3 addressing */
     /* List of contiguous occupied by the redistributors */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 061c92acbd..9dd703f661 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1787,9 +1787,12 @@ static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
 }
 
 /*
- * When using ACPI, most of the MMIO regions will be mapped on-demand
- * in stage-2 page tables for the hardware domain because Xen is not
- * able to know from the EFI memory map the MMIO regions.
+ * Try to map the MMIO regions for some special cases:
+ * 1. When using ACPI, most of the MMIO regions will be mapped on-demand
+ *    in stage-2 page tables for the hardware domain because Xen is not
+ *    able to know from the EFI memory map the MMIO regions.
+ * 2. For guests using GICv2, the GICv2 CPU interface mapping is created
+ *    on the first access of the MMIO region.
  */
 static bool try_map_mmio(gfn_t gfn)
 {
@@ -1798,6 +1801,15 @@ static bool try_map_mmio(gfn_t gfn)
     /* For the hardware domain, all MMIOs are mapped with GFN == MFN */
     mfn_t mfn = _mfn(gfn_x(gfn));
 
+    /*
+     * Map the GICv2 virtual CPU interface in the GIC CPU interface
+     * region of the guest on the first access of the MMIO region.
+     */
+    if ( d->arch.vgic.version == GIC_V2 &&
+         gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) )
+        return !map_mmio_regions(d, gfn, d->arch.vgic.csize / PAGE_SIZE,
+                                 maddr_to_mfn(d->arch.vgic.vbase));
+
     /*
      * Device-Tree should already have everything mapped when building
      * the hardware domain.
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 0026cb4360..0b083c33e6 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -644,10 +644,6 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
 
 static int vgic_v2_domain_init(struct domain *d)
 {
-    int ret;
-    paddr_t csize;
-    paddr_t vbase;
-
     /*
      * The hardware domain and direct-mapped domain both get the hardware
      * address.
@@ -667,8 +663,8 @@ static int vgic_v2_domain_init(struct domain *d)
          * aligned to PAGE_SIZE.
          */
         d->arch.vgic.cbase = vgic_v2_hw.cbase;
-        csize = vgic_v2_hw.csize;
-        vbase = vgic_v2_hw.vbase;
+        d->arch.vgic.csize = vgic_v2_hw.csize;
+        d->arch.vgic.vbase = vgic_v2_hw.vbase;
     }
     else if ( is_domain_direct_mapped(d) )
     {
@@ -683,8 +679,8 @@ static int vgic_v2_domain_init(struct domain *d)
          */
         d->arch.vgic.dbase = vgic_v2_hw.dbase;
         d->arch.vgic.cbase = vgic_v2_hw.cbase;
-        csize = GUEST_GICC_SIZE;
-        vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
+        d->arch.vgic.csize = GUEST_GICC_SIZE;
+        d->arch.vgic.vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
     }
     else
     {
@@ -697,18 +693,11 @@ static int vgic_v2_domain_init(struct domain *d)
          */
         BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
         d->arch.vgic.cbase = GUEST_GICC_BASE;
-        csize = GUEST_GICC_SIZE;
-        vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
+        d->arch.vgic.csize = GUEST_GICC_SIZE;
+        d->arch.vgic.vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
     }
 
-    /*
-     * Map the gic virtual cpu interface in the gic cpu interface
-     * region of the guest.
-     */
-    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
-                           csize / PAGE_SIZE, maddr_to_mfn(vbase));
-    if ( ret )
-        return ret;
+    /* Mapping of the virtual CPU interface is deferred until first access */
 
     register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase,
                           PAGE_SIZE, NULL);
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index 07c8f8a005..1308948eec 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -258,8 +258,6 @@ void vgic_v2_enable(struct vcpu *vcpu)
 int vgic_v2_map_resources(struct domain *d)
 {
     struct vgic_dist *dist = &d->arch.vgic;
-    paddr_t csize;
-    paddr_t vbase;
     int ret;
 
     /*
@@ -272,7 +270,7 @@ int vgic_v2_map_resources(struct domain *d)
      */
     if ( is_hardware_domain(d) )
     {
-        d->arch.vgic.dbase = gic_v2_hw_data.dbase;
+        dist->dbase = gic_v2_hw_data.dbase;
         /*
          * For the hardware domain, we always map the whole HW CPU
          * interface region in order to match the device tree (the "reg"
@@ -280,13 +278,13 @@ int vgic_v2_map_resources(struct domain *d)
          * Note that we assume the size of the CPU interface is always
          * aligned to PAGE_SIZE.
          */
-        d->arch.vgic.cbase = gic_v2_hw_data.cbase;
-        csize = gic_v2_hw_data.csize;
-        vbase = gic_v2_hw_data.vbase;
+        dist->cbase = gic_v2_hw_data.cbase;
+        dist->csize = gic_v2_hw_data.csize;
+        dist->vbase = gic_v2_hw_data.vbase;
     }
     else if ( is_domain_direct_mapped(d) )
     {
-        d->arch.vgic.dbase = gic_v2_hw_data.dbase;
+        dist->dbase = gic_v2_hw_data.dbase;
         /*
          * For all the direct-mapped domain other than the hardware domain,
          * we only map a 8kB CPU interface but we make sure it is at a location
@@ -296,13 +294,13 @@ int vgic_v2_map_resources(struct domain *d)
          * address when the GIC is aliased to get a 8kB contiguous
          * region.
          */
-        d->arch.vgic.cbase = gic_v2_hw_data.cbase;
-        csize = GUEST_GICC_SIZE;
-        vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
+        dist->cbase = gic_v2_hw_data.cbase;
+        dist->csize = GUEST_GICC_SIZE;
+        dist->vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
     }
     else
     {
-        d->arch.vgic.dbase = GUEST_GICD_BASE;
+        dist->dbase = GUEST_GICD_BASE;
         /*
          * The CPU interface exposed to the guest is always 8kB. We may
          * need to add an offset to the virtual CPU interface base
@@ -310,9 +308,9 @@ int vgic_v2_map_resources(struct domain *d)
          * region.
          */
         BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
-        d->arch.vgic.cbase = GUEST_GICC_BASE;
-        csize = GUEST_GICC_SIZE;
-        vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
+        dist->cbase = GUEST_GICC_BASE;
+        dist->csize = GUEST_GICC_SIZE;
+        dist->vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
     }
 
 
@@ -323,17 +321,7 @@ int vgic_v2_map_resources(struct domain *d)
         return ret;
     }
 
-    /*
-     * Map the gic virtual cpu interface in the gic cpu interface
-     * region of the guest.
-     */
-    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
-                           csize / PAGE_SIZE, maddr_to_mfn(vbase));
-    if ( ret )
-    {
-        gdprintk(XENLOG_ERR, "Unable to remap VGIC CPU to VCPU\n");
-        return ret;
-    }
+    /* Mapping of the virtual CPU interface is deferred until first access */
 
     dist->ready = true;
 
-- 
2.25.1

[PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

[Henry Wang]

With the change in previous patch, the initial 16 pages in the P2M
pool is not necessary anymore. Drop them for code simplification.

Also the call to p2m_teardown() from arch_domain_destroy() is not
necessary anymore since the movement of the P2M allocation out of
arch_domain_create(). Drop the code and the above in-code comment
mentioning it.

Signed-off-by: Henry Wang <Henry.Wang@arm.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
---
I am not entirely sure if I should also drop the "TODO" on top of
the p2m_set_entry(). Because although we are sure there is no
p2m pages populated in domain_create() stage now, but we are not
sure if anyone will add more in the future...Any comments?
v1 -> v2:
1. Add Reviewed-by tag from Michal.
---
 xen/arch/arm/include/asm/p2m.h |  4 ----
 xen/arch/arm/p2m.c             | 20 +-------------------
 2 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
index bf5183e53a..cf06d3cc21 100644
--- a/xen/arch/arm/include/asm/p2m.h
+++ b/xen/arch/arm/include/asm/p2m.h
@@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
  *  - p2m_final_teardown() will be called when domain struct is been
  *    freed. This *cannot* be preempted and therefore one small
  *    resources should be freed here.
- *  Note that p2m_final_teardown() will also call p2m_teardown(), to properly
- *  free the P2M when failures happen in the domain creation with P2M pages
- *  already in use. In this case p2m_teardown() is called non-preemptively and
- *  p2m_teardown() will always return 0.
  */
 int p2m_teardown(struct domain *d, bool allow_preemption);
 void p2m_final_teardown(struct domain *d);
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index f1ccd7efbd..46406a9eb1 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1750,13 +1750,9 @@ void p2m_final_teardown(struct domain *d)
     /*
      * No need to call relinquish_p2m_mapping() here because
      * p2m_final_teardown() is called either after domain_relinquish_resources()
-     * where relinquish_p2m_mapping() has been called, or from failure path of
-     * domain_create()/arch_domain_create() where mappings that require
-     * p2m_put_l3_page() should never be created. For the latter case, also see
-     * comment on top of the p2m_set_entry() for more info.
+     * where relinquish_p2m_mapping() has been called.
      */
 
-    BUG_ON(p2m_teardown(d, false));
     ASSERT(page_list_empty(&p2m->pages));
 
     while ( p2m_teardown_allocation(d) == -ERESTART )
@@ -1827,20 +1823,6 @@ int p2m_init(struct domain *d)
     if ( rc )
         return rc;
 
-    /*
-     * Hardware using GICv2 needs to create a P2M mapping of 8KB GICv2 area
-     * when the domain is created. Considering the worst case for page
-     * tables and keep a buffer, populate 16 pages to the P2M pages pool here.
-     * For GICv3, the above-mentioned P2M mapping is not necessary, but since
-     * the allocated 16 pages here would not be lost, hence populate these
-     * pages unconditionally.
-     */
-    spin_lock(&d->arch.paging.lock);
-    rc = p2m_set_allocation(d, 16, NULL);
-    spin_unlock(&d->arch.paging.lock);
-    if ( rc )
-        return rc;
-
     return 0;
 }
 
-- 
2.25.1

RE: [PATCH v2 0/4] P2M improvements for Arm

[Henry Wang]

Hi everyone,

> -----Original Message-----
> Subject: [PATCH v2 0/4] P2M improvements for Arm
> 
> There are some clean-up/improvement work that can be done in the
> Arm P2M code triggered by [1] and [2]. These were found at the 4.17
> code freeze period so the issues were not fixed at that time.
> Therefore do the follow-ups here.
> 
> Patch#1 addresses one comment in [1]. It was sent earlier and reviewed
> once. Pick the updated version, i.e. "[PATCH v2] xen/arm: Reduce
> redundant clear root pages when teardown p2m", to this series.
> 
> Patch#2 is a new patch based on v1 comments, this is a pre-requisite
> patch for patch#3 where the deferring of GICv2 CPU interface mapping
> should also be applied for new vGIC.
> 
> Patch#3 and #4 addresses the comment in [2] following the discussion
> between two possible options.
> 
> [1] https://lore.kernel.org/xen-devel/a947e0b4-8f76-cea6-893f-
> abf30ff95e0d@xen.org/
> [2] https://lore.kernel.org/xen-devel/e6643bfc-5bdf-f685-1b68-
> b28d341071c1@xen.org/
> 
> v1 -> v2:
> 1. Move in-code comment for p2m_force_tlb_flush_sync() on top of
>    p2m_clear_root_pages().
> 2. Add a new patch as patch #2.
> 3. Correct style in in-code comment in patch #3.
> 4. Avoid open-coding gfn_eq() and gaddr_to_gfn(d->arch.vgic.cbase).
> 5. Apply same changes for the new vGICv2 implementation, update the
>    commit message accordingly.
> 6. Add in-code comment in old GICv2's vgic_v2_domain_init() and
>    new GICv2's vgic_v2_map_resources() to mention the mapping of the
>    virtual CPU interface is deferred until first access.
> 7. Add reviewed-by and acked-by tags accordingly.
> 
> Henry Wang (4):
>   xen/arm: Reduce redundant clear root pages when teardown p2m
>   xen/arm: Rename vgic_cpu_base and vgic_dist_base for new vGIC
>   xen/arm: Defer GICv2 CPU interface mapping until the first access
>   xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

Gentle ping of last 3 patches of this series as it has been a while. I
understand since we lost all arm32 boards in OSSTest so it is not likely
for a GICv2 series to progress, but this series passed our internal CI on
all GICv2 boards and it would be good to have some feedbacks so that
I can make the series ready to be committed before the arm32 boards
are re-added to OSSTest.

Thanks very much!

Kind regards,
Henry

Re: [PATCH v2 2/4] xen/arm: Rename vgic_cpu_base and vgic_dist_base for new vGIC

[Julien Grall]

Hi Henry,

On 30/01/2023 04:06, Henry Wang wrote:
> In the follow-up patch from this series, the GICv2 CPU interface
> mapping will be deferred until the first access in the stage-2
> data abort trap handling code. Since the data abort trap handling
> code is common for the current and the new vGIC implementation,
> it is necessary to unify the variable names in struct vgic_dist
> for these two implementations.
> 
> Therefore, this commit renames the vgic_cpu_base and vgic_dist_base
> for new vGIC to cbase and dbase. So we can use the same code in
> the data abort trap handling code for both vGIC implementations.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until the first access

[Julien Grall]

Hi Henry,

On 30/01/2023 04:06, Henry Wang wrote:
> Currently, the mapping of the GICv2 CPU interface is created in
> arch_domain_create(). This causes some troubles in populating and
> freeing of the domain P2M pages pool. For example, a default 16
> P2M pages are required in p2m_init() to cope with the P2M mapping
> of 8KB GICv2 CPU interface area, and these 16 P2M pages would cause
> the complexity of P2M destroy in the failure path of
> arch_domain_create().
> 
> As per discussion in [1], similarly as the MMIO access for ACPI, this
> patch defers the GICv2 CPU interface mapping until the first MMIO
> access. This is achieved by moving the GICv2 CPU interface mapping
> code from vgic_v2_domain_init()/vgic_v2_map_resources() to the
> stage-2 data abort trap handling code. The original CPU interface
> size and virtual CPU interface base address is now saved in
> `struct vgic_dist` instead of the local variable of
> vgic_v2_domain_init()/vgic_v2_map_resources().
> 
> Take the opportunity to unify the way of data access using the
> existing pointer to struct vgic_dist in vgic_v2_map_resources() for
> new GICv2.
> 
> Since the hardware domain (Dom0) has an unlimited size P2M pool, the
> gicv2_map_hwdom_extra_mappings() is also not touched by this patch.

I didn't notice this in the previous version. The fact that dom0 has 
unlimited size P2M pool doesn't matter here (this could also change in 
the future). Even if the P2M pool was limited, then it would be fine 
because the extra mappings happen after domain_create(). So there is no 
need to map them on-demand as the code could be order the way we want.

So this paragraph needs to be reworded.

>   #ifdef CONFIG_GICV3
>       /* GIC V3 addressing */
>       /* List of contiguous occupied by the redistributors */
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 061c92acbd..9dd703f661 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1787,9 +1787,12 @@ static inline bool hpfar_is_valid(bool s1ptw, uint8_t fsc)
>   }
>   
>   /*
> - * When using ACPI, most of the MMIO regions will be mapped on-demand
> - * in stage-2 page tables for the hardware domain because Xen is not
> - * able to know from the EFI memory map the MMIO regions.
> + * Try to map the MMIO regions for some special cases:
> + * 1. When using ACPI, most of the MMIO regions will be mapped on-demand
> + *    in stage-2 page tables for the hardware domain because Xen is not
> + *    able to know from the EFI memory map the MMIO regions.
> + * 2. For guests using GICv2, the GICv2 CPU interface mapping is created
> + *    on the first access of the MMIO region.
>    */
>   static bool try_map_mmio(gfn_t gfn)
>   {
> @@ -1798,6 +1801,15 @@ static bool try_map_mmio(gfn_t gfn)
>       /* For the hardware domain, all MMIOs are mapped with GFN == MFN */
>       mfn_t mfn = _mfn(gfn_x(gfn));
>   
> +    /*
> +     * Map the GICv2 virtual CPU interface in the GIC CPU interface
> +     * region of the guest on the first access of the MMIO region.
> +     */
> +    if ( d->arch.vgic.version == GIC_V2 &&
> +         gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) )

The CPU interface size is 8KB (bigger in some cases) but here you only 
check for the access to be in the first 4KB.

Cheers,

-- 
Julien Grall

RE: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until the first access

[Henry Wang]

Hi Julien,

Thanks very much for your time and review :)

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until
> the first access
> 
> Hi Henry,
> 
> On 30/01/2023 04:06, Henry Wang wrote:
> > Since the hardware domain (Dom0) has an unlimited size P2M pool, the
> > gicv2_map_hwdom_extra_mappings() is also not touched by this patch.
> 
> I didn't notice this in the previous version. The fact that dom0 has
> unlimited size P2M pool doesn't matter here (this could also change in
> the future). Even if the P2M pool was limited, then it would be fine
> because the extra mappings happen after domain_create(). So there is no
> need to map them on-demand as the code could be order the way we want.
> 
> So this paragraph needs to be reworded.

Sure, I've reworded this paragraph to below:
"Since gicv2_map_hwdom_extra_mappings() happens after domain_create(),
so there is no need to map the extra mappings on-demand, and therefore
keep the hwdom extra mappings as untouched."

> 
> > +    /*
> > +     * Map the GICv2 virtual CPU interface in the GIC CPU interface
> > +     * region of the guest on the first access of the MMIO region.
> > +     */
> > +    if ( d->arch.vgic.version == GIC_V2 &&
> > +         gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) )
> 
> The CPU interface size is 8KB (bigger in some cases) but here you only
> check for the access to be in the first 4KB.

Yeah indeed, gfn might fall into the range between 4KB and 8KB, sorry
about that.

Considering that the CPU interface is continuous (I suppose), I have two
ways of rewriting the gfn check, we can do either:

gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) ||
gfn_eq(gfn, gfn_add(gaddr_to_gfn(d->arch.vgic.cbase), 1))

or

gfn_to_gaddr(gfn) >= d->arch.vgic.cbase ||
gfn_to_gaddr(gfn) < d->arch.vgic.cbase + d->arch.vgic.csize

May I ask which one would you think is better? Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until the first access

[Henry Wang]

Hi,

> Considering that the CPU interface is continuous (I suppose), I have two
> ways of rewriting the gfn check, we can do either:
> 
> gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) ||
> gfn_eq(gfn, gfn_add(gaddr_to_gfn(d->arch.vgic.cbase), 1))
> 
> or
> 
> gfn_to_gaddr(gfn) >= d->arch.vgic.cbase ||
> gfn_to_gaddr(gfn) < d->arch.vgic.cbase + d->arch.vgic.csize

Oops, copy paste error, this should be 

gfn_to_gaddr(gfn) >= d->arch.vgic.cbase &&
gfn_to_gaddr(gfn) < d->arch.vgic.cbase + d->arch.vgic.csize

Kind regards,
Henry

Re: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until the first access

[Julien Grall]

On 21/03/2023 03:57, Henry Wang wrote:
> Hi Julien,

Hi Henry,

>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until
>> the first access
>>
>> Hi Henry,
>>
>> On 30/01/2023 04:06, Henry Wang wrote:
>>> Since the hardware domain (Dom0) has an unlimited size P2M pool, the
>>> gicv2_map_hwdom_extra_mappings() is also not touched by this patch.
>>
>> I didn't notice this in the previous version. The fact that dom0 has
>> unlimited size P2M pool doesn't matter here (this could also change in
>> the future). Even if the P2M pool was limited, then it would be fine
>> because the extra mappings happen after domain_create(). So there is no
>> need to map them on-demand as the code could be order the way we want.
>>
>> So this paragraph needs to be reworded.
> 
> Sure, I've reworded this paragraph to below:
> "Since gicv2_map_hwdom_extra_mappings() happens after domain_create(),
> so there is no need to map the extra mappings on-demand, and therefore
> keep the hwdom extra mappings as untouched."

Looks good to me.

> 
>>
>>> +    /*
>>> +     * Map the GICv2 virtual CPU interface in the GIC CPU interface
>>> +     * region of the guest on the first access of the MMIO region.
>>> +     */
>>> +    if ( d->arch.vgic.version == GIC_V2 &&
>>> +         gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) )
>>
>> The CPU interface size is 8KB (bigger in some cases) but here you only
>> check for the access to be in the first 4KB.
> 
> Yeah indeed, gfn might fall into the range between 4KB and 8KB, sorry
> about that.
> 
> Considering that the CPU interface is continuous (I suppose), I have two
> ways of rewriting the gfn check, we can do either:
> 
> gfn_eq(gfn, gaddr_to_gfn(d->arch.vgic.cbase)) ||
> gfn_eq(gfn, gfn_add(gaddr_to_gfn(d->arch.vgic.cbase), 1))

This check is incorrect as you are making the assumption that the size 
is 8KB. As I hinted in the previous e-mail, the size could be bigger. 
For instance, for dom0, it could be up to 128KB when the GICC is aliased.

So you want to...

> 
> or
> 
> gfn_to_gaddr(gfn) >= d->arch.vgic.cbase ||
> gfn_to_gaddr(gfn) < d->arch.vgic.cbase + d->arch.vgic.csize

... use the size in the check. With the || switch to &&, my preference 
would be to use:

((d->arch.vgic.cbase - gfn_to_addr(gfn)) < d->arch.vgic.csize)

to avoid a potential overflow in the unlikely case the CPU interface is 
at the top of the address space.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

[Julien Grall]

Hi Henry,

On 30/01/2023 04:06, Henry Wang wrote:
> With the change in previous patch, the initial 16 pages in the P2M
> pool is not necessary anymore. Drop them for code simplification.
> 
> Also the call to p2m_teardown() from arch_domain_destroy() is not
> necessary anymore since the movement of the P2M allocation out of
> arch_domain_create(). Drop the code and the above in-code comment
> mentioning it.
> 
> Signed-off-by: Henry Wang <Henry.Wang@arm.com>
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> ---
> I am not entirely sure if I should also drop the "TODO" on top of
> the p2m_set_entry(). Because although we are sure there is no
> p2m pages populated in domain_create() stage now, but we are not
> sure if anyone will add more in the future...Any comments?

I would keep it.

> v1 -> v2:
> 1. Add Reviewed-by tag from Michal.
> ---
>   xen/arch/arm/include/asm/p2m.h |  4 ----
>   xen/arch/arm/p2m.c             | 20 +-------------------
>   2 files changed, 1 insertion(+), 23 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> index bf5183e53a..cf06d3cc21 100644
> --- a/xen/arch/arm/include/asm/p2m.h
> +++ b/xen/arch/arm/include/asm/p2m.h
> @@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
>    *  - p2m_final_teardown() will be called when domain struct is been
>    *    freed. This *cannot* be preempted and therefore one small

NIT: As you are touching the comment, would you mind to fix the typo 
s/one/only/.

>    *    resources should be freed here.
> - *  Note that p2m_final_teardown() will also call p2m_teardown(), to properly
> - *  free the P2M when failures happen in the domain creation with P2M pages
> - *  already in use. In this case p2m_teardown() is called non-preemptively and
> - *  p2m_teardown() will always return 0.
>    */
>   int p2m_teardown(struct domain *d, bool allow_preemption);
>   void p2m_final_teardown(struct domain *d);
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index f1ccd7efbd..46406a9eb1 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1750,13 +1750,9 @@ void p2m_final_teardown(struct domain *d)
>       /*
>        * No need to call relinquish_p2m_mapping() here because
>        * p2m_final_teardown() is called either after domain_relinquish_resources()
> -     * where relinquish_p2m_mapping() has been called, or from failure path of
> -     * domain_create()/arch_domain_create() where mappings that require
> -     * p2m_put_l3_page() should never be created. For the latter case, also see
> -     * comment on top of the p2m_set_entry() for more info.
> +     * where relinquish_p2m_mapping() has been called.
>        */
>   
> -    BUG_ON(p2m_teardown(d, false));

With this change, I think we can also drop the second parameter of 
p2m_teardown(). Preferably with this change in the patch:

Acked-by: Julien Grall <jgrall@amazon.com>

Preferably, I would like this to happen in this patch.

Cheers,

-- 
Julien Grall

RE: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and
> p2m_final_teardown()
> 
> Hi Henry,
> 
> > ---
> > I am not entirely sure if I should also drop the "TODO" on top of
> > the p2m_set_entry(). Because although we are sure there is no
> > p2m pages populated in domain_create() stage now, but we are not
> > sure if anyone will add more in the future...Any comments?
> 
> I would keep it.

Sure.

> 
> > @@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
> >    *  - p2m_final_teardown() will be called when domain struct is been
> >    *    freed. This *cannot* be preempted and therefore one small
> 
> NIT: As you are touching the comment, would you mind to fix the typo
> s/one/only/.

I would be more than happy to fix it. Thanks for noticing this :)

> 
> > -    BUG_ON(p2m_teardown(d, false));
> 
> With this change, I think we can also drop the second parameter of
> p2m_teardown(). Preferably with this change in the patch:

Yes indeed, I was also thinking of this when writing this patch but in
the end decided to do minimal change..

> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks! I am not 100% comfortable to take this tag because I made
some extra code change, below is the diff to drop the second param
of p2m_teardown():

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
@@ -1030,7 +1030,7 @@ int domain_relinquish_resources(struct domain *d)
         p2m_clear_root_pages(&d->arch.p2m);

     PROGRESS(p2m):
-        ret = p2m_teardown(d, true);
+        ret = p2m_teardown(d);
         if ( ret )
             return ret;

diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
@@ -201,7 +201,7 @@ int p2m_init(struct domain *d);
  *    freed. This *cannot* be preempted and therefore only small
  *    resources should be freed here.
  */
-int p2m_teardown(struct domain *d, bool allow_preemption);
+int p2m_teardown(struct domain *d);
 void p2m_final_teardown(struct domain *d);

diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
-int p2m_teardown(struct domain *d, bool allow_preemption)
+int p2m_teardown(struct domain *d)
 {
[...]
         /* Arbitrarily preempt every 512 iterations */
-        if ( allow_preemption && !(count % 512) && hypercall_preempt_check() )
+        if ( !(count % 512) && hypercall_preempt_check() )

If you are happy, I will take this acked-by. Same question to Michal for his
Reviewed-by.

Kind regards,
Henry

> 
> Preferably, I would like this to happen in this patch.
> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until the first access

[Henry Wang]

Hi Julien,

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Subject: Re: [PATCH v2 3/4] xen/arm: Defer GICv2 CPU interface mapping until
> the first access
>
> > gfn_to_gaddr(gfn) >= d->arch.vgic.cbase ||
> > gfn_to_gaddr(gfn) < d->arch.vgic.cbase + d->arch.vgic.csize
> 
> ... use the size in the check. With the || switch to &&, my preference
> would be to use:
> 
> ((d->arch.vgic.cbase - gfn_to_addr(gfn)) < d->arch.vgic.csize)
> 
> to avoid a potential overflow in the unlikely case the CPU interface is
> at the top of the address space.

Oh I like the idea of using the "minus approach" to avoid the overflow
very much. Thanks! I will keep this in mind in my future development.

One more question, since this "minus approach" is directly derived from
`gfn_to_gaddr(gfn) < d->arch.vgic.cbase + d->arch.vgic.csize`,
shouldn't it be
`(gfn_to_addr(gfn) - d->arch.vgic.cbase) < d->arch.vgic.csize` instead?

Otherwise I think `d->arch.vgic.cbase - gfn_to_addr(gfn)` will produce
a huge u64 and this check would always fail?

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

[Michal Orzel]

Hi Henry,

On 22/03/2023 03:21, Henry Wang wrote:
> 
> 
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and
>> p2m_final_teardown()
>>
>> Hi Henry,
>>
>>> ---
>>> I am not entirely sure if I should also drop the "TODO" on top of
>>> the p2m_set_entry(). Because although we are sure there is no
>>> p2m pages populated in domain_create() stage now, but we are not
>>> sure if anyone will add more in the future...Any comments?
>>
>> I would keep it.
> 
> Sure.
> 
>>
>>> @@ -200,10 +200,6 @@ int p2m_init(struct domain *d);
>>>    *  - p2m_final_teardown() will be called when domain struct is been
>>>    *    freed. This *cannot* be preempted and therefore one small
>>
>> NIT: As you are touching the comment, would you mind to fix the typo
>> s/one/only/.
> 
> I would be more than happy to fix it. Thanks for noticing this :)
> 
>>
>>> -    BUG_ON(p2m_teardown(d, false));
>>
>> With this change, I think we can also drop the second parameter of
>> p2m_teardown(). Preferably with this change in the patch:
> 
> Yes indeed, I was also thinking of this when writing this patch but in
> the end decided to do minimal change..
> 
>>
>> Acked-by: Julien Grall <jgrall@amazon.com>
> 
> Thanks! I am not 100% comfortable to take this tag because I made
> some extra code change, below is the diff to drop the second param
> of p2m_teardown():
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> @@ -1030,7 +1030,7 @@ int domain_relinquish_resources(struct domain *d)
>          p2m_clear_root_pages(&d->arch.p2m);
> 
>      PROGRESS(p2m):
> -        ret = p2m_teardown(d, true);
> +        ret = p2m_teardown(d);
>          if ( ret )
>              return ret;
> 
> diff --git a/xen/arch/arm/include/asm/p2m.h b/xen/arch/arm/include/asm/p2m.h
> @@ -201,7 +201,7 @@ int p2m_init(struct domain *d);
>   *    freed. This *cannot* be preempted and therefore only small
>   *    resources should be freed here.
>   */
> -int p2m_teardown(struct domain *d, bool allow_preemption);
> +int p2m_teardown(struct domain *d);
>  void p2m_final_teardown(struct domain *d);
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> -int p2m_teardown(struct domain *d, bool allow_preemption)
> +int p2m_teardown(struct domain *d)
>  {
> [...]
>          /* Arbitrarily preempt every 512 iterations */
> -        if ( allow_preemption && !(count % 512) && hypercall_preempt_check() )
> +        if ( !(count % 512) && hypercall_preempt_check() )
> 
> If you are happy, I will take this acked-by. Same question to Michal for his
> Reviewed-by.
The diff looks good to me and surely you can keep my tag.
However, we might want to modify the comment on top of p2m_teardown() prototype as
it assumes presence of preemptive/non-preemptive p2m_teardown() call (at least this
is how I understand this).

~Michal

RE: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and p2m_final_teardown()

[Henry Wang]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> Subject: Re: [PATCH v2 4/4] xen/arm: Clean-up in p2m_init() and
> p2m_final_teardown()
> > If you are happy, I will take this acked-by. Same question to Michal for his
> > Reviewed-by.
> The diff looks good to me and surely you can keep my tag.

Great, thanks!

> However, we might want to modify the comment on top of p2m_teardown()
> prototype as
> it assumes presence of preemptive/non-preemptive p2m_teardown() call (at
> least this
> is how I understand this).

I understand your point. I will revert it to its original form written by Julien:

"p2m_teardown() will be called when relinquish the resources. It
will free large resources (e.g. intermediate page-tables) that
requires preemption."

Kind regards,
Henry

> 
> ~Michal