[PATCH v3 00/10] direct-map memory map

[PATCH v3 00/10] direct-map memory map

[Penny Zheng]

Cases where domU needs direct-map memory map:
  * IOMMU not present in the system.
  * IOMMU disabled if it doesn't cover a specific device and all the guests
are trusted. Thinking a mixed scenario, where a few devices with IOMMU and
a few without, then guest DMA security still could not be totally guaranteed.
So users may want to disable the IOMMU, to at least gain some performance
improvement from IOMMU disabled.
  * IOMMU disabled as a workaround when it doesn't have enough bandwidth.
To be specific, in a few extreme situation, when multiple devices do DMA
concurrently, these requests may exceed IOMMU's transmission capacity.
  * IOMMU disabled when it adds too much latency on DMA. For example,
TLB may be missing in some IOMMU hardware, which may bring latency in DMA
progress, so users may want to disable it in some realtime scenario.
  * Guest OS relies on the host memory layout

"direct-map" property shall be added under the appropriate domain node,
when users requesting direct-map memory mapping for the domain.

Right now, direct-map is only supported when domain on Static Allocation,
that is, "xen,static-mem" is also necessary in the domain configuration.

Looking into related [design link](
https://lists.xenproject.org/archives/html/xen-devel/2021-05/msg00882.html)
for more details.

The whole design is about Static Allocation and direct-map, and this
Patch Serie only covers parts of it, which are direct-map memory map.
Other features will be delievered through different patch series.

See https://lists.xenproject.org/archives/html/xen-devel/2021-09/msg00855.html
for Domain on Static Allocation.

This patch serie is based on
https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00822.html
---
v3 changes:
- move flag XEN_DOMCTL_CDF_INTERNAL_directmap back to xen/include/xen/domain.h,
to let it be only available for domain created by XEN.
- name it with extra "INTERNAL" and add comments to warn developers not
to accidently use its bitfield when introducing new XEN_DOMCTL_CDF_xxx flag.
- reject this flag in x86'es arch_sanitise_domain_config()
- add ASSERT_UNREACHABLE to catch any misuse in allocate_static_memory()
and allocate_static_memory_11()
- add another check of validating flag XEN_DOMCTL_CDF_INTERNAL_directmap only
when CONFIG_STATIC_MEMORY is set.
- simply map the CPU interface at the GPA vgic_v2_hw.cbase
- drop 'cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS)'
- rename 'is_domain_use_host_layout()' to 'domain_use_host_layout()'
---
v2 changes:
- remove the introduce of internal flag
- Refine is_domain_direct_mapped to check whether the flag
XEN_DOMCTL_CDF_directmap is set
- reword "1:1 direct-map" to just "direct-map"
- split the common codes into two helpers: parse_static_mem_prop and
acquire_static_memory_bank to deduce complexity.
- introduce a new helper allocate_static_memory_11 for allocating static
memory for direct-map guests
- remove panic action since it is fine to assign a non-DMA capable device when
IOMMU and direct-map both off
- remove redistributor accessor
- introduce new helper "is_domain_use_host_layout()"
- explain why vpl011 initialization before creating its device tree node
- error out if the domain is direct-mapped and the IRQ is not found
- harden the code and add a check/comment when the hardware UART region
is smaller than CUEST_VPL011_SIZE.

Penny Zheng (4):
  xen/arm: introduce new helper parse_static_mem_prop and ...
  xen/arm: introduce direct-map for domUs
  xen/arm: add ASSERT_UNREACHABLE in allocate_static_memory
  xen/arm: gate make_gicv3_domU_node with CONFIG_GICV3

Stefano Stabellini (6):
  xen: introduce XEN_DOMCTL_CDF_INTERNAL_directmap
  xen/arm: avoid setting XEN_DOMCTL_CDF_iommu when IOMMU off
  xen/arm: if direct-map domain use native addresses for GICv2
  xen/arm: if direct-map domain use native addresses for GICv3
  xen/arm: if direct-map domain use native UART address and IRQ ...
  xen/docs: Document how to do passthrough without IOMMU

 docs/misc/arm/device-tree/booting.txt |   6 +
 docs/misc/arm/passthrough-noiommu.txt |  52 +++++
 xen/arch/arm/domain.c                 |   3 +-
 xen/arch/arm/domain_build.c           | 310 +++++++++++++++++++++-----
 xen/arch/arm/vgic-v2.c                |  31 ++-
 xen/arch/arm/vgic-v3.c                |  29 ++-
 xen/arch/arm/vgic/vgic-v2.c           |  31 ++-
 xen/arch/arm/vpl011.c                 |  60 ++++-
 xen/arch/x86/domain.c                 |   6 +
 xen/common/domain.c                   |   3 +-
 xen/include/asm-arm/domain.h          |  11 +-
 xen/include/asm-arm/new_vgic.h        |  10 +
 xen/include/asm-arm/vgic.h            |  11 +
 xen/include/asm-arm/vpl011.h          |   2 +
 xen/include/public/domctl.h           |   4 +
 xen/include/xen/domain.h              |   3 +
 16 files changed, 471 insertions(+), 101 deletions(-)
 create mode 100644 docs/misc/arm/passthrough-noiommu.txt

-- 
2.25.1

[PATCH v3 01/10] xen: introduce XEN_DOMCTL_CDF_INTERNAL_directmap

[Penny Zheng]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

This commit introduces a new arm-specific flag
XEN_DOMCTL_CDF_INTERNAL_directmap to specify that a domain should
have its memory direct-map(guest physical address == physical address)
at domain creation.

Since this flag is only available for domain created by XEN, not exposed
to the toolstack, we name it with extra "INTERNAL" to distinguish from
other public XEN_DOMCTL_CDF_xxx flags, and add comments to warn developers
not to accidently use its bitfield when introducing new
XEN_DOMCTL_CDF_xxx flag.

Refine is_domain_direct_mapped to check whether the flag
XEN_DOMCTL_CDF_INTERNAL_directmap is set.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
CC: andrew.cooper3@citrix.com
CC: jbeulich@suse.com
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wl@xen.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
---
v2 changes
- remove the introduce of internal flag
- remove flag direct_map since we already store this flag in d->options
- Refine is_domain_direct_mapped to check whether the flag
XEN_DOMCTL_CDF_directmap is set
- reword "1:1 direct-map" to just "direct-map"
---
v3 changes
- move flag back to xen/include/xen/domain.h, to let it be only available for
domain created by XEN.
- name it with extra "INTERNAL" and add comments to warn developers not
to accidently use its bitfield when introducing new XEN_DOMCTL_CDF_xxx flag.
- reject this flag in x86'es arch_sanitise_domain_config()
---
 xen/arch/arm/domain.c        | 3 ++-
 xen/arch/arm/domain_build.c  | 4 +++-
 xen/arch/x86/domain.c        | 6 ++++++
 xen/common/domain.c          | 3 ++-
 xen/include/asm-arm/domain.h | 4 ++--
 xen/include/public/domctl.h  | 4 ++++
 xen/include/xen/domain.h     | 3 +++
 7 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 96e1b23550..d77265c03f 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -629,7 +629,8 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
 {
     unsigned int max_vcpus;
     unsigned int flags_required = (XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap);
-    unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu);
+    unsigned int flags_optional = (XEN_DOMCTL_CDF_iommu | XEN_DOMCTL_CDF_vpmu |
+                                   XEN_DOMCTL_CDF_INTERNAL_directmap);
 
     if ( (config->flags & ~flags_optional) != flags_required )
     {
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 19487c79da..664c88ebe4 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3089,8 +3089,10 @@ static int __init construct_dom0(struct domain *d)
 void __init create_dom0(void)
 {
     struct domain *dom0;
+    /* DOM0 has always its memory direct-map. */
     struct xen_domctl_createdomain dom0_cfg = {
-        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
+        .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
+                 XEN_DOMCTL_CDF_INTERNAL_directmap,
         .max_evtchn_port = -1,
         .max_grant_frames = gnttab_dom0_frames(),
         .max_maptrack_frames = -1,
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ef1812dc14..eba6502218 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -692,6 +692,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
         return -EINVAL;
     }
 
+    if ( config->flags & XEN_DOMCTL_CDF_INTERNAL_directmap )
+    {
+        dprintk(XENLOG_INFO, "direct-map cannot be enabled yet\n");
+        return -EINVAL;
+    }
+
     return 0;
 }
 
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 56d47dd664..13ac5950bc 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -486,7 +486,8 @@ static int sanitise_domain_config(struct xen_domctl_createdomain *config)
          ~(XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap |
            XEN_DOMCTL_CDF_s3_integrity | XEN_DOMCTL_CDF_oos_off |
            XEN_DOMCTL_CDF_xs_domain | XEN_DOMCTL_CDF_iommu |
-           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpmu) )
+           XEN_DOMCTL_CDF_nested_virt | XEN_DOMCTL_CDF_vpmu |
+           XEN_DOMCTL_CDF_INTERNAL_directmap) )
     {
         dprintk(XENLOG_INFO, "Unknown CDF flags %#x\n", config->flags);
         return -EINVAL;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 9b3647587a..4f2c3f09d4 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -29,8 +29,8 @@ enum domain_type {
 #define is_64bit_domain(d) (0)
 #endif
 
-/* The hardware domain has always its memory direct mapped. */
-#define is_domain_direct_mapped(d) is_hardware_domain(d)
+#define is_domain_direct_mapped(d) \
+        (d->options & XEN_DOMCTL_CDF_INTERNAL_directmap)
 
 struct vtimer {
     struct vcpu *v;
diff --git a/xen/include/public/domctl.h b/xen/include/public/domctl.h
index 1c21d4dc75..054e545c97 100644
--- a/xen/include/public/domctl.h
+++ b/xen/include/public/domctl.h
@@ -72,6 +72,10 @@ struct xen_domctl_createdomain {
 #define XEN_DOMCTL_CDF_nested_virt    (1U << _XEN_DOMCTL_CDF_nested_virt)
 /* Should we expose the vPMU to the guest? */
 #define XEN_DOMCTL_CDF_vpmu           (1U << 7)
+/*
+ * Be aware that bit 8 has already been occupied by flag
+ * XEN_DOMCTL_CDF_INTERNAL_directmap, defined in xen/include/xen/domain.h.
+ */
 
 /* Max XEN_DOMCTL_CDF_* constant.  Used for ABI checking. */
 #define XEN_DOMCTL_CDF_MAX XEN_DOMCTL_CDF_vpmu
diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h
index 160c8dbdab..2b9edfdcee 100644
--- a/xen/include/xen/domain.h
+++ b/xen/include/xen/domain.h
@@ -28,6 +28,9 @@ void getdomaininfo(struct domain *d, struct xen_domctl_getdomaininfo *info);
 void arch_get_domain_info(const struct domain *d,
                           struct xen_domctl_getdomaininfo *info);
 
+/* Should domain memory be directly mapped? */
+#define XEN_DOMCTL_CDF_INTERNAL_directmap      (1U << 8)
+
 /*
  * Arch-specifics.
  */
-- 
2.25.1

[PATCH v3 02/10] xen/arm: avoid setting XEN_DOMCTL_CDF_iommu when IOMMU off

[Penny Zheng]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

This commit avoids setting XEN_DOMCTL_CDF_iommu when the IOMMU is
absent/disabled, otherwise xen will fail later when handling
device assignment.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
v3 changes:
- new commit, split from the original "[PATCH v2 2/6] xen/arm: introduce
direct-map for domUs"
---
 xen/arch/arm/domain_build.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 664c88ebe4..7a063f62fe 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2996,7 +2996,8 @@ void __init create_domUs(void)
             panic("Missing property 'cpus' for domain %s\n",
                   dt_node_name(node));
 
-        if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
+        if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
+             iommu_enabled )
             d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
 
         if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
-- 
2.25.1

[PATCH v3 03/10] xen/arm: introduce new helper parse_static_mem_prop and acquire_static_memory_bank

[Penny Zheng]

Later, we will introduce allocate_static_memory_11 for allocating static
memory for direct-map domains, and it will share a lot common codes with
the existing allocate_static_memory.

In order not to bring a lot of duplicate codes, and also to make the whole
code more readable, this commit extracts common codes into two new helpers
parse_static_mem_prop and acquire_static_memory_bank.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v3 changes:
- new commit to move the split off of the code outside in a separate patch
---
 xen/arch/arm/domain_build.c | 100 +++++++++++++++++++++++-------------
 1 file changed, 64 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 7a063f62fe..1dc728e848 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -515,12 +515,69 @@ static bool __init append_static_memory_to_bank(struct domain *d,
     return true;
 }
 
+static mfn_t __init acquire_static_memory_bank(struct domain *d,
+                                             const __be32 **cell,
+                                             u32 addr_cells, u32 size_cells,
+                                             paddr_t *pbase, paddr_t *psize)
+{
+    mfn_t smfn;
+    int res;
+
+    device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
+    ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
+    if ( PFN_DOWN(*psize) > UINT_MAX )
+    {
+        printk(XENLOG_ERR "%pd: static memory size too large: %#"PRIpaddr,
+               d, *psize);
+        return INVALID_MFN;
+    }
+
+    smfn = maddr_to_mfn(*pbase);
+    res = acquire_domstatic_pages(d, smfn, PFN_DOWN(*psize), 0);
+    if ( res )
+    {
+        printk(XENLOG_ERR
+               "%pd: failed to acquire static memory: %d.\n", d, res);
+        return INVALID_MFN;
+    }
+
+    return smfn;
+}
+
+static int __init parse_static_mem_prop(const struct dt_device_node *node,
+                                        u32 *addr_cells, u32 *size_cells,
+                                        int *length, const __be32 **cell)
+{
+    const struct dt_property *prop;
+
+    prop = dt_find_property(node, "xen,static-mem", NULL);
+    if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
+                               addr_cells) )
+    {
+        printk(XENLOG_ERR
+               "failed to read \"#xen,static-mem-address-cells\".\n");
+        return -EINVAL;
+    }
+
+    if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
+                               size_cells) )
+    {
+        printk(XENLOG_ERR
+               "failed to read \"#xen,static-mem-size-cells\".\n");
+        return -EINVAL;
+    }
+
+    *cell = (const __be32 *)prop->value;
+    *length = prop->length;
+
+    return 0;
+}
+
 /* Allocate memory from static memory as RAM for one specific domain d. */
 static void __init allocate_static_memory(struct domain *d,
                                           struct kernel_info *kinfo,
                                           const struct dt_device_node *node)
 {
-    const struct dt_property *prop;
     u32 addr_cells, size_cells, reg_cells;
     unsigned int nr_banks, gbank, bank = 0;
     const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
@@ -529,24 +586,10 @@ static void __init allocate_static_memory(struct domain *d,
     u64 tot_size = 0;
     paddr_t pbase, psize, gsize;
     mfn_t smfn;
-    int res;
-
-    prop = dt_find_property(node, "xen,static-mem", NULL);
-    if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
-                               &addr_cells) )
-    {
-        printk(XENLOG_ERR
-               "%pd: failed to read \"#xen,static-mem-address-cells\".\n", d);
-        goto fail;
-    }
+    int length;
 
-    if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
-                               &size_cells) )
-    {
-        printk(XENLOG_ERR
-               "%pd: failed to read \"#xen,static-mem-size-cells\".\n", d);
+    if ( parse_static_mem_prop(node, &addr_cells, &size_cells, &length, &cell) )
         goto fail;
-    }
     reg_cells = addr_cells + size_cells;
 
     /*
@@ -557,29 +600,14 @@ static void __init allocate_static_memory(struct domain *d,
     gbank = 0;
     gsize = ramsize[gbank];
     kinfo->mem.bank[gbank].start = rambase[gbank];
-
-    cell = (const __be32 *)prop->value;
-    nr_banks = (prop->length) / (reg_cells * sizeof (u32));
+    nr_banks = length / (reg_cells * sizeof (u32));
 
     for ( ; bank < nr_banks; bank++ )
     {
-        device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
-        ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
-
-        if ( PFN_DOWN(psize) > UINT_MAX )
-        {
-            printk(XENLOG_ERR "%pd: static memory size too large: %#"PRIpaddr,
-                   d, psize);
+        smfn = acquire_static_memory_bank(d, &cell, addr_cells, size_cells,
+                                          &pbase, &psize);
+        if ( !mfn_valid(smfn) )
             goto fail;
-        }
-        smfn = maddr_to_mfn(pbase);
-        res = acquire_domstatic_pages(d, smfn, PFN_DOWN(psize), 0);
-        if ( res )
-        {
-            printk(XENLOG_ERR
-                   "%pd: failed to acquire static memory: %d.\n", d, res);
-            goto fail;
-        }
 
         printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n",
                d, bank, pbase, pbase + psize);
-- 
2.25.1

[PATCH v3 04/10] xen/arm: introduce direct-map for domUs

[Penny Zheng]

Cases where domU needs direct-map memory map:
  * IOMMU not present in the system.
  * IOMMU disabled if it doesn't cover a specific device and all the guests
are trusted. Thinking a mixed scenario, where a few devices with IOMMU and
a few without, then guest DMA security still could not be totally guaranteed.
So users may want to disable the IOMMU, to at least gain some performance
improvement from IOMMU disabled.
  * IOMMU disabled as a workaround when it doesn't have enough bandwidth.
To be specific, in a few extreme situation, when multiple devices do DMA
concurrently, these requests may exceed IOMMU's transmission capacity.
  * IOMMU disabled when it adds too much latency on DMA. For example,
TLB may be missing in some IOMMU hardware, which may bring latency in DMA
progress, so users may want to disable it in some realtime scenario.
  * Guest OS relies on the host memory layout

This commit introduces a new helper allocate_static_memory_11 to allocate
static memory as guest RAM for direct-map domain.

For now, direct-map is only available when statically allocated memory is
used for the domain, that is, "xen,static-mem" must be also defined in the
domain configuration.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
v2 changes:
- split the common codes into two helpers: parse_static_mem_prop and
acquire_static_memory_bank to deduce complexity.
- introduce a new helper allocate_static_memory_11 for allocating static
memory for direct-map guests
- remove redundant use "bool direct_map", to be replaced by
d_cfg.flags & XEN_DOMCTL_CDF_directmap
- remove panic action since it is fine to assign a non-DMA capable device when
IOMMU and direct-map both off
---
v3 changes:
- doc refinement
- drop the pointless gbank
- add check of the size of nr_banks shall not exceed NR_MEM_BANKS
- add ASSERT_UNREACHABLE to catch any misuse
- add another check of validating flag XEN_DOMCTL_CDF_INTERNAL_directmap only
when CONFIG_STATIC_MEMORY is set.
---
 docs/misc/arm/device-tree/booting.txt |   6 ++
 xen/arch/arm/domain_build.c           | 105 +++++++++++++++++++++++++-
 2 files changed, 108 insertions(+), 3 deletions(-)

diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
index 71895663a4..a94125394e 100644
--- a/docs/misc/arm/device-tree/booting.txt
+++ b/docs/misc/arm/device-tree/booting.txt
@@ -182,6 +182,12 @@ with the following properties:
     Both #address-cells and #size-cells need to be specified because
     both sub-nodes (described shortly) have reg properties.
 
+- direct-map
+
+    Only available when statically allocated memory is used for the domain.
+    An empty property to request the memory of the domain to be
+    direct-map (guest physical address == physical address).
+
 Under the "xen,domain" compatible node, one or more sub-nodes are present
 for the DomU kernel and ramdisk.
 
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 1dc728e848..97a5b5dedd 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -500,8 +500,13 @@ static bool __init append_static_memory_to_bank(struct domain *d,
 {
     int res;
     unsigned int nr_pages = PFN_DOWN(size);
-    /* Infer next GFN. */
-    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
+    gfn_t sgfn;
+
+    if ( !is_domain_direct_mapped(d) )
+        /* Infer next GFN when GFN != MFN. */
+        sgfn = gaddr_to_gfn(bank->start + bank->size);
+    else
+        sgfn = gaddr_to_gfn(mfn_to_maddr(smfn));
 
     res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
     if ( res )
@@ -674,12 +679,94 @@ static void __init allocate_static_memory(struct domain *d,
  fail:
     panic("Failed to allocate requested static memory for domain %pd.", d);
 }
+
+/*
+ * Allocate static memory as RAM for one specific domain d.
+ * The static memory will be directly mapped in the guest(Guest Physical
+ * Address == Physical Address).
+ */
+static void __init allocate_static_memory_11(struct domain *d,
+                                             struct kernel_info *kinfo,
+                                             const struct dt_device_node *node)
+{
+    u32 addr_cells, size_cells, reg_cells;
+    unsigned int nr_banks, bank = 0;
+    const __be32 *cell;
+    u64 tot_size = 0;
+    paddr_t pbase, psize;
+    mfn_t smfn;
+    int length;
+
+    if ( parse_static_mem_prop(node, &addr_cells, &size_cells, &length, &cell) )
+    {
+        printk(XENLOG_ERR
+               "%pd: failed to parse \"xen,static-mem\" property.\n", d);
+        goto fail;
+    }
+    reg_cells = addr_cells + size_cells;
+    nr_banks = length / (reg_cells * sizeof (u32));
+
+    if ( nr_banks > NR_MEM_BANKS )
+    {
+        printk(XENLOG_ERR
+               "%pd: exceed max number of supported guest memory banks.\n", d);
+        goto fail;
+    }
+
+    for ( ; bank < nr_banks; bank++ )
+    {
+        smfn = acquire_static_memory_bank(d, &cell, addr_cells, size_cells,
+                                          &pbase, &psize);
+        if ( !mfn_valid(smfn) )
+            goto fail;
+
+        printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n",
+               d, bank, pbase, pbase + psize);
+
+        /* One guest memory bank is matched with one physical memory bank. */
+        kinfo->mem.bank[bank].start = pbase;
+        if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[bank],
+                                           smfn, psize) )
+            goto fail;
+
+        tot_size += psize;
+    }
+
+    kinfo->mem.nr_banks = nr_banks;
+
+    kinfo->unassigned_mem -= tot_size;
+    /*
+     * The property 'memory' should match the amount of memory given to the
+     * guest.
+     * Currently, it is only possible to either acquire static memory or let
+     * Xen allocate. *Mixing* is not supported.
+     */
+    if ( kinfo->unassigned_mem )
+    {
+        printk(XENLOG_ERR
+               "Size of \"memory\" property doesn't match up with the sum-up of \"xen,static-mem\". Unsupported configuration.\n");
+        goto fail;
+    }
+
+    return;
+
+ fail:
+    panic("Failed to allocate requested static memory for direct-map domain %pd.",
+          d);
+}
 #else
 static void __init allocate_static_memory(struct domain *d,
                                           struct kernel_info *kinfo,
                                           const struct dt_device_node *node)
 {
 }
+
+static void __init allocate_static_memory_11(struct domain *d,
+                                             struct kernel_info *kinfo,
+                                             const struct dt_device_node *node)
+{
+    ASSERT_UNREACHABLE();
+}
 #endif
 
 static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
@@ -2983,7 +3070,12 @@ static int __init construct_domU(struct domain *d,
     if ( !dt_find_property(node, "xen,static-mem", NULL) )
         allocate_memory(d, &kinfo);
     else
-        allocate_static_memory(d, &kinfo, node);
+    {
+        if ( is_domain_direct_mapped(d) )
+            allocate_static_memory_11(d, &kinfo, node);
+        else
+            allocate_static_memory(d, &kinfo, node);
+    }
 
     rc = prepare_dtb_domU(d, &kinfo);
     if ( rc < 0 )
@@ -3024,6 +3116,13 @@ void __init create_domUs(void)
             panic("Missing property 'cpus' for domain %s\n",
                   dt_node_name(node));
 
+        if ( dt_property_read_bool(node, "direct-map") )
+        {
+            if ( !IS_ENABLED(CONFIG_STATIC_MEMORY) )
+                panic("direct-map not valid without CONFIG_STATIC_MEMORY\n");
+            d_cfg.flags |= XEN_DOMCTL_CDF_INTERNAL_directmap;
+        }
+
         if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
              iommu_enabled )
             d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
-- 
2.25.1

[PATCH v3 05/10] xen/arm: add ASSERT_UNREACHABLE in allocate_static_memory

[Penny Zheng]

Helper allocate_static_memory is not meant to be reachable when built with
!CONFIG_STATIC_MEMORY, so this commit adds ASSERT_UNREACHABLE in it to catch
potential misuse.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v3 changes:
- new commit
---
 xen/arch/arm/domain_build.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 97a5b5dedd..b6fde74d74 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -759,6 +759,7 @@ static void __init allocate_static_memory(struct domain *d,
                                           struct kernel_info *kinfo,
                                           const struct dt_device_node *node)
 {
+    ASSERT_UNREACHABLE();
 }
 
 static void __init allocate_static_memory_11(struct domain *d,
-- 
2.25.1

[PATCH v3 06/10] xen/arm: if direct-map domain use native addresses for GICv2

[Penny Zheng]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

Today we use native addresses to map the GICv2 for Dom0 and fixed
addresses for DomUs.

This patch changes the behavior so that native addresses are used for
all domains that are direct-mapped.

NEW VGIC has different naming schemes, like referring distributor base
address as vgic_dist_base, other than the dbase. So this patch also introduces
vgic_dist_base/vgic_cpu_base accessor to access correct distributor base
address/cpu interface base address on varied scenarios,

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
v2 changes
- combine all changes in patch 4-6 here
---
v3 changes
- refine comment message
- add a comment explaining how the 38 was found of "char buf[38]"
- simply map the CPU interface at the GPA vgic_v2_hw.cbase
- remove a spurious change
---
 xen/arch/arm/domain_build.c    | 11 ++++++++---
 xen/arch/arm/vgic-v2.c         | 31 ++++++++++++++++++++++---------
 xen/arch/arm/vgic/vgic-v2.c    | 31 ++++++++++++++++++++++---------
 xen/include/asm-arm/new_vgic.h | 10 ++++++++++
 xen/include/asm-arm/vgic.h     | 11 +++++++++++
 5 files changed, 73 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b6fde74d74..c419a4b2cc 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2230,8 +2230,13 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
     int res = 0;
     __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
     __be32 *cells;
+    const struct domain *d = kinfo->d;
+    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
+    char buf[38];
 
-    res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICD_BASE));
+    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+             vgic_dist_base(&d->arch.vgic));
+    res = fdt_begin_node(fdt, buf);
     if ( res )
         return res;
 
@@ -2253,9 +2258,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
 
     cells = &reg[0];
     dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
-                       GUEST_GICD_BASE, GUEST_GICD_SIZE);
+                       vgic_dist_base(&d->arch.vgic), GUEST_GICD_SIZE);
     dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
-                       GUEST_GICC_BASE, GUEST_GICC_SIZE);
+                       vgic_cpu_base(&d->arch.vgic), GUEST_GICC_SIZE);
 
     res = fdt_property(fdt, "reg", reg, sizeof(reg));
     if (res)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index 589c033eda..6f5492e30e 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -654,13 +654,10 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
 static int vgic_v2_domain_init(struct domain *d)
 {
     int ret;
-    paddr_t cbase, csize;
+    paddr_t csize;
     paddr_t vbase;
 
-    /*
-     * The hardware domain gets the hardware address.
-     * Guests get the virtual platform layout.
-     */
+    /* The hardware domain gets the hardware address. */
     if ( is_hardware_domain(d) )
     {
         d->arch.vgic.dbase = vgic_v2_hw.dbase;
@@ -671,10 +668,26 @@ static int vgic_v2_domain_init(struct domain *d)
          * Note that we assume the size of the CPU interface is always
          * aligned to PAGE_SIZE.
          */
-        cbase = vgic_v2_hw.cbase;
+        d->arch.vgic.cbase = vgic_v2_hw.cbase;
         csize = vgic_v2_hw.csize;
         vbase = vgic_v2_hw.vbase;
     }
+    else if ( is_domain_direct_mapped(d) )
+    {
+        /*
+         * 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 occupied by the physical GIC in the host device tree.
+         *
+         * We need to add an offset to the virtual CPU interface base
+         * address when the GIC is aliased to get a 8kB contiguous
+         * region.
+         */
+        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;
+    }
     else
     {
         d->arch.vgic.dbase = GUEST_GICD_BASE;
@@ -685,7 +698,7 @@ static int vgic_v2_domain_init(struct domain *d)
          * region.
          */
         BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
-        cbase = GUEST_GICC_BASE;
+        d->arch.vgic.cbase = GUEST_GICC_BASE;
         csize = GUEST_GICC_SIZE;
         vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
     }
@@ -694,8 +707,8 @@ static int vgic_v2_domain_init(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(cbase), csize / PAGE_SIZE,
-                           maddr_to_mfn(vbase));
+    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
+                           csize / PAGE_SIZE, maddr_to_mfn(vbase));
     if ( ret )
         return ret;
 
diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index b5ba4ace87..63d0f03688 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -258,14 +258,11 @@ void vgic_v2_enable(struct vcpu *vcpu)
 int vgic_v2_map_resources(struct domain *d)
 {
     struct vgic_dist *dist = &d->arch.vgic;
-    paddr_t cbase, csize;
+    paddr_t csize;
     paddr_t vbase;
     int ret;
 
-    /*
-     * The hardware domain gets the hardware address.
-     * Guests get the virtual platform layout.
-     */
+    /* The hardware domain gets the hardware address. */
     if ( is_hardware_domain(d) )
     {
         d->arch.vgic.vgic_dist_base = gic_v2_hw_data.dbase;
@@ -276,10 +273,26 @@ int vgic_v2_map_resources(struct domain *d)
          * Note that we assume the size of the CPU interface is always
          * aligned to PAGE_SIZE.
          */
-        cbase = gic_v2_hw_data.cbase;
+        d->arch.vgic.vgic_cpu_base = 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;
+        /*
+         * 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
+         * occupied by the physical GIC in the host device tree.
+         *
+         * We need to add an offset to the virtual CPU interface base
+         * address when the GIC is aliased to get a 8kB contiguous
+         * region.
+         */
+        d->arch.vgic.vgic_cpu_base = 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;
@@ -290,7 +303,7 @@ int vgic_v2_map_resources(struct domain *d)
          * region.
          */
         BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
-        cbase = GUEST_GICC_BASE;
+        d->arch.vgic.vgic_cpu_base = GUEST_GICC_BASE;
         csize = GUEST_GICC_SIZE;
         vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
     }
@@ -308,8 +321,8 @@ 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(cbase), csize / PAGE_SIZE,
-                           maddr_to_mfn(vbase));
+    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.vgic_cpu_base),
+                           csize / PAGE_SIZE, maddr_to_mfn(vbase));
     if ( ret )
     {
         gdprintk(XENLOG_ERR, "Unable to remap VGIC CPU to VCPU\n");
diff --git a/xen/include/asm-arm/new_vgic.h b/xen/include/asm-arm/new_vgic.h
index 97d622bff6..ab57fcd91d 100644
--- a/xen/include/asm-arm/new_vgic.h
+++ b/xen/include/asm-arm/new_vgic.h
@@ -186,6 +186,16 @@ struct vgic_cpu {
     uint32_t num_id_bits;
 };
 
+static inline paddr_t vgic_cpu_base(const struct vgic_dist *vgic)
+{
+    return vgic->vgic_cpu_base;
+}
+
+static inline paddr_t vgic_dist_base(const struct vgic_dist *vgic)
+{
+    return vgic->vgic_dist_base;
+}
+
 #endif /* __ASM_ARM_NEW_VGIC_H */
 
 /*
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index e69a59063a..a81a06c711 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -152,6 +152,7 @@ struct vgic_dist {
     struct pending_irq *pending_irqs;
     /* Base address for guest GIC */
     paddr_t dbase; /* Distributor base address */
+    paddr_t cbase; /* CPU interface base address */
 #ifdef CONFIG_GICV3
     /* GIC V3 addressing */
     /* List of contiguous occupied by the redistributors */
@@ -271,6 +272,16 @@ static inline int REG_RANK_NR(int b, uint32_t n)
 
 enum gic_sgi_mode;
 
+static inline paddr_t vgic_cpu_base(const struct vgic_dist *vgic)
+{
+    return vgic->cbase;
+}
+
+static inline paddr_t vgic_dist_base(const struct vgic_dist *vgic)
+{
+    return vgic->dbase;
+}
+
 /*
  * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> size <s> with
  * <b>-bits-per-interrupt.
-- 
2.25.1

[PATCH v3 07/10] xen/arm: gate make_gicv3_domU_node with CONFIG_GICV3

[Penny Zheng]

This commit gates function make_gicv3_domU_node with CONFIG_GICV3, and
also adds ASSERT_UNREACHABLE to catch any misuse.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
v3 changes:
- new commit
---
 xen/arch/arm/domain_build.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c419a4b2cc..24f3edf069 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2279,6 +2279,7 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
     return res;
 }
 
+#ifdef CONFIG_GICV3
 static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
 {
     void *fdt = kinfo->fdt;
@@ -2328,6 +2329,12 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
 
     return res;
 }
+#else
+static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
+{
+    ASSERT_UNREACHABLE();
+}
+#endif
 
 static int __init make_gic_domU_node(struct kernel_info *kinfo)
 {
-- 
2.25.1

[PATCH v3 08/10] xen/arm: if direct-map domain use native addresses for GICv3

[Penny Zheng]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

Today we use native addresses to map the GICv3 for Dom0 and fixed
addresses for DomUs.

This patch changes the behavior so that native addresses are used for
all domain which is using the host memory layout

Considering that DOM0 may not always be directly mapped in the future,
this patch introduces a new helper "domain_use_host_layout()" that
wraps both two check "is_domain_direct_mapped(d) || is_hardware_domain(d)"
for more flexible usage.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
v2 changes:
- remove redistributor accessor
- introduce new helper "is_domain_use_host_layout()"
- comment fix
---
v3 changes:
- the comment on top of 'buf' to explain how 38 was found
- fix res getting overwritten
- drop 'cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS)'
- free 'reg' right way
- fix comment
- rename 'is_domain_use_host_layout()' to 'domain_use_host_layout()'
---
 xen/arch/arm/domain_build.c  | 37 +++++++++++++++++++++++++++---------
 xen/arch/arm/vgic-v3.c       | 29 ++++++++++++++++------------
 xen/include/asm-arm/domain.h |  7 +++++++
 3 files changed, 52 insertions(+), 21 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 24f3edf069..61fd374c5d 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2284,10 +2284,16 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
 {
     void *fdt = kinfo->fdt;
     int res = 0;
-    __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
-    __be32 *cells;
+    __be32 *reg;
+    const struct domain *d = kinfo->d;
+    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
+    char buf[38];
+    unsigned int i, len = 0;
 
-    res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE));
+    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
+             vgic_dist_base(&d->arch.vgic));
+
+    res = fdt_begin_node(fdt, buf);
     if ( res )
         return res;
 
@@ -2307,13 +2313,26 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
     if ( res )
         return res;
 
-    cells = &reg[0];
-    dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
-                       GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE);
-    dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
-                       GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE);
+    /* reg specifies all re-distributors and Distributor. */
+    len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
+          (d->arch.vgic.nr_regions + 1) * sizeof(__be32);
+    reg = xmalloc_bytes(len);
+    if ( reg == NULL )
+        return -ENOMEM;
 
-    res = fdt_property(fdt, "reg", reg, sizeof(reg));
+    dt_child_set_range(&reg, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                       vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE);
+
+    for ( i = 0; i < d->arch.vgic.nr_regions; i++)
+    {
+        dt_child_set_range(&reg,
+                           GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
+                           d->arch.vgic.rdist_regions[i].base,
+                           d->arch.vgic.rdist_regions[i].size);
+    }
+
+    res = fdt_property(fdt, "reg", reg, len);
+    xfree(reg);
     if (res)
         return res;
 
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 65bb7991a6..181b66513d 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -1640,14 +1640,15 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
      * Normally there is only one GICv3 redistributor region.
      * The GICv3 DT binding provisions for multiple regions, since there are
      * platforms out there which need those (multi-socket systems).
-     * For Dom0 we have to live with the MMIO layout the hardware provides,
-     * so we have to copy the multiple regions - as the first region may not
-     * provide enough space to hold all redistributors we need.
+     * For domain using the host memory layout, we have to live with the MMIO
+     * layout the hardware provides, so we have to copy the multiple regions
+     * - as the first region may not provide enough space to hold all
+     * redistributors we need.
      * However DomU get a constructed memory map, so we can go with
      * the architected single redistributor region.
      */
-    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
-               GUEST_GICV3_RDIST_REGIONS;
+    return domain_use_host_layout(d) ? vgic_v3_hw.nr_rdist_regions :
+                                       GUEST_GICV3_RDIST_REGIONS;
 }
 
 static int vgic_v3_domain_init(struct domain *d)
@@ -1669,10 +1670,14 @@ static int vgic_v3_domain_init(struct domain *d)
     radix_tree_init(&d->arch.vgic.pend_lpi_tree);
 
     /*
-     * Domain 0 gets the hardware address.
-     * Guests get the virtual platform layout.
+     * Since we map the whole GICv3 register memory map(64KB) for
+     * all domain, DOM0 and direct-map domain could be treated the
+     * same way here.
+     * For domain using the host memory layout, it gets the hardware
+     * address.
+     * Other domains get the virtual platform layout.
      */
-    if ( is_hardware_domain(d) )
+    if ( domain_use_host_layout(d) )
     {
         unsigned int first_cpu = 0;
 
@@ -1695,10 +1700,10 @@ static int vgic_v3_domain_init(struct domain *d)
         }
 
         /*
-         * The hardware domain may not use all the re-distributors
-         * regions (e.g when the number of vCPUs does not match the
-         * number of pCPUs). Update the number of regions to avoid
-         * exposing unused region as they will not get emulated.
+         * For domain using the host memory layout, it may not use all
+         * the re-distributors regions (e.g when the number of vCPUs does
+         * not match the number of pCPUs). Update the number of regions to
+         * avoid exposing unused region as they will not get emulated.
          */
         d->arch.vgic.nr_regions = i + 1;
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 4f2c3f09d4..0eff93197e 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -32,6 +32,13 @@ enum domain_type {
 #define is_domain_direct_mapped(d) \
         (d->options & XEN_DOMCTL_CDF_INTERNAL_directmap)
 
+/*
+ * For domain using the host memory layout, we have to live with the MMIO
+ * layout the hardware provides.
+ */
+#define domain_use_host_layout(d) (is_domain_direct_mapped(d) || \
+                                   is_hardware_domain(d))
+
 struct vtimer {
     struct vcpu *v;
     int irq;
-- 
2.25.1

[PATCH v3 09/10] xen/arm: if direct-map domain use native UART address and IRQ number for vPL011

[Penny Zheng]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

We always use a fix address to map the vPL011 to domains. The address
could be a problem for direct-map domains.

So, for domains that are directly mapped, reuse the address of the
physical UART on the platform to avoid potential clashes.

Do the same for the virtual IRQ number: instead of always using
GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
---
v2 changes:
- explain why vpl011 initialization before creating its device tree node
- error out if the domain is direct-mapped and the IRQ is not found
- harden the code and add a check/comment when the hardware UART region
is smaller than GUEST_VPL011_SIZE.
---
v3 changes:
- explain how the '27' was found for 'buf'
- fix checking before dereferencing
- refine comment message
---
 xen/arch/arm/domain_build.c  | 42 ++++++++++++++++++++-----
 xen/arch/arm/vpl011.c        | 60 +++++++++++++++++++++++++++++++-----
 xen/include/asm-arm/vpl011.h |  2 ++
 3 files changed, 90 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 61fd374c5d..871c7114ae 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -30,6 +30,7 @@
 
 #include <xen/irq.h>
 #include <xen/grant_table.h>
+#include <xen/serial.h>
 
 static unsigned int __initdata opt_dom0_max_vcpus;
 integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
@@ -2376,8 +2377,12 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
     gic_interrupt_t intr;
     __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
     __be32 *cells;
+    struct domain *d = kinfo->d;
+    /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */
+    char buf[27];
 
-    res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE));
+    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr);
+    res = fdt_begin_node(fdt, buf);
     if ( res )
         return res;
 
@@ -2387,14 +2392,14 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
 
     cells = &reg[0];
     dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
-                       GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
+                       GUEST_ROOT_SIZE_CELLS, d->arch.vpl011.base_addr,
                        GUEST_PL011_SIZE);
 
     res = fdt_property(fdt, "reg", reg, sizeof(reg));
     if ( res )
         return res;
 
-    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
+    set_interrupt(intr, d->arch.vpl011.virq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
 
     res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
     if ( res )
@@ -3109,6 +3114,14 @@ static int __init construct_domU(struct domain *d,
             allocate_static_memory(d, &kinfo, node);
     }
 
+    /*
+     * Base address and irq number are needed when creating vpl011 device
+     * tree node in prepare_dtb_domU, so initialization on related variables
+     * shall be done first.
+     */
+    if ( kinfo.vpl011 )
+        rc = domain_vpl011_init(d, NULL);
+
     rc = prepare_dtb_domU(d, &kinfo);
     if ( rc < 0 )
         return rc;
@@ -3117,9 +3130,6 @@ static int __init construct_domU(struct domain *d,
     if ( rc < 0 )
         return rc;
 
-    if ( kinfo.vpl011 )
-        rc = domain_vpl011_init(d, NULL);
-
     return rc;
 }
 
@@ -3161,15 +3171,33 @@ void __init create_domUs(void)
 
         if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
         {
+            unsigned int vpl011_virq = GUEST_VPL011_SPI;
+
             d_cfg.arch.nr_spis = gic_number_lines() - 32;
 
+            /*
+             * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is
+             * set, in which case it'll match the hardware.
+             *
+             * Since here the domain is not totally built, we need to
+             * open-code the logic to find the vIRQ. and the logic here
+             * is consistent with the ones in domain_vpl011_init().
+             */
+            if ( d_cfg.flags & XEN_DOMCTL_CDF_INTERNAL_directmap )
+            {
+                vpl011_virq = serial_irq(SERHND_DTUART);
+                if ( vpl011_virq < 0 )
+                    panic("Error getting IRQ number for this serial port %d\n",
+                          SERHND_DTUART);
+            }
+
             /*
              * vpl011 uses one emulated SPI. If vpl011 is requested, make
              * sure that we allocate enough SPIs for it.
              */
             if ( dt_property_read_bool(node, "vpl011") )
                 d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
-                                         GUEST_VPL011_SPI - 32 + 1);
+                                         vpl011_virq - 32 + 1);
         }
 
         /*
diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
index 895f436cc4..65610bccaf 100644
--- a/xen/arch/arm/vpl011.c
+++ b/xen/arch/arm/vpl011.c
@@ -29,6 +29,7 @@
 #include <xen/mm.h>
 #include <xen/sched.h>
 #include <xen/console.h>
+#include <xen/serial.h>
 #include <public/domctl.h>
 #include <public/io/console.h>
 #include <asm/pl011-uart.h>
@@ -71,11 +72,11 @@ static void vpl011_update_interrupt_status(struct domain *d)
      * status bit has been set since the last time.
      */
     if ( uartmis & ~vpl011->shadow_uartmis )
-        vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
+        vgic_inject_irq(d, NULL, vpl011->virq, true);
 
     vpl011->shadow_uartmis = uartmis;
 #else
-    vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
+    vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
 #endif
 }
 
@@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
                             void *priv)
 {
     struct hsr_dabt dabt = info->dabt;
-    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
+    uint32_t vpl011_reg = (uint32_t)(info->gpa -
+                                     v->domain->arch.vpl011.base_addr);
     struct vpl011 *vpl011 = &v->domain->arch.vpl011;
     struct domain *d = v->domain;
     unsigned long flags;
@@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
                              void *priv)
 {
     struct hsr_dabt dabt = info->dabt;
-    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
+    uint32_t vpl011_reg = (uint32_t)(info->gpa -
+                                     v->domain->arch.vpl011.base_addr);
     struct vpl011 *vpl011 = &v->domain->arch.vpl011;
     struct domain *d = v->domain;
     unsigned long flags;
@@ -626,6 +629,49 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
     if ( vpl011->backend.dom.ring_buf )
         return -EINVAL;
 
+    /*
+     * The VPL011 virq is GUEST_VPL011_SPI, except for direct-map domains
+     * where the hardware value shall be used.
+     * And the logic here should stay in sync with the one in
+     * create_domUs().
+     */
+    if ( is_domain_direct_mapped(d) )
+    {
+        const struct vuart_info *uart = serial_vuart_info(SERHND_DTUART);
+        int vpl011_irq = serial_irq(SERHND_DTUART);
+
+        if ( (uart != NULL) && (vpl011_irq > 0) )
+        {
+            vpl011->base_addr = uart->base_addr;
+            vpl011->virq = vpl011_irq;
+        }
+        else
+        {
+            printk(XENLOG_ERR
+                   "vpl011: Unable to re-use the Xen UART information.\n");
+            return -EINVAL;
+        }
+
+        /*
+         * Since the PL011 we emulate for the guest requires a 4KB region,
+         * and on some Hardware (e.g. on some sunxi SoC), the UART MMIO
+         * region is less than 4KB, in which case, there may exist multiple
+         * devices within the same 4KB region, here adds the following check to
+         * prevent potential known pitfalls
+         */
+        if ( uart->size < GUEST_PL011_SIZE )
+        {
+            printk(XENLOG_ERR
+                   "vpl011: Can't re-use the Xen UART MMIO region as it is too small.\n");
+            return -EINVAL;
+        }
+    }
+    else
+    {
+        vpl011->base_addr = GUEST_PL011_BASE;
+        vpl011->virq = GUEST_VPL011_SPI;
+    }
+
     /*
      * info is NULL when the backend is in Xen.
      * info is != NULL when the backend is in a domain.
@@ -661,7 +707,7 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
         }
     }
 
-    rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
+    rc = vgic_reserve_virq(d, vpl011->virq);
     if ( !rc )
     {
         rc = -EINVAL;
@@ -673,12 +719,12 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
     spin_lock_init(&vpl011->lock);
 
     register_mmio_handler(d, &vpl011_mmio_handler,
-                          GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
+                          vpl011->base_addr, GUEST_PL011_SIZE, NULL);
 
     return 0;
 
 out2:
-    vgic_free_virq(d, GUEST_VPL011_SPI);
+    vgic_free_virq(d, vpl011->virq);
 
 out1:
     if ( vpl011->backend_in_domain )
diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
index e6c7ab7381..c09abcd7a9 100644
--- a/xen/include/asm-arm/vpl011.h
+++ b/xen/include/asm-arm/vpl011.h
@@ -53,6 +53,8 @@ struct vpl011 {
     uint32_t    uarticr;        /* Interrupt clear register */
     uint32_t    uartris;        /* Raw interrupt status register */
     uint32_t    shadow_uartmis; /* shadow masked interrupt register */
+    paddr_t     base_addr;
+    unsigned int virq;
     spinlock_t  lock;
     evtchn_port_t evtchn;
 };
-- 
2.25.1

[PATCH v3 10/10] xen/docs: Document how to do passthrough without IOMMU

[Penny Zheng]

From: Stefano Stabellini <stefano.stabellini@xilinx.com>

This commit creates a new doc to document how to do passthrough without IOMMU.

Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
 docs/misc/arm/passthrough-noiommu.txt | 52 +++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 docs/misc/arm/passthrough-noiommu.txt

diff --git a/docs/misc/arm/passthrough-noiommu.txt b/docs/misc/arm/passthrough-noiommu.txt
new file mode 100644
index 0000000000..3e2ef21ad7
--- /dev/null
+++ b/docs/misc/arm/passthrough-noiommu.txt
@@ -0,0 +1,52 @@
+Request Device Assignment without IOMMU support
+===============================================
+
+*WARNING:
+Users should be aware that it is not always secure to assign a device without
+IOMMU protection.
+When the device is not protected by the IOMMU, the administrator should make
+sure that:
+ 1. The device is assigned to a trusted guest.
+ 2. Users have additional security mechanism on the platform.
+
+This document assumes that the IOMMU is absent from the system or it is
+disabled (status = "disabled" in device tree).
+
+Add xen,force-assign-without-iommu; to the device tree snippet:
+
+ethernet: ethernet@ff0e0000 {
+	compatible = "cdns,zynqmp-gem";
+	xen,path = "/amba/ethernet@ff0e0000";
+	xen,reg = <0x0 0xff0e0000 0x1000 0x0 0xff0e0000>;
+	xen,force-assign-without-iommu;
+};
+
+Request 1:1 memory mapping for the domain on static allocation
+==============================================================
+
+Add a direct-map property under the appropriate /chosen/domU node which
+is also statically allocated with physical memory ranges through
+xen,static-mem property as its guest RAM.
+
+Below is an example on how to specify the 1:1 memory mapping for the domain
+on static allocation in the device-tree:
+
+/ {
+	chosen {
+		domU1 {
+			compatible = "xen,domain";
+			#address-cells = <0x2>;
+			#size-cells = <0x2>;
+			cpus = <2>;
+			memory = <0x0 0x80000>;
+			#xen,static-mem-address-cells = <0x1>;
+			#xen,static-mem-size-cells = <0x1>;
+			xen,static-mem = <0x30000000 0x20000000>;
+			direct-map;
+			...
+		};
+	};
+};
+
+Besides reserving a 512MB region starting at the host physical address
+0x30000000 to DomU1, it also requests 1:1 memory mapping.
-- 
2.25.1

Re: [PATCH v3 00/10] direct-map memory map

[Jan Beulich]

On 16.11.2021 07:31, Penny Zheng wrote:
> v3 changes:
> - move flag XEN_DOMCTL_CDF_INTERNAL_directmap back to xen/include/xen/domain.h,
> to let it be only available for domain created by XEN.
> - name it with extra "INTERNAL" and add comments to warn developers not
> to accidently use its bitfield when introducing new XEN_DOMCTL_CDF_xxx flag.

FTR I continue to object to the hijacking of a public interface bit for
this purpose. Not to the degree of nak-ing the change, but still.

Jan

Re: [PATCH v3 01/10] xen: introduce XEN_DOMCTL_CDF_INTERNAL_directmap

[Jan Beulich]

On 16.11.2021 07:31, Penny Zheng wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -692,6 +692,12 @@ int arch_sanitise_domain_config(struct xen_domctl_createdomain *config)
>          return -EINVAL;
>      }
>  
> +    if ( config->flags & XEN_DOMCTL_CDF_INTERNAL_directmap )
> +    {
> +        dprintk(XENLOG_INFO, "direct-map cannot be enabled yet\n");
> +        return -EINVAL;
> +    }

If this flag is to be retained in its present shape, then besides
rejecting it here (or perhaps instead of, with the check here simply
becoming ASSERT()) you want to reject it in the handling of
XEN_DOMCTL_createdomain, before calling domain_create(). Only then
would the flag become truly internal.

Jan

Re: [PATCH v3 00/10] direct-map memory map

[Julien Grall]

Hi Jan,

On 16/11/2021 07:49, Jan Beulich wrote:
> On 16.11.2021 07:31, Penny Zheng wrote:
>> v3 changes:
>> - move flag XEN_DOMCTL_CDF_INTERNAL_directmap back to xen/include/xen/domain.h,
>> to let it be only available for domain created by XEN.
>> - name it with extra "INTERNAL" and add comments to warn developers not
>> to accidently use its bitfield when introducing new XEN_DOMCTL_CDF_xxx flag.
> 
> FTR I continue to object to the hijacking of a public interface bit for
> this purpose. Not to the degree of nak-ing the change, but still.

I remember this discussion in v1 and I am a bit confused why this was 
re-introduced. Looking at the thread, I think you and I were happy with 
the following approach:

   1) Switch the last parameter of domain_create() (i.e. bool is_priv) 
to flags.
   2) Define a bit in the parameter to indicate whether the domain will 
be direct-mapped.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 00/10] direct-map memory map

[Jan Beulich]

On 16.11.2021 10:44, Julien Grall wrote:
> On 16/11/2021 07:49, Jan Beulich wrote:
>> On 16.11.2021 07:31, Penny Zheng wrote:
>>> v3 changes:
>>> - move flag XEN_DOMCTL_CDF_INTERNAL_directmap back to xen/include/xen/domain.h,
>>> to let it be only available for domain created by XEN.
>>> - name it with extra "INTERNAL" and add comments to warn developers not
>>> to accidently use its bitfield when introducing new XEN_DOMCTL_CDF_xxx flag.
>>
>> FTR I continue to object to the hijacking of a public interface bit for
>> this purpose. Not to the degree of nak-ing the change, but still.
> 
> I remember this discussion in v1 and I am a bit confused why this was 
> re-introduced. Looking at the thread, I think you and I were happy with 
> the following approach:
> 
>    1) Switch the last parameter of domain_create() (i.e. bool is_priv) 
> to flags.
>    2) Define a bit in the parameter to indicate whether the domain will 
> be direct-mapped.

Indeed, that's how I too would prefer this to be dealt with.

Jan

Re: [PATCH v3 00/10] direct-map memory map

[Stefano Stabellini]

On Tue, 16 Nov 2021, Penny Zheng wrote:
> Cases where domU needs direct-map memory map:
>   * IOMMU not present in the system.
>   * IOMMU disabled if it doesn't cover a specific device and all the guests
> are trusted. Thinking a mixed scenario, where a few devices with IOMMU and
> a few without, then guest DMA security still could not be totally guaranteed.
> So users may want to disable the IOMMU, to at least gain some performance
> improvement from IOMMU disabled.
>   * IOMMU disabled as a workaround when it doesn't have enough bandwidth.
> To be specific, in a few extreme situation, when multiple devices do DMA
> concurrently, these requests may exceed IOMMU's transmission capacity.
>   * IOMMU disabled when it adds too much latency on DMA. For example,
> TLB may be missing in some IOMMU hardware, which may bring latency in DMA
> progress, so users may want to disable it in some realtime scenario.
>   * Guest OS relies on the host memory layout
> 
> "direct-map" property shall be added under the appropriate domain node,
> when users requesting direct-map memory mapping for the domain.
> 
> Right now, direct-map is only supported when domain on Static Allocation,
> that is, "xen,static-mem" is also necessary in the domain configuration.
> 
> Looking into related [design link](
> https://lists.xenproject.org/archives/html/xen-devel/2021-05/msg00882.html)
> for more details.
> 
> The whole design is about Static Allocation and direct-map, and this
> Patch Serie only covers parts of it, which are direct-map memory map.
> Other features will be delievered through different patch series.
> 
> See https://lists.xenproject.org/archives/html/xen-devel/2021-09/msg00855.html
> for Domain on Static Allocation.
> 
> This patch serie is based on
> https://lists.xenproject.org/archives/html/xen-devel/2021-10/msg00822.html

I haven't had a chance to review the series but I wanted to say that I
tested it successfully both with and without direct-map, so:

Tested-by: Stefano Stabellini <sstabellini@kernel.org>

Re: [PATCH v3 10/10] xen/docs: Document how to do passthrough without IOMMU

[Bertrand Marquis]

Hi Penny,

> On 16 Nov 2021, at 06:31, Penny Zheng <penny.zheng@arm.com> wrote:
> 
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> This commit creates a new doc to document how to do passthrough without IOMMU.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> docs/misc/arm/passthrough-noiommu.txt | 52 +++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
> create mode 100644 docs/misc/arm/passthrough-noiommu.txt
> 
> diff --git a/docs/misc/arm/passthrough-noiommu.txt b/docs/misc/arm/passthrough-noiommu.txt
> new file mode 100644
> index 0000000000..3e2ef21ad7
> --- /dev/null
> +++ b/docs/misc/arm/passthrough-noiommu.txt
> @@ -0,0 +1,52 @@
> +Request Device Assignment without IOMMU support
> +===============================================
> +
> +*WARNING:
> +Users should be aware that it is not always secure to assign a device without
> +IOMMU protection.
> +When the device is not protected by the IOMMU, the administrator should make
> +sure that:
> + 1. The device is assigned to a trusted guest.
> + 2. Users have additional security mechanism on the platform.
> +
> +This document assumes that the IOMMU is absent from the system or it is
> +disabled (status = "disabled" in device tree).
> +
> +Add xen,force-assign-without-iommu; to the device tree snippet:
> +
> +ethernet: ethernet@ff0e0000 {
> +	compatible = "cdns,zynqmp-gem";
> +	xen,path = "/amba/ethernet@ff0e0000";
> +	xen,reg = <0x0 0xff0e0000 0x1000 0x0 0xff0e0000>;
> +	xen,force-assign-without-iommu;
> +};
> +
> +Request 1:1 memory mapping for the domain on static allocation
> +==============================================================
> +
> +Add a direct-map property under the appropriate /chosen/domU node which
> +is also statically allocated with physical memory ranges through
> +xen,static-mem property as its guest RAM.
> +
> +Below is an example on how to specify the 1:1 memory mapping for the domain
> +on static allocation in the device-tree:
> +
> +/ {
> +	chosen {
> +		domU1 {
> +			compatible = "xen,domain";
> +			#address-cells = <0x2>;
> +			#size-cells = <0x2>;
> +			cpus = <2>;
> +			memory = <0x0 0x80000>;
> +			#xen,static-mem-address-cells = <0x1>;
> +			#xen,static-mem-size-cells = <0x1>;
> +			xen,static-mem = <0x30000000 0x20000000>;
> +			direct-map;
> +			...
> +		};
> +	};
> +};
> +
> +Besides reserving a 512MB region starting at the host physical address
> +0x30000000 to DomU1, it also requests 1:1 memory mapping.

If the guest has some reserved-memory in the device tree, those will not be handle by Xen for DomUs.
I think it could be a good idea in the documentation to mention that any reserved-memory required must be assigned using static-mem to the DomU.

This could take the form of a comment or a warning in this document.

Regards
Bertrand

> -- 
> 2.25.1
> 

Re: [PATCH v3 02/10] xen/arm: avoid setting XEN_DOMCTL_CDF_iommu when IOMMU off

[Julien Grall]

Hi Penny,

On 16/11/2021 06:31, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> This commit avoids setting XEN_DOMCTL_CDF_iommu when the IOMMU is
> absent/disabled, otherwise xen will fail later when handling
> device assignment.

I would explain briefly in the commit message why you want to do device 
assignment without PCI passthrough.

Other than that, the change below is fine with me.

> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> v3 changes:
> - new commit, split from the original "[PATCH v2 2/6] xen/arm: introduce
> direct-map for domUs"
> ---
>   xen/arch/arm/domain_build.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 664c88ebe4..7a063f62fe 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2996,7 +2996,8 @@ void __init create_domUs(void)
>               panic("Missing property 'cpus' for domain %s\n",
>                     dt_node_name(node));
>   
> -        if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") )
> +        if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
> +             iommu_enabled )
>               d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
>   
>           if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v3 03/10] xen/arm: introduce new helper parse_static_mem_prop and acquire_static_memory_bank

[Julien Grall]

Hi Penny,

On 16/11/2021 06:31, Penny Zheng wrote:
> Later, we will introduce allocate_static_memory_11 for allocating static
> memory for direct-map domains, and it will share a lot common codes with
> the existing allocate_static_memory.
> 
> In order not to bring a lot of duplicate codes, and also to make the whole
> code more readable, this commit extracts common codes into two new helpers
> parse_static_mem_prop and acquire_static_memory_bank.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v3 changes:
> - new commit to move the split off of the code outside in a separate patch
> ---
>   xen/arch/arm/domain_build.c | 100 +++++++++++++++++++++++-------------
>   1 file changed, 64 insertions(+), 36 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 7a063f62fe..1dc728e848 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -515,12 +515,69 @@ static bool __init append_static_memory_to_bank(struct domain *d,
>       return true;
>   }
>   
> +static mfn_t __init acquire_static_memory_bank(struct domain *d,
> +                                             const __be32 **cell,
> +                                             u32 addr_cells, u32 size_cells,
> +                                             paddr_t *pbase, paddr_t *psize)
> +{
> +    mfn_t smfn;
> +    int res;
> +
> +    device_tree_get_reg(cell, addr_cells, size_cells, pbase, psize);
> +    ASSERT(IS_ALIGNED(*pbase, PAGE_SIZE) && IS_ALIGNED(*psize, PAGE_SIZE));
> +    if ( PFN_DOWN(*psize) > UINT_MAX )
> +    {
> +        printk(XENLOG_ERR "%pd: static memory size too large: %#"PRIpaddr,
> +               d, *psize);
> +        return INVALID_MFN;
> +    }
> +
> +    smfn = maddr_to_mfn(*pbase);
> +    res = acquire_domstatic_pages(d, smfn, PFN_DOWN(*psize), 0);
> +    if ( res )
> +    {
> +        printk(XENLOG_ERR
> +               "%pd: failed to acquire static memory: %d.\n", d, res);
> +        return INVALID_MFN;
> +    }
> +
> +    return smfn;
> +}
> +
> +static int __init parse_static_mem_prop(const struct dt_device_node *node,
> +                                        u32 *addr_cells, u32 *size_cells,
> +                                        int *length, const __be32 **cell)
> +{
> +    const struct dt_property *prop;
> +
> +    prop = dt_find_property(node, "xen,static-mem", NULL);
> +    if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> +                               addr_cells) )
> +    {
> +        printk(XENLOG_ERR
> +               "failed to read \"#xen,static-mem-address-cells\".\n");
> +        return -EINVAL;
> +    }
> +
> +    if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> +                               size_cells) )
> +    {
> +        printk(XENLOG_ERR
> +               "failed to read \"#xen,static-mem-size-cells\".\n");
> +        return -EINVAL;
> +    }
> +
> +    *cell = (const __be32 *)prop->value;
> +    *length = prop->length;
> +
> +    return 0;
> +}
> +
>   /* Allocate memory from static memory as RAM for one specific domain d. */
>   static void __init allocate_static_memory(struct domain *d,
>                                             struct kernel_info *kinfo,
>                                             const struct dt_device_node *node)
>   {
> -    const struct dt_property *prop;
>       u32 addr_cells, size_cells, reg_cells;
>       unsigned int nr_banks, gbank, bank = 0;
>       const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> @@ -529,24 +586,10 @@ static void __init allocate_static_memory(struct domain *d,
>       u64 tot_size = 0;
>       paddr_t pbase, psize, gsize;
>       mfn_t smfn;
> -    int res;
> -
> -    prop = dt_find_property(node, "xen,static-mem", NULL);
> -    if ( !dt_property_read_u32(node, "#xen,static-mem-address-cells",
> -                               &addr_cells) )
> -    {
> -        printk(XENLOG_ERR
> -               "%pd: failed to read \"#xen,static-mem-address-cells\".\n", d);
> -        goto fail;
> -    }
> +    int length;
>   
> -    if ( !dt_property_read_u32(node, "#xen,static-mem-size-cells",
> -                               &size_cells) )
> -    {
> -        printk(XENLOG_ERR
> -               "%pd: failed to read \"#xen,static-mem-size-cells\".\n", d);
> +    if ( parse_static_mem_prop(node, &addr_cells, &size_cells, &length, &cell) )
>           goto fail;
> -    }
>       reg_cells = addr_cells + size_cells;
>   
>       /*
> @@ -557,29 +600,14 @@ static void __init allocate_static_memory(struct domain *d,
>       gbank = 0;
>       gsize = ramsize[gbank];
>       kinfo->mem.bank[gbank].start = rambase[gbank];
> -
> -    cell = (const __be32 *)prop->value;
> -    nr_banks = (prop->length) / (reg_cells * sizeof (u32));
> +    nr_banks = length / (reg_cells * sizeof (u32));
>   
>       for ( ; bank < nr_banks; bank++ )
>       {
> -        device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
> -        ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize, PAGE_SIZE));
> -
> -        if ( PFN_DOWN(psize) > UINT_MAX )
> -        {
> -            printk(XENLOG_ERR "%pd: static memory size too large: %#"PRIpaddr,
> -                   d, psize);
> +        smfn = acquire_static_memory_bank(d, &cell, addr_cells, size_cells,
> +                                          &pbase, &psize);
> +        if ( !mfn_valid(smfn) )

I would use mfn_eq(mfn, INVALID_MFN) because this is the only value used 
to indicate there is an error and this is more lightweight than mfn_valid().

>               goto fail;
> -        }
> -        smfn = maddr_to_mfn(pbase);
> -        res = acquire_domstatic_pages(d, smfn, PFN_DOWN(psize), 0);
> -        if ( res )
> -        {
> -            printk(XENLOG_ERR
> -                   "%pd: failed to acquire static memory: %d.\n", d, res);
> -            goto fail;
> -        }
>   
>           printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n",
>                  d, bank, pbase, pbase + psize);
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v3 04/10] xen/arm: introduce direct-map for domUs

[Julien Grall]

Hi Penny,

On 16/11/2021 06:31, Penny Zheng wrote:
> Cases where domU needs direct-map memory map:
>    * IOMMU not present in the system.
>    * IOMMU disabled if it doesn't cover a specific device and all the guests
> are trusted. Thinking a mixed scenario, where a few devices with IOMMU and
> a few without, then guest DMA security still could not be totally guaranteed.
> So users may want to disable the IOMMU, to at least gain some performance
> improvement from IOMMU disabled.
>    * IOMMU disabled as a workaround when it doesn't have enough bandwidth.
> To be specific, in a few extreme situation, when multiple devices do DMA
> concurrently, these requests may exceed IOMMU's transmission capacity.
>    * IOMMU disabled when it adds too much latency on DMA. For example,
> TLB may be missing in some IOMMU hardware, which may bring latency in DMA
> progress, so users may want to disable it in some realtime scenario.
>    * Guest OS relies on the host memory layout
> 
> This commit introduces a new helper allocate_static_memory_11 to allocate
> static memory as guest RAM for direct-map domain.
> 
> For now, direct-map is only available when statically allocated memory is
> used for the domain, that is, "xen,static-mem" must be also defined in the
> domain configuration.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> ---
> v2 changes:
> - split the common codes into two helpers: parse_static_mem_prop and
> acquire_static_memory_bank to deduce complexity.
> - introduce a new helper allocate_static_memory_11 for allocating static
> memory for direct-map guests
> - remove redundant use "bool direct_map", to be replaced by
> d_cfg.flags & XEN_DOMCTL_CDF_directmap
> - remove panic action since it is fine to assign a non-DMA capable device when
> IOMMU and direct-map both off
> ---
> v3 changes:
> - doc refinement
> - drop the pointless gbank
> - add check of the size of nr_banks shall not exceed NR_MEM_BANKS
> - add ASSERT_UNREACHABLE to catch any misuse
> - add another check of validating flag XEN_DOMCTL_CDF_INTERNAL_directmap only
> when CONFIG_STATIC_MEMORY is set.
> ---
>   docs/misc/arm/device-tree/booting.txt |   6 ++
>   xen/arch/arm/domain_build.c           | 105 +++++++++++++++++++++++++-
>   2 files changed, 108 insertions(+), 3 deletions(-)
> 
> diff --git a/docs/misc/arm/device-tree/booting.txt b/docs/misc/arm/device-tree/booting.txt
> index 71895663a4..a94125394e 100644
> --- a/docs/misc/arm/device-tree/booting.txt
> +++ b/docs/misc/arm/device-tree/booting.txt
> @@ -182,6 +182,12 @@ with the following properties:
>       Both #address-cells and #size-cells need to be specified because
>       both sub-nodes (described shortly) have reg properties.
>   
> +- direct-map
> +
> +    Only available when statically allocated memory is used for the domain.
> +    An empty property to request the memory of the domain to be
> +    direct-map (guest physical address == physical address).
> +
>   Under the "xen,domain" compatible node, one or more sub-nodes are present
>   for the DomU kernel and ramdisk.
>   
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 1dc728e848..97a5b5dedd 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -500,8 +500,13 @@ static bool __init append_static_memory_to_bank(struct domain *d,
>   {
>       int res;
>       unsigned int nr_pages = PFN_DOWN(size);
> -    /* Infer next GFN. */
> -    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> +    gfn_t sgfn;
> +
> +    if ( !is_domain_direct_mapped(d) )
> +        /* Infer next GFN when GFN != MFN. */

I would move this comment just before the if and write something like:

/*
  * For direct-mapped domain, the GFN match the MFN.
  * Otherwise, this is inferred on what has already been allocated in the
  * bank.
  */

> +        sgfn = gaddr_to_gfn(bank->start + bank->size);
> +    else
> +        sgfn = gaddr_to_gfn(mfn_to_maddr(smfn));
>   
>       res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
>       if ( res )
> @@ -674,12 +679,94 @@ static void __init allocate_static_memory(struct domain *d,
>    fail:
>       panic("Failed to allocate requested static memory for domain %pd.", d);
>   }
> +
> +/*
> + * Allocate static memory as RAM for one specific domain d.
> + * The static memory will be directly mapped in the guest(Guest Physical
> + * Address == Physical Address).
> + */
> +static void __init allocate_static_memory_11(struct domain *d,

I would rename the function to assign_static_memory_11() to make
clear there is no allocation done. Instead we are only mapping 
pre-defined host regions to pre-defined guest regions.

> +                                             struct kernel_info *kinfo,
> +                                             const struct dt_device_node *node)
> +{
> +    u32 addr_cells, size_cells, reg_cells;
> +    unsigned int nr_banks, bank = 0;
> +    const __be32 *cell;
> +    u64 tot_size = 0;
> +    paddr_t pbase, psize;
> +    mfn_t smfn;
> +    int length;
> +
> +    if ( parse_static_mem_prop(node, &addr_cells, &size_cells, &length, &cell) )
> +    {
> +        printk(XENLOG_ERR
> +               "%pd: failed to parse \"xen,static-mem\" property.\n", d);
> +        goto fail;
> +    }
> +    reg_cells = addr_cells + size_cells;
> +    nr_banks = length / (reg_cells * sizeof (u32));
> +
> +    if ( nr_banks > NR_MEM_BANKS )
> +    {
> +        printk(XENLOG_ERR
> +               "%pd: exceed max number of supported guest memory banks.\n", d);
> +        goto fail;
> +    }
> +
> +    for ( ; bank < nr_banks; bank++ )
> +    {
> +        smfn = acquire_static_memory_bank(d, &cell, addr_cells, size_cells,
> +                                          &pbase, &psize);
> +        if ( !mfn_valid(smfn) )
> +            goto fail;
> +
> +        printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-%#"PRIpaddr"\n",
> +               d, bank, pbase, pbase + psize);
> +
> +        /* One guest memory bank is matched with one physical memory bank. */
> +        kinfo->mem.bank[bank].start = pbase;
> +        if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[bank],
> +                                           smfn, psize) )
> +            goto fail;
> +
> +        tot_size += psize;

Rather than using tot_size, I would directly substract psize from 
kinfo->unassigned_mem. Regardless that...

> +    }
> +
> +    kinfo->mem.nr_banks = nr_banks;
> +
> +    kinfo->unassigned_mem -= tot_size;

... we should check that this doesn't underflow.

> +    /*
> +     * The property 'memory' should match the amount of memory given to the
> +     * guest.
> +     * Currently, it is only possible to either acquire static memory or let
> +     * Xen allocate. *Mixing* is not supported.
> +     */
> +    if ( kinfo->unassigned_mem )
> +    {
> +        printk(XENLOG_ERR
> +               "Size of \"memory\" property doesn't match up with the sum-up of \"xen,static-mem\". Unsupported configuration.\n");
> +        goto fail;
> +    }
> +
> +    return;
> +
> + fail:
> +    panic("Failed to allocate requested static memory for direct-map domain %pd.",
> +          d);
> +}
>   #else
>   static void __init allocate_static_memory(struct domain *d,
>                                             struct kernel_info *kinfo,
>                                             const struct dt_device_node *node)
>   {
>   }
> +
> +static void __init allocate_static_memory_11(struct domain *d,
> +                                             struct kernel_info *kinfo,
> +                                             const struct dt_device_node *node)
> +{
> +    ASSERT_UNREACHABLE();
> +}
>   #endif
>   
>   static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
> @@ -2983,7 +3070,12 @@ static int __init construct_domU(struct domain *d,
>       if ( !dt_find_property(node, "xen,static-mem", NULL) )
>           allocate_memory(d, &kinfo);
>       else
> -        allocate_static_memory(d, &kinfo, node);
> +    {
> +        if ( is_domain_direct_mapped(d) )
> +            allocate_static_memory_11(d, &kinfo, node);
> +        else
> +            allocate_static_memory(d, &kinfo, node);

The nested if/else can be avoided if you use:

if ( !dt_find_property() )
    ...
else if ( !is_domain_direct_mapped() )
    ...
else
    ...

> +    }
>   
>       rc = prepare_dtb_domU(d, &kinfo);
>       if ( rc < 0 )
> @@ -3024,6 +3116,13 @@ void __init create_domUs(void)
>               panic("Missing property 'cpus' for domain %s\n",
>                     dt_node_name(node));
>   
> +        if ( dt_property_read_bool(node, "direct-map") )
> +        {
> +            if ( !IS_ENABLED(CONFIG_STATIC_MEMORY) )
> +                panic("direct-map not valid without CONFIG_STATIC_MEMORY\n");

I would print the node name (see an example above) to help the admin to 
find the "error" in the DT.

Also I would write "direct-map is not valid ...".

> +            d_cfg.flags |= XEN_DOMCTL_CDF_INTERNAL_directmap;
> +        }
> +
>           if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") &&
>                iommu_enabled )
>               d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v3 05/10] xen/arm: add ASSERT_UNREACHABLE in allocate_static_memory

[Julien Grall]

Hi Penny,

On 16/11/2021 06:31, Penny Zheng wrote:
> Helper allocate_static_memory is not meant to be reachable when built with

^ The helper...

> !CONFIG_STATIC_MEMORY, so this commit adds ASSERT_UNREACHABLE in it to catch
> potential misuse.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>

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

Cheers,

-- 
Julien Grall

Re: [PATCH v3 06/10] xen/arm: if direct-map domain use native addresses for GICv2

[Julien Grall]

Hi,

On 16/11/2021 06:31, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Today we use native addresses to map the GICv2 for Dom0 and fixed
> addresses for DomUs.
> 
> This patch changes the behavior so that native addresses are used for
> all domains that are direct-mapped.
> 
> NEW VGIC has different naming schemes, like referring distributor base
> address as vgic_dist_base, other than the dbase. So this patch also introduces
> vgic_dist_base/vgic_cpu_base accessor to access correct distributor base
> address/cpu interface base address on varied scenarios,
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

I think it is more common to have the Signed-off-by of the Author first. 
Assuming the main author is Stefano, then this should be switched around.

> ---
> v2 changes
> - combine all changes in patch 4-6 here
> ---
> v3 changes
> - refine comment message
> - add a comment explaining how the 38 was found of "char buf[38]"
> - simply map the CPU interface at the GPA vgic_v2_hw.cbase
> - remove a spurious change
> ---
>   xen/arch/arm/domain_build.c    | 11 ++++++++---
>   xen/arch/arm/vgic-v2.c         | 31 ++++++++++++++++++++++---------
>   xen/arch/arm/vgic/vgic-v2.c    | 31 ++++++++++++++++++++++---------
>   xen/include/asm-arm/new_vgic.h | 10 ++++++++++
>   xen/include/asm-arm/vgic.h     | 11 +++++++++++
>   5 files changed, 73 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b6fde74d74..c419a4b2cc 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2230,8 +2230,13 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
>       int res = 0;
>       __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
>       __be32 *cells;
> +    const struct domain *d = kinfo->d;
> +    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
> +    char buf[38];
>   
> -    res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICD_BASE));
> +    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> +             vgic_dist_base(&d->arch.vgic));
> +    res = fdt_begin_node(fdt, buf);
>       if ( res )
>           return res;
>   
> @@ -2253,9 +2258,9 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
>   
>       cells = &reg[0];
>       dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> -                       GUEST_GICD_BASE, GUEST_GICD_SIZE);
> +                       vgic_dist_base(&d->arch.vgic), GUEST_GICD_SIZE);
>       dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> -                       GUEST_GICC_BASE, GUEST_GICC_SIZE);
> +                       vgic_cpu_base(&d->arch.vgic), GUEST_GICC_SIZE);
>   
>       res = fdt_property(fdt, "reg", reg, sizeof(reg));
>       if (res)
> diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> index 589c033eda..6f5492e30e 100644
> --- a/xen/arch/arm/vgic-v2.c
> +++ b/xen/arch/arm/vgic-v2.c
> @@ -654,13 +654,10 @@ static int vgic_v2_vcpu_init(struct vcpu *v)
>   static int vgic_v2_domain_init(struct domain *d)
>   {
>       int ret;
> -    paddr_t cbase, csize;
> +    paddr_t csize;
>       paddr_t vbase;
>   
> -    /*
> -     * The hardware domain gets the hardware address.
> -     * Guests get the virtual platform layout.
> -     */
> +    /* The hardware domain gets the hardware address. */

I would prefer if this comment is a summary of the if/else if/else. This 
would be easier to understand we need a different path for the hwdom
and direct-mapped domain.

"
The hardware domain and direct-mapped domains get the hardware address. 
We have to handle them separately because the hwdom is re-using the same 
Device-Tree as the host (see more details below).

Other domais get the virtual platform layout.
"

>       if ( is_hardware_domain(d) )
>       {
>           d->arch.vgic.dbase = vgic_v2_hw.dbase;
> @@ -671,10 +668,26 @@ static int vgic_v2_domain_init(struct domain *d)
>            * Note that we assume the size of the CPU interface is always
>            * aligned to PAGE_SIZE.
>            */
> -        cbase = vgic_v2_hw.cbase;
> +        d->arch.vgic.cbase = vgic_v2_hw.cbase;
>           csize = vgic_v2_hw.csize;
>           vbase = vgic_v2_hw.vbase;
>       }
> +    else if ( is_domain_direct_mapped(d) )
> +    {
> +        /*
> +         * 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 occupied by the physical GIC in the host device tree.
> +         *
> +         * We need to add an offset to the virtual CPU interface base
> +         * address when the GIC is aliased to get a 8kB contiguous
> +         * region.
> +         */
> +        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;
> +    }
>       else
>       {
>           d->arch.vgic.dbase = GUEST_GICD_BASE;
> @@ -685,7 +698,7 @@ static int vgic_v2_domain_init(struct domain *d)
>            * region.
>            */
>           BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
> -        cbase = GUEST_GICC_BASE;
> +        d->arch.vgic.cbase = GUEST_GICC_BASE;
>           csize = GUEST_GICC_SIZE;
>           vbase = vgic_v2_hw.vbase + vgic_v2_hw.aliased_offset;
>       }
> @@ -694,8 +707,8 @@ static int vgic_v2_domain_init(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(cbase), csize / PAGE_SIZE,
> -                           maddr_to_mfn(vbase));
> +    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.cbase),
> +                           csize / PAGE_SIZE, maddr_to_mfn(vbase));
>       if ( ret )
>           return ret;
>   
> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> index b5ba4ace87..63d0f03688 100644
> --- a/xen/arch/arm/vgic/vgic-v2.c
> +++ b/xen/arch/arm/vgic/vgic-v2.c
> @@ -258,14 +258,11 @@ void vgic_v2_enable(struct vcpu *vcpu)
>   int vgic_v2_map_resources(struct domain *d)
>   {
>       struct vgic_dist *dist = &d->arch.vgic;
> -    paddr_t cbase, csize;
> +    paddr_t csize;
>       paddr_t vbase;
>       int ret;
>   
> -    /*
> -     * The hardware domain gets the hardware address.
> -     * Guests get the virtual platform layout.
> -     */
> +    /* The hardware domain gets the hardware address. */

Same as above. You could simply copy/paste the comment.

>       if ( is_hardware_domain(d) )
>       {
>           d->arch.vgic.vgic_dist_base = gic_v2_hw_data.dbase;
> @@ -276,10 +273,26 @@ int vgic_v2_map_resources(struct domain *d)
>            * Note that we assume the size of the CPU interface is always
>            * aligned to PAGE_SIZE.
>            */
> -        cbase = gic_v2_hw_data.cbase;
> +        d->arch.vgic.vgic_cpu_base = 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;
> +        /*
> +         * 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
> +         * occupied by the physical GIC in the host device tree.
> +         *
> +         * We need to add an offset to the virtual CPU interface base
> +         * address when the GIC is aliased to get a 8kB contiguous
> +         * region.
> +         */
> +        d->arch.vgic.vgic_cpu_base = 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;
> @@ -290,7 +303,7 @@ int vgic_v2_map_resources(struct domain *d)
>            * region.
>            */
>           BUILD_BUG_ON(GUEST_GICC_SIZE != SZ_8K);
> -        cbase = GUEST_GICC_BASE;
> +        d->arch.vgic.vgic_cpu_base = GUEST_GICC_BASE;
>           csize = GUEST_GICC_SIZE;
>           vbase = gic_v2_hw_data.vbase + gic_v2_hw_data.aliased_offset;
>       }
> @@ -308,8 +321,8 @@ 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(cbase), csize / PAGE_SIZE,
> -                           maddr_to_mfn(vbase));
> +    ret = map_mmio_regions(d, gaddr_to_gfn(d->arch.vgic.vgic_cpu_base),
> +                           csize / PAGE_SIZE, maddr_to_mfn(vbase));
>       if ( ret )
>       {
>           gdprintk(XENLOG_ERR, "Unable to remap VGIC CPU to VCPU\n");
> diff --git a/xen/include/asm-arm/new_vgic.h b/xen/include/asm-arm/new_vgic.h
> index 97d622bff6..ab57fcd91d 100644
> --- a/xen/include/asm-arm/new_vgic.h
> +++ b/xen/include/asm-arm/new_vgic.h
> @@ -186,6 +186,16 @@ struct vgic_cpu {
>       uint32_t num_id_bits;
>   };
>   
> +static inline paddr_t vgic_cpu_base(const struct vgic_dist *vgic)
> +{
> +    return vgic->vgic_cpu_base;
> +}
> +
> +static inline paddr_t vgic_dist_base(const struct vgic_dist *vgic)
> +{
> +    return vgic->vgic_dist_base;
> +}
> +
>   #endif /* __ASM_ARM_NEW_VGIC_H */
>   
>   /*
> diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> index e69a59063a..a81a06c711 100644
> --- a/xen/include/asm-arm/vgic.h
> +++ b/xen/include/asm-arm/vgic.h
> @@ -152,6 +152,7 @@ struct vgic_dist {
>       struct pending_irq *pending_irqs;
>       /* Base address for guest GIC */
>       paddr_t dbase; /* Distributor base address */
> +    paddr_t cbase; /* CPU interface base address */
>   #ifdef CONFIG_GICV3
>       /* GIC V3 addressing */
>       /* List of contiguous occupied by the redistributors */
> @@ -271,6 +272,16 @@ static inline int REG_RANK_NR(int b, uint32_t n)
>   
>   enum gic_sgi_mode;
>   
> +static inline paddr_t vgic_cpu_base(const struct vgic_dist *vgic)
> +{
> +    return vgic->cbase;
> +}
> +
> +static inline paddr_t vgic_dist_base(const struct vgic_dist *vgic)
> +{
> +    return vgic->dbase;
> +}
> +
>   /*
>    * Offset of GICD_<FOO><n> with its rank, for GICD_<FOO> size <s> with
>    * <b>-bits-per-interrupt.
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v3 07/10] xen/arm: gate make_gicv3_domU_node with CONFIG_GICV3

[Julien Grall]

Hi Penny,

On 16/11/2021 06:31, Penny Zheng wrote:
> This commit gates function make_gicv3_domU_node with CONFIG_GICV3, and
> also adds ASSERT_UNREACHABLE to catch any misuse.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> v3 changes:
> - new commit
> ---
>   xen/arch/arm/domain_build.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c419a4b2cc..24f3edf069 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2279,6 +2279,7 @@ static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
>       return res;
>   }
>   
> +#ifdef CONFIG_GICV3
>   static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>   {
>       void *fdt = kinfo->fdt;
> @@ -2328,6 +2329,12 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>   
>       return res;
>   }
> +#else
> +static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
> +{
> +    ASSERT_UNREACHABLE();

This will break compilation on at least prod build with CONFIG_GICV3=n. 
However...

> +}
> +#endif

... I would prefer to #ifdef the case (as you did in the previous 
version). That said, if you strongly prefer this approach then I am OK 
with it.

>   
>   static int __init make_gic_domU_node(struct kernel_info *kinfo)
>   {
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v3 08/10] xen/arm: if direct-map domain use native addresses for GICv3

[Julien Grall]

Hi Penny,

On 16/11/2021 06:31, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> Today we use native addresses to map the GICv3 for Dom0 and fixed
> addresses for DomUs.
> 
> This patch changes the behavior so that native addresses are used for
> all domain which is using the host memory layout
> 
> Considering that DOM0 may not always be directly mapped in the future,
> this patch introduces a new helper "domain_use_host_layout()" that
> wraps both two check "is_domain_direct_mapped(d) || is_hardware_domain(d)"
> for more flexible usage.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

Same remark as another patch about the order of the signed-off-by.

> ---
> v2 changes:
> - remove redistributor accessor
> - introduce new helper "is_domain_use_host_layout()"
> - comment fix
> ---
> v3 changes:
> - the comment on top of 'buf' to explain how 38 was found
> - fix res getting overwritten
> - drop 'cells += (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS)'
> - free 'reg' right way
> - fix comment
> - rename 'is_domain_use_host_layout()' to 'domain_use_host_layout()'
> ---
>   xen/arch/arm/domain_build.c  | 37 +++++++++++++++++++++++++++---------
>   xen/arch/arm/vgic-v3.c       | 29 ++++++++++++++++------------
>   xen/include/asm-arm/domain.h |  7 +++++++
>   3 files changed, 52 insertions(+), 21 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 24f3edf069..61fd374c5d 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2284,10 +2284,16 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>   {
>       void *fdt = kinfo->fdt;
>       int res = 0;
> -    __be32 reg[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) * 2];
> -    __be32 *cells;
> +    __be32 *reg;
> +    const struct domain *d = kinfo->d;
> +    /* Placeholder for interrupt-controller@ + a 64-bit number + \0 */
> +    char buf[38];
> +    unsigned int i, len = 0;
>   
> -    res = fdt_begin_node(fdt, "interrupt-controller@"__stringify(GUEST_GICV3_GICD_BASE));
> +    snprintf(buf, sizeof(buf), "interrupt-controller@%"PRIx64,
> +             vgic_dist_base(&d->arch.vgic));
> +
> +    res = fdt_begin_node(fdt, buf);
>       if ( res )
>           return res;
>   
> @@ -2307,13 +2313,26 @@ static int __init make_gicv3_domU_node(struct kernel_info *kinfo)
>       if ( res )
>           return res;
>   
> -    cells = &reg[0];
> -    dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> -                       GUEST_GICV3_GICD_BASE, GUEST_GICV3_GICD_SIZE);
> -    dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> -                       GUEST_GICV3_GICR0_BASE, GUEST_GICV3_GICR0_SIZE);
> +    /* reg specifies all re-distributors and Distributor. */
> +    len = (GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> +          (d->arch.vgic.nr_regions + 1) * sizeof(__be32);
> +    reg = xmalloc_bytes(len);
> +    if ( reg == NULL )
> +        return -ENOMEM;
>   
> -    res = fdt_property(fdt, "reg", reg, sizeof(reg));
> +    dt_child_set_range(&reg, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +                       vgic_dist_base(&d->arch.vgic), GUEST_GICV3_GICD_SIZE);
> +
> +    for ( i = 0; i < d->arch.vgic.nr_regions; i++)
> +    {
> +        dt_child_set_range(&reg,
> +                           GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
> +                           d->arch.vgic.rdist_regions[i].base,
> +                           d->arch.vgic.rdist_regions[i].size);
> +    }
> +
> +    res = fdt_property(fdt, "reg", reg, len);
> +    xfree(reg);
>       if (res)
>           return res;
>   
> diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
> index 65bb7991a6..181b66513d 100644
> --- a/xen/arch/arm/vgic-v3.c
> +++ b/xen/arch/arm/vgic-v3.c
> @@ -1640,14 +1640,15 @@ static inline unsigned int vgic_v3_max_rdist_count(struct domain *d)
>        * Normally there is only one GICv3 redistributor region.
>        * The GICv3 DT binding provisions for multiple regions, since there are
>        * platforms out there which need those (multi-socket systems).
> -     * For Dom0 we have to live with the MMIO layout the hardware provides,
> -     * so we have to copy the multiple regions - as the first region may not
> -     * provide enough space to hold all redistributors we need.
> +     * For domain using the host memory layout, we have to live with the MMIO
> +     * layout the hardware provides, so we have to copy the multiple regions
> +     * - as the first region may not provide enough space to hold all
> +     * redistributors we need.
>        * However DomU get a constructed memory map, so we can go with
>        * the architected single redistributor region.
>        */
> -    return is_hardware_domain(d) ? vgic_v3_hw.nr_rdist_regions :
> -               GUEST_GICV3_RDIST_REGIONS;
> +    return domain_use_host_layout(d) ? vgic_v3_hw.nr_rdist_regions :
> +                                       GUEST_GICV3_RDIST_REGIONS;
>   }
>   
>   static int vgic_v3_domain_init(struct domain *d)
> @@ -1669,10 +1670,14 @@ static int vgic_v3_domain_init(struct domain *d)
>       radix_tree_init(&d->arch.vgic.pend_lpi_tree);
>   
>       /*
> -     * Domain 0 gets the hardware address.
> -     * Guests get the virtual platform layout.
> +     * Since we map the whole GICv3 register memory map(64KB) for
> +     * all domain, DOM0 and direct-map domain could be treated the
> +     * same way here.

I find this confusing because it is not clear what you refer to with the 
"register memory map" here (I think you mean the Distributor). That 
said, I would drop this paragraph as what matters is we are using the 
same layout as the host.

> +     * For domain using the host memory layout, it gets the hardware
> +     * address.
> +     * Other domains get the virtual platform layout.
>        */
> -    if ( is_hardware_domain(d) )
> +    if ( domain_use_host_layout(d) )
>       {
>           unsigned int first_cpu = 0;
>   
> @@ -1695,10 +1700,10 @@ static int vgic_v3_domain_init(struct domain *d)
>           }
>   
>           /*
> -         * The hardware domain may not use all the re-distributors
> -         * regions (e.g when the number of vCPUs does not match the
> -         * number of pCPUs). Update the number of regions to avoid
> -         * exposing unused region as they will not get emulated.
> +         * For domain using the host memory layout, it may not use all
> +         * the re-distributors regions (e.g when the number of vCPUs does
> +         * not match the number of pCPUs). Update the number of regions to
> +         * avoid exposing unused region as they will not get emulated.
>            */
>           d->arch.vgic.nr_regions = i + 1;
>   
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 4f2c3f09d4..0eff93197e 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -32,6 +32,13 @@ enum domain_type {
>   #define is_domain_direct_mapped(d) \
>           (d->options & XEN_DOMCTL_CDF_INTERNAL_directmap)
>   
> +/*
> + * For domain using the host memory layout, we have to live with the MMIO
> + * layout the hardware provides.
> + */

How about:

/*
  * Is the domain using the host memory layout?
  *
  * Direct-mapped domain will always have the RAM mapped with GFN == 
MFN.
  * To avoid any trouble finding space, it is easier to force using the
  * host memory layout.
  *
  * The hardware domain will use the host layout regardless of
  * direct-mapped because some OS may rely on a specific address ranges
  * for the devices.
  */

> +#define domain_use_host_layout(d) (is_domain_direct_mapped(d) || \
> +                                   is_hardware_domain(d))
> +
>   struct vtimer {
>       struct vcpu *v;
>       int irq;
> 

Cheers,

-- 
Julien Grall

Re: [PATCH v3 09/10] xen/arm: if direct-map domain use native UART address and IRQ number for vPL011

[Julien Grall]

Hi Penny,

On 16/11/2021 06:31, Penny Zheng wrote:
> From: Stefano Stabellini <stefano.stabellini@xilinx.com>
> 
> We always use a fix address to map the vPL011 to domains. The address
> could be a problem for direct-map domains.
> 
> So, for domains that are directly mapped, reuse the address of the
> physical UART on the platform to avoid potential clashes.
> 
> Do the same for the virtual IRQ number: instead of always using
> GUEST_VPL011_SPI, try to reuse the physical SPI number if possible.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>

AFAICT, you exchanged the signed-off-by compare to the previous version. 
May I ask why?

> ---
> v2 changes:
> - explain why vpl011 initialization before creating its device tree node
> - error out if the domain is direct-mapped and the IRQ is not found
> - harden the code and add a check/comment when the hardware UART region
> is smaller than GUEST_VPL011_SIZE.
> ---
> v3 changes:
> - explain how the '27' was found for 'buf'
> - fix checking before dereferencing
> - refine comment message
> ---
>   xen/arch/arm/domain_build.c  | 42 ++++++++++++++++++++-----
>   xen/arch/arm/vpl011.c        | 60 +++++++++++++++++++++++++++++++-----
>   xen/include/asm-arm/vpl011.h |  2 ++
>   3 files changed, 90 insertions(+), 14 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 61fd374c5d..871c7114ae 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -30,6 +30,7 @@
>   
>   #include <xen/irq.h>
>   #include <xen/grant_table.h>
> +#include <xen/serial.h>
>   
>   static unsigned int __initdata opt_dom0_max_vcpus;
>   integer_param("dom0_max_vcpus", opt_dom0_max_vcpus);
> @@ -2376,8 +2377,12 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
>       gic_interrupt_t intr;
>       __be32 reg[GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS];
>       __be32 *cells;
> +    struct domain *d = kinfo->d;
> +    /* Placeholder for sbsa-uart@ + a 64-bit number + \0 */
> +    char buf[27];
>   
> -    res = fdt_begin_node(fdt, "sbsa-uart@"__stringify(GUEST_PL011_BASE));
> +    snprintf(buf, sizeof(buf), "sbsa-uart@%"PRIx64, d->arch.vpl011.base_addr);
> +    res = fdt_begin_node(fdt, buf);
>       if ( res )
>           return res;
>   
> @@ -2387,14 +2392,14 @@ static int __init make_vpl011_uart_node(struct kernel_info *kinfo)
>   
>       cells = &reg[0];
>       dt_child_set_range(&cells, GUEST_ROOT_ADDRESS_CELLS,
> -                       GUEST_ROOT_SIZE_CELLS, GUEST_PL011_BASE,
> +                       GUEST_ROOT_SIZE_CELLS, d->arch.vpl011.base_addr,
>                          GUEST_PL011_SIZE);
>   
>       res = fdt_property(fdt, "reg", reg, sizeof(reg));
>       if ( res )
>           return res;
>   
> -    set_interrupt(intr, GUEST_VPL011_SPI, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
> +    set_interrupt(intr, d->arch.vpl011.virq, 0xf, DT_IRQ_TYPE_LEVEL_HIGH);
>   
>       res = fdt_property(fdt, "interrupts", intr, sizeof (intr));
>       if ( res )
> @@ -3109,6 +3114,14 @@ static int __init construct_domU(struct domain *d,
>               allocate_static_memory(d, &kinfo, node);
>       }
>   
> +    /*
> +     * Base address and irq number are needed when creating vpl011 device
> +     * tree node in prepare_dtb_domU, so initialization on related variables
> +     * shall be done first.
> +     */
> +    if ( kinfo.vpl011 )
> +        rc = domain_vpl011_init(d, NULL);
> +
>       rc = prepare_dtb_domU(d, &kinfo);
>       if ( rc < 0 )
>           return rc;
> @@ -3117,9 +3130,6 @@ static int __init construct_domU(struct domain *d,
>       if ( rc < 0 )
>           return rc;
>   
> -    if ( kinfo.vpl011 )
> -        rc = domain_vpl011_init(d, NULL);
> -
>       return rc;
>   }
>   
> @@ -3161,15 +3171,33 @@ void __init create_domUs(void)
>   
>           if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
>           {
> +            unsigned int vpl011_virq = GUEST_VPL011_SPI;
> +
>               d_cfg.arch.nr_spis = gic_number_lines() - 32;
>   
> +            /*
> +             * The VPL011 virq is GUEST_VPL011_SPI, unless direct-map is
> +             * set, in which case it'll match the hardware.
> +             *
> +             * Since here the domain is not totally built, we need to

The domain is not even built at this point. So I would say, "Since the 
domain is not yet created, we can't use d->arch.vpl011.irq. So the logic 
to find the vIRQ has to be hardcoded.".

> +             * open-code the logic to find the vIRQ. and the logic here

You added a full stop. So s/and the/The/.

> +             * is consistent with the ones in domain_vpl011_init().

s/ones/one/ I think.

> +             */
> +            if ( d_cfg.flags & XEN_DOMCTL_CDF_INTERNAL_directmap )
> +            {
> +                vpl011_virq = serial_irq(SERHND_DTUART);
> +                if ( vpl011_virq < 0 )
> +                    panic("Error getting IRQ number for this serial port %d\n",
> +                          SERHND_DTUART);
> +            }
> +
>               /*
>                * vpl011 uses one emulated SPI. If vpl011 is requested, make
>                * sure that we allocate enough SPIs for it.
>                */
>               if ( dt_property_read_bool(node, "vpl011") )
>                   d_cfg.arch.nr_spis = MAX(d_cfg.arch.nr_spis,
> -                                         GUEST_VPL011_SPI - 32 + 1);
> +                                         vpl011_virq - 32 + 1);
>           }
>   
>           /*
> diff --git a/xen/arch/arm/vpl011.c b/xen/arch/arm/vpl011.c
> index 895f436cc4..65610bccaf 100644
> --- a/xen/arch/arm/vpl011.c
> +++ b/xen/arch/arm/vpl011.c
> @@ -29,6 +29,7 @@
>   #include <xen/mm.h>
>   #include <xen/sched.h>
>   #include <xen/console.h>
> +#include <xen/serial.h>
>   #include <public/domctl.h>
>   #include <public/io/console.h>
>   #include <asm/pl011-uart.h>
> @@ -71,11 +72,11 @@ static void vpl011_update_interrupt_status(struct domain *d)
>        * status bit has been set since the last time.
>        */
>       if ( uartmis & ~vpl011->shadow_uartmis )
> -        vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, true);
> +        vgic_inject_irq(d, NULL, vpl011->virq, true);
>   
>       vpl011->shadow_uartmis = uartmis;
>   #else
> -    vgic_inject_irq(d, NULL, GUEST_VPL011_SPI, uartmis);
> +    vgic_inject_irq(d, NULL, vpl011->virq, uartmis);
>   #endif
>   }
>   
> @@ -347,7 +348,8 @@ static int vpl011_mmio_read(struct vcpu *v,
>                               void *priv)
>   {
>       struct hsr_dabt dabt = info->dabt;
> -    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> +    uint32_t vpl011_reg = (uint32_t)(info->gpa -
> +                                     v->domain->arch.vpl011.base_addr);
>       struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>       struct domain *d = v->domain;
>       unsigned long flags;
> @@ -430,7 +432,8 @@ static int vpl011_mmio_write(struct vcpu *v,
>                                void *priv)
>   {
>       struct hsr_dabt dabt = info->dabt;
> -    uint32_t vpl011_reg = (uint32_t)(info->gpa - GUEST_PL011_BASE);
> +    uint32_t vpl011_reg = (uint32_t)(info->gpa -
> +                                     v->domain->arch.vpl011.base_addr);
>       struct vpl011 *vpl011 = &v->domain->arch.vpl011;
>       struct domain *d = v->domain;
>       unsigned long flags;
> @@ -626,6 +629,49 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>       if ( vpl011->backend.dom.ring_buf )
>           return -EINVAL;
>   
> +    /*
> +     * The VPL011 virq is GUEST_VPL011_SPI, except for direct-map domains
> +     * where the hardware value shall be used.
> +     * And the logic here should stay in sync with the one in

I would drop "And".

> +     * create_domUs().
> +     */
> +    if ( is_domain_direct_mapped(d) )
> +    {
> +        const struct vuart_info *uart = serial_vuart_info(SERHND_DTUART);
> +        int vpl011_irq = serial_irq(SERHND_DTUART);
> +
> +        if ( (uart != NULL) && (vpl011_irq > 0) )
> +        {
> +            vpl011->base_addr = uart->base_addr;
> +            vpl011->virq = vpl011_irq;
> +        }
> +        else
> +        {
> +            printk(XENLOG_ERR
> +                   "vpl011: Unable to re-use the Xen UART information.\n");
> +            return -EINVAL;
> +        }
> +
> +        /*
> +         * Since the PL011 we emulate for the guest requires a 4KB region,
> +         * and on some Hardware (e.g. on some sunxi SoC), the UART MMIO
> +         * region is less than 4KB, in which case, there may exist multiple
> +         * devices within the same 4KB region, here adds the following check to
> +         * prevent potential known pitfalls
> +         */
> +        if ( uart->size < GUEST_PL011_SIZE )
> +        {
> +            printk(XENLOG_ERR
> +                   "vpl011: Can't re-use the Xen UART MMIO region as it is too small.\n");
> +            return -EINVAL;
> +        }
> +    }
> +    else
> +    {
> +        vpl011->base_addr = GUEST_PL011_BASE;
> +        vpl011->virq = GUEST_VPL011_SPI;
> +    }
> +
>       /*
>        * info is NULL when the backend is in Xen.
>        * info is != NULL when the backend is in a domain.
> @@ -661,7 +707,7 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>           }
>       }
>   
> -    rc = vgic_reserve_virq(d, GUEST_VPL011_SPI);
> +    rc = vgic_reserve_virq(d, vpl011->virq);
>       if ( !rc )
>       {
>           rc = -EINVAL;
> @@ -673,12 +719,12 @@ int domain_vpl011_init(struct domain *d, struct vpl011_init_info *info)
>       spin_lock_init(&vpl011->lock);
>   
>       register_mmio_handler(d, &vpl011_mmio_handler,
> -                          GUEST_PL011_BASE, GUEST_PL011_SIZE, NULL);
> +                          vpl011->base_addr, GUEST_PL011_SIZE, NULL);
>   
>       return 0;
>   
>   out2:
> -    vgic_free_virq(d, GUEST_VPL011_SPI);
> +    vgic_free_virq(d, vpl011->virq);
>   
>   out1:
>       if ( vpl011->backend_in_domain )
> diff --git a/xen/include/asm-arm/vpl011.h b/xen/include/asm-arm/vpl011.h
> index e6c7ab7381..c09abcd7a9 100644
> --- a/xen/include/asm-arm/vpl011.h
> +++ b/xen/include/asm-arm/vpl011.h
> @@ -53,6 +53,8 @@ struct vpl011 {
>       uint32_t    uarticr;        /* Interrupt clear register */
>       uint32_t    uartris;        /* Raw interrupt status register */
>       uint32_t    shadow_uartmis; /* shadow masked interrupt register */
> +    paddr_t     base_addr;
> +    unsigned int virq;
>       spinlock_t  lock;
>       evtchn_port_t evtchn;
>   };
> 

Cheers,

-- 
Julien Grall