[PATCH 0/3] P2M improvements for Arm

[PATCH 0/3] 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 and #3 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/

Henry Wang (3):
  xen/arm: Reduce redundant clear root pages when teardown p2m
  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           | 12 ++++++++
 xen/arch/arm/include/asm/p2m.h  |  5 +--
 xen/arch/arm/include/asm/vgic.h |  2 ++
 xen/arch/arm/p2m.c              | 54 +++++++++------------------------
 xen/arch/arm/traps.c            | 19 ++++++++++--
 xen/arch/arm/vgic-v2.c          | 25 ++++-----------
 6 files changed, 52 insertions(+), 65 deletions(-)

-- 
2.25.1

[PATCH 1/3] 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>
---
Was: [PATCH v2] xen/arm: Reduce redundant clear root pages when
teardown p2m. Picked to this series with changes in original v1:
1. Introduce a new PROGRESS for p2m_clear_root_pages() to avoid
   multiple calling when p2m_teardown() is preempted.
2. Move p2m_force_tlb_flush_sync() to p2m_clear_root_pages().
---
 xen/arch/arm/domain.c          | 12 ++++++++++++
 xen/arch/arm/include/asm/p2m.h |  1 +
 xen/arch/arm/p2m.c             | 34 ++++++++++++++--------------------
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 99577adb6c..961dab9166 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,17 @@ 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.
+         * 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_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..7de7d822e9 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1314,6 +1314,20 @@ static void p2m_invalidate_table(struct p2m_domain *p2m, mfn_t mfn)
     p2m->need_flush = true;
 }
 
+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 +1712,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 2/3] 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() 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().

Note that GICv2 changes introduced by this patch is not applied to the
"New vGIC" implementation, as the "New vGIC" is not used. Also 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>
---
 xen/arch/arm/include/asm/vgic.h |  2 ++
 xen/arch/arm/traps.c            | 19 ++++++++++++++++---
 xen/arch/arm/vgic-v2.c          | 25 ++++++-------------------
 3 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
index 3d44868039..1d37c291e1 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..d98f166050 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,16 @@ 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_x(gfn) == gfn_x(gaddr_to_gfn(d->arch.vgic.cbase)) )
+        return !map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
+                                 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..21e14a5a6f 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,19 +693,10 @@ 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;
-
     register_mmio_handler(d, &vgic_v2_distr_mmio_handler, d->arch.vgic.dbase,
                           PAGE_SIZE, NULL);
 
-- 
2.25.1

[PATCH 3/3] 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>
---
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?
---
 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 7de7d822e9..d41a316d18 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1744,13 +1744,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 )
@@ -1821,20 +1817,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 1/3] xen/arm: Reduce redundant clear root pages when teardown p2m

[Michal Orzel]

Hi Henry,

On 16/01/2023 02:58, Henry Wang wrote:
> 
> 
> 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>
> ---
> Was: [PATCH v2] xen/arm: Reduce redundant clear root pages when
> teardown p2m. Picked to this series with changes in original v1:
> 1. Introduce a new PROGRESS for p2m_clear_root_pages() to avoid
>    multiple calling when p2m_teardown() is preempted.
> 2. Move p2m_force_tlb_flush_sync() to p2m_clear_root_pages().
> ---
>  xen/arch/arm/domain.c          | 12 ++++++++++++
>  xen/arch/arm/include/asm/p2m.h |  1 +
>  xen/arch/arm/p2m.c             | 34 ++++++++++++++--------------------
>  3 files changed, 27 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 99577adb6c..961dab9166 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,17 @@ 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.
The comment from here...
> +         * 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.
> +         */
to here does not make a lot of sense in this place and should be moved to p2m_clear_root_pages
where a user can see the call to p2m_force_tlb_flush_sync.

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

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

[Michal Orzel]

Hi Henry,

On 16/01/2023 02:58, 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() 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().
> 
> Note that GICv2 changes introduced by this patch is not applied to the
> "New vGIC" implementation, as the "New vGIC" is not used. Also since
The fact that "New vGIC" is not used very often and its work is in-progress
does not mean that we can implement changes resulting in a build failure
when enabling CONFIG_NEW_VGIC, which is the case with your patch.
So this needs to be fixed.

> 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>
> ---
>  xen/arch/arm/include/asm/vgic.h |  2 ++
>  xen/arch/arm/traps.c            | 19 ++++++++++++++++---
>  xen/arch/arm/vgic-v2.c          | 25 ++++++-------------------
>  3 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/vgic.h b/xen/arch/arm/include/asm/vgic.h
> index 3d44868039..1d37c291e1 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 */
Could you swap them so that base address variables are grouped?

>  #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..d98f166050 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,16 @@ 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
NIT: in all the other places you use CPU (capital letters)

> +     * region of the guest on the first access of the MMIO region.
> +     */
> +    if ( d->arch.vgic.version == GIC_V2 &&
> +         gfn_x(gfn) == gfn_x(gaddr_to_gfn(d->arch.vgic.cbase)) )
There is a macro gnf_eq that you can use to avoid opencoding it.

> +        return !map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
At this point you can use just gfn instead of gaddr_to_gfn(d->arch.vgic.cbase)

> +                                 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..21e14a5a6f 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,19 +693,10 @@ 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;
> -
Maybe adding some comment like "Mapping of the virtual CPU interface is deferred until first access"
would be helpful.

~Michal

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

[Michal Orzel]

Hi Henry,

On 16/01/2023 02:58, 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>
> ---
> 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?
> ---
>  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 7de7d822e9..d41a316d18 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1744,13 +1744,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));
Because you remove this,
>      ASSERT(page_list_empty(&p2m->pages));
you no longer need this assert, right?

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

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

[Julien Grall]

Hi,

On 20/01/2023 10:23, Michal Orzel wrote:
> Hi Henry,
> 
> On 16/01/2023 02:58, 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>
>> ---
>> 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?
>> ---
>>   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 7de7d822e9..d41a316d18 100644
>> --- a/xen/arch/arm/p2m.c
>> +++ b/xen/arch/arm/p2m.c
>> @@ -1744,13 +1744,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));
> Because you remove this,
>>       ASSERT(page_list_empty(&p2m->pages));
> you no longer need this assert, right?
I think the ASSERT() is still useful as it at least show that the pages 
should have been freed before the call to p2m_final_teardown().

Cheers,

-- 
Julien Grall

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

[Julien Grall]

On 20/01/2023 09:43, Michal Orzel wrote:
> Hi Henry,
> 
> On 16/01/2023 02:58, Henry Wang wrote:
>>
>>
>> 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>
>> ---
>> Was: [PATCH v2] xen/arm: Reduce redundant clear root pages when
>> teardown p2m. Picked to this series with changes in original v1:
>> 1. Introduce a new PROGRESS for p2m_clear_root_pages() to avoid
>>     multiple calling when p2m_teardown() is preempted.
>> 2. Move p2m_force_tlb_flush_sync() to p2m_clear_root_pages().
>> ---
>>   xen/arch/arm/domain.c          | 12 ++++++++++++
>>   xen/arch/arm/include/asm/p2m.h |  1 +
>>   xen/arch/arm/p2m.c             | 34 ++++++++++++++--------------------
>>   3 files changed, 27 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>> index 99577adb6c..961dab9166 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,17 @@ 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.
> The comment from here...
>> +         * 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.
>> +         */
> to here does not make a lot of sense in this place and should be moved to p2m_clear_root_pages
> where a user can see the call to p2m_force_tlb_flush_sync.

+1

> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

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

Cheers,

-- 
Julien Grall

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

[Henry Wang]

Hi Michal and Julien,

Apologies for the late response.

> -----Original Message-----
> >> +    PROGRESS(p2m_root):
> >> +        /*
> >> +         * We are about to free the intermediate page-tables, so clear the
> >> +         * root to prevent any walk to use them.
> > The comment from here...
> >> +         * 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.
> >> +         */
> > to here does not make a lot of sense in this place and should be moved to
> p2m_clear_root_pages
> > where a user can see the call to p2m_force_tlb_flush_sync.
> 
> +1

I will move this code comment block to the place that you suggested in v2.

> 
> > Apart from that:
> > Reviewed-by: Michal Orzel <michal.orzel@amd.com>
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks to both of you. I will keep these two tags with the fix about the above
in-code comment position.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

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

[Henry Wang]

Hi Michal,

> -----Original Message-----
> From: Michal Orzel <michal.orzel@amd.com>
> Subject: Re: [PATCH 2/3] xen/arm: Defer GICv2 CPU interface mapping until
> the first access
> 
> Hi Henry,
> 
> On 16/01/2023 02:58, Henry Wang wrote:
> > Note that GICv2 changes introduced by this patch is not applied to the
> > "New vGIC" implementation, as the "New vGIC" is not used. Also since
> The fact that "New vGIC" is not used very often and its work is in-progress
> does not mean that we can implement changes resulting in a build failure
> when enabling CONFIG_NEW_VGIC, which is the case with your patch.
> So this needs to be fixed.

I will add the "New vGIC" changes in v2.

> 
> > @@ -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 */
> Could you swap them so that base address variables are grouped?

Sure, my original thought was grouping the CPU interface related fields but
since you prefer grouping the base address, I will follow your suggestion.

> 
> >  #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..d98f166050 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,16 @@ 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
> NIT: in all the other places you use CPU (capital letters)

Oh good catch, thank you. I think this part is the same as the original in-code
comment, but since I am touching this part anyway, it would be good to
correct them.

> 
> > +     * region of the guest on the first access of the MMIO region.
> > +     */
> > +    if ( d->arch.vgic.version == GIC_V2 &&
> > +         gfn_x(gfn) == gfn_x(gaddr_to_gfn(d->arch.vgic.cbase)) )
> There is a macro gnf_eq that you can use to avoid opencoding it.

Thanks! I will fix in v2.

> 
> > +        return !map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
> At this point you can use just gfn instead of gaddr_to_gfn(d->arch.vgic.cbase)

Will fix in v2.

> 
> > +                                 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..21e14a5a6f 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,19 +693,10 @@ 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;
> > -
> Maybe adding some comment like "Mapping of the virtual CPU interface is
> deferred until first access"
> would be helpful.

Sure, I will add the comment in v2.

Kind regards,
Henry

> 
> ~Michal

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

[Henry Wang]

Hi Michal,

> -----Original Message-----
> >>
> >> -    BUG_ON(p2m_teardown(d, false));
> > Because you remove this,
> >>       ASSERT(page_list_empty(&p2m->pages));
> > you no longer need this assert, right?
> I think the ASSERT() is still useful as it at least show that the pages
> should have been freed before the call to p2m_final_teardown().

I think I also prefer to have this ASSERT(), because of the exactly same
reason as Julien's answer. I think having this ASSERT() will help us to
avoid potential mistakes in the future.

May I please ask if you are happy with keeping this ASSERT() and I can
carry your reviewed-by tag? Thanks!

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

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

[Julien Grall]

Hi,

On 27/01/2023 11:11, Henry Wang wrote:
>> -----Original Message-----
>> From: Michal Orzel <michal.orzel@amd.com>
>> Subject: Re: [PATCH 2/3] xen/arm: Defer GICv2 CPU interface mapping until
>> the first access
>>
>> Hi Henry,
>>
>> On 16/01/2023 02:58, Henry Wang wrote:
>>> Note that GICv2 changes introduced by this patch is not applied to the
>>> "New vGIC" implementation, as the "New vGIC" is not used. Also since
>> The fact that "New vGIC" is not used very often and its work is in-progress
>> does not mean that we can implement changes resulting in a build failure
>> when enabling CONFIG_NEW_VGIC, which is the case with your patch.
>> So this needs to be fixed.
> 
> I will add the "New vGIC" changes in v2.
> 
>>
>>> @@ -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 */
>> Could you swap them so that base address variables are grouped?
> 
> Sure, my original thought was grouping the CPU interface related fields but
> since you prefer grouping the base address, I will follow your suggestion.

I would actually prefer your approach because it is easier to associate 
the size with the base.

An alternative would be to use a structure to combine the base/size. So 
it is even clearer the association.

I don't have a strong opinion on either of the two approach I suggested.

Cheers,

-- 
Julien Grall

RE: [PATCH 2/3] 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 2/3] xen/arm: Defer GICv2 CPU interface mapping until
> the first access
> 
> Hi,
> 
> >>> @@ -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 */
> >> Could you swap them so that base address variables are grouped?
> >
> > Sure, my original thought was grouping the CPU interface related fields but
> > since you prefer grouping the base address, I will follow your suggestion.
> 
> I would actually prefer your approach because it is easier to associate
> the size with the base.
> 
> An alternative would be to use a structure to combine the base/size. So
> it is even clearer the association.
> 
> I don't have a strong opinion on either of the two approach I suggested.

Maybe we can do something like this:
```
paddr_t dbase; /* Distributor base address */
paddr_t vbase; /* virtual CPU interface base address */
paddr_t cbase; /* CPU interface base address */
paddr_t csize; /* CPU interface size */    
```

So we can ensure both "base address variables are grouped" and
"CPU interface variables are grouped".

If you don't like this, I would prefer the way I am currently doing, as
personally I think an extra structure would slightly be an overkill :)

Thanks.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

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

[Julien Grall]

On 27/01/2023 11:30, Henry Wang wrote:
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH 2/3] xen/arm: Defer GICv2 CPU interface mapping until
>> the first access
>>
>> Hi,
>>
>>>>> @@ -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 */
>>>> Could you swap them so that base address variables are grouped?
>>>
>>> Sure, my original thought was grouping the CPU interface related fields but
>>> since you prefer grouping the base address, I will follow your suggestion.
>>
>> I would actually prefer your approach because it is easier to associate
>> the size with the base.
>>
>> An alternative would be to use a structure to combine the base/size. So
>> it is even clearer the association.
>>
>> I don't have a strong opinion on either of the two approach I suggested.
> 
> Maybe we can do something like this:
> ```
> paddr_t dbase; /* Distributor base address */
> paddr_t vbase; /* virtual CPU interface base address */
> paddr_t cbase; /* CPU interface base address */
> paddr_t csize; /* CPU interface size */
> ```
> 
> So we can ensure both "base address variables are grouped" and
> "CPU interface variables are grouped".
> 
> If you don't like this, I would prefer the way I am currently doing, as
> personally I think an extra structure would slightly be an overkill :)

This is really a matter of taste here. My preference is your initial 
approach because I find strange to have virtual CPU interface 
information the physical one.

Cheers,

-- 
Julien Grall

RE: [PATCH 2/3] 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 2/3] xen/arm: Defer GICv2 CPU interface mapping until
> the first access
> >>>>> @@ -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 */
> >>>> Could you swap them so that base address variables are grouped?
> >>>
> >>> Sure, my original thought was grouping the CPU interface related fields
> but
> >>> since you prefer grouping the base address, I will follow your suggestion.
> >>
> >> I would actually prefer your approach because it is easier to associate
> >> the size with the base.
> >>
> >> An alternative would be to use a structure to combine the base/size. So
> >> it is even clearer the association.
> >>
> >> I don't have a strong opinion on either of the two approach I suggested.
> >
> > Maybe we can do something like this:
> > ```
> > paddr_t dbase; /* Distributor base address */
> > paddr_t vbase; /* virtual CPU interface base address */
> > paddr_t cbase; /* CPU interface base address */
> > paddr_t csize; /* CPU interface size */
> > ```
> >
> > So we can ensure both "base address variables are grouped" and
> > "CPU interface variables are grouped".
> >
> > If you don't like this, I would prefer the way I am currently doing, as
> > personally I think an extra structure would slightly be an overkill :)
> 
> This is really a matter of taste here.

Indeed,

> My preference is your initial
> approach because I find strange to have virtual CPU interface
> information the physical one.

then I will keep it as it is if there is no strong objection from Michal.

Kind regards,
Henry

> 
> Cheers,
> 
> --
> Julien Grall

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

[Michal Orzel]

Hi Henry,

On 27/01/2023 12:39, Henry Wang wrote:
> 
> 
> Hi Julien,
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Subject: Re: [PATCH 2/3] xen/arm: Defer GICv2 CPU interface mapping until
>> the first access
>>>>>>> @@ -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 */
>>>>>> Could you swap them so that base address variables are grouped?
>>>>>
>>>>> Sure, my original thought was grouping the CPU interface related fields
>> but
>>>>> since you prefer grouping the base address, I will follow your suggestion.
>>>>
>>>> I would actually prefer your approach because it is easier to associate
>>>> the size with the base.
>>>>
>>>> An alternative would be to use a structure to combine the base/size. So
>>>> it is even clearer the association.
>>>>
>>>> I don't have a strong opinion on either of the two approach I suggested.
>>>
>>> Maybe we can do something like this:
>>> ```
>>> paddr_t dbase; /* Distributor base address */
>>> paddr_t vbase; /* virtual CPU interface base address */
>>> paddr_t cbase; /* CPU interface base address */
>>> paddr_t csize; /* CPU interface size */
>>> ```
>>>
>>> So we can ensure both "base address variables are grouped" and
>>> "CPU interface variables are grouped".
>>>
>>> If you don't like this, I would prefer the way I am currently doing, as
>>> personally I think an extra structure would slightly be an overkill :)
>>
>> This is really a matter of taste here.
> 
> Indeed,
> 
>> My preference is your initial
>> approach because I find strange to have virtual CPU interface
>> information the physical one.
> 
> then I will keep it as it is if there is no strong objection from Michal.
there are none. It was just a suggestion.

~Michal

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

[Michal Orzel]

Hi Henry,

On 27/01/2023 12:15, Henry Wang wrote:
> 
> 
> Hi Michal,
> 
>> -----Original Message-----
>>>>
>>>> -    BUG_ON(p2m_teardown(d, false));
>>> Because you remove this,
>>>>       ASSERT(page_list_empty(&p2m->pages));
>>> you no longer need this assert, right?
>> I think the ASSERT() is still useful as it at least show that the pages
>> should have been freed before the call to p2m_final_teardown().
> 
> I think I also prefer to have this ASSERT(), because of the exactly same
> reason as Julien's answer. I think having this ASSERT() will help us to
> avoid potential mistakes in the future.
> 
> May I please ask if you are happy with keeping this ASSERT() and I can
> carry your reviewed-by tag? Thanks!
Yes, you can :)

~Michal

RE: [PATCH 3/3] 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 3/3] xen/arm: Clean-up in p2m_init() and
> p2m_final_teardown()
> 
> Hi Henry,
> 
> On 27/01/2023 12:15, Henry Wang wrote:
> >
> >
> > Hi Michal,
> >
> >> -----Original Message-----
> >>>>
> >>>> -    BUG_ON(p2m_teardown(d, false));
> >>> Because you remove this,
> >>>>       ASSERT(page_list_empty(&p2m->pages));
> >>> you no longer need this assert, right?
> >> I think the ASSERT() is still useful as it at least show that the pages
> >> should have been freed before the call to p2m_final_teardown().
> >
> > I think I also prefer to have this ASSERT(), because of the exactly same
> > reason as Julien's answer. I think having this ASSERT() will help us to
> > avoid potential mistakes in the future.
> >
> > May I please ask if you are happy with keeping this ASSERT() and I can
> > carry your reviewed-by tag? Thanks!
> Yes, you can :)

Thank you very much! And also thank you for your detailed review :)

Kind regards,
Henry

> 
> ~Michal