libxl will create an emulated PCI device tree node in the device tree to
enable the guest OS to discover the virtual PCI during guest boot.
Emulated PCI device tree node will only be created when there is any
device assigned to guest.

A new area has been reserved in the arm guest physical map at
which the VPCI bus is declared in the device tree (reg and ranges
parameters of the node).

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 tools/libs/light/libxl_arm.c          | 109 ++++++++++++++++++++++++++
 tools/libs/light/libxl_types.idl      |   1 +
 tools/xl/xl_parse.c                   |   2 +
 xen/include/public/arch-arm.h         |  11 +++
 xen/include/public/device_tree_defs.h |   1 +
 5 files changed, 124 insertions(+)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6e00..a091e97e76 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -269,6 +269,58 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
     return fdt_property(fdt, "reg", regs, sizeof(regs));
 }
 
+static int fdt_property_values(libxl__gc *gc, void *fdt,
+        const char *name, unsigned num_cells, ...)
+{
+    uint32_t prop[num_cells];
+    be32 *cells = &prop[0];
+    int i;
+    va_list ap;
+    uint32_t arg;
+
+    va_start(ap, num_cells);
+    for (i = 0 ; i < num_cells; i++) {
+        arg = va_arg(ap, uint32_t);
+        set_cell(&cells, 1, arg);
+    }
+    va_end(ap);
+
+    return fdt_property(fdt, name, prop, sizeof(prop));
+}
+
+static int fdt_property_vpci_ranges(libxl__gc *gc, void *fdt,
+                                    unsigned addr_cells,
+                                    unsigned size_cells,
+                                    unsigned num_regs, ...)
+{
+    uint32_t regs[num_regs*((addr_cells*2)+size_cells+1)];
+    be32 *cells = &regs[0];
+    int i;
+    va_list ap;
+    uint64_t arg;
+
+    va_start(ap, num_regs);
+    for (i = 0 ; i < num_regs; i++) {
+        /* Set the memory bit field */
+        arg = va_arg(ap, uint64_t);
+        set_cell(&cells, 1, arg);
+
+        /* Set the vpci bus address */
+        arg = addr_cells ? va_arg(ap, uint64_t) : 0;
+        set_cell(&cells, addr_cells , arg);
+
+        /* Set the cpu bus address where vpci address is mapped */
+        set_cell(&cells, addr_cells, arg);
+
+        /* Set the vpci size requested */
+        arg = size_cells ? va_arg(ap, uint64_t) : 0;
+        set_cell(&cells, size_cells,arg);
+    }
+    va_end(ap);
+
+    return fdt_property(fdt, "ranges", regs, sizeof(regs));
+}
+
 static int make_root_properties(libxl__gc *gc,
                                 const libxl_version_info *vers,
                                 void *fdt)
@@ -668,6 +720,57 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+static int make_vpci_node(libxl__gc *gc, void *fdt,
+        const struct arch_info *ainfo,
+        struct xc_dom_image *dom)
+{
+    int res;
+    const uint64_t vpci_ecam_base = GUEST_VPCI_ECAM_BASE;
+    const uint64_t vpci_ecam_size = GUEST_VPCI_ECAM_SIZE;
+    const char *name = GCSPRINTF("pcie@%"PRIx64, vpci_ecam_base);
+
+    res = fdt_begin_node(fdt, name);
+    if (res) return res;
+
+    res = fdt_property_compat(gc, fdt, 1, "pci-host-ecam-generic");
+    if (res) return res;
+
+    res = fdt_property_string(fdt, "device_type", "pci");
+    if (res) return res;
+
+    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
+            GUEST_ROOT_SIZE_CELLS, 1, vpci_ecam_base, vpci_ecam_size);
+    if (res) return res;
+
+    res = fdt_property_values(gc, fdt, "bus-range", 2, 0,17);
+    if (res) return res;
+
+    res = fdt_property_cell(fdt, "#address-cells", 3);
+    if (res) return res;
+
+    res = fdt_property_cell(fdt, "#size-cells", 2);
+    if (res) return res;
+
+    res = fdt_property_string(fdt, "status", "okay");
+    if (res) return res;
+
+    res = fdt_property_vpci_ranges(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
+        GUEST_ROOT_SIZE_CELLS, 2,
+        GUEST_VPCI_ADDR_TYPE_MEM, GUEST_VPCI_MEM_ADDR, GUEST_VPCI_MEM_SIZE,
+        GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM, GUEST_VPCI_PREFETCH_MEM_ADDR,
+        GUEST_VPCI_PREFETCH_MEM_SIZE);
+    if (res) return res;
+
+    res = fdt_property_values(gc, fdt, "msi-map", 4, 0, GUEST_PHANDLE_ITS,
+                              0, 0x10000);
+    if (res) return res;
+
+    res = fdt_end_node(fdt);
+    if (res) return res;
+
+    return 0;
+}
+
 static const struct arch_info *get_arch_info(libxl__gc *gc,
                                              const struct xc_dom_image *dom)
 {
@@ -971,6 +1074,9 @@ next_resize:
         if (info->tee == LIBXL_TEE_TYPE_OPTEE)
             FDT( make_optee_node(gc, fdt) );
 
+        if (libxl_defbool_val(info->arch_arm.vpci))
+            FDT( make_vpci_node(gc, fdt, ainfo, dom) );
+
         if (pfdt)
             FDT( copy_partial_fdt(gc, fdt, pfdt) );
 
@@ -1189,6 +1295,9 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
     /* ACPI is disabled by default */
     libxl_defbool_setdefault(&b_info->acpi, false);
 
+    /* VPCI is disabled by default */
+    libxl_defbool_setdefault(&b_info->arch_arm.vpci, false);
+
     if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
         return;
 
diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
index 3f9fff653a..78b1ddf0b8 100644
--- a/tools/libs/light/libxl_types.idl
+++ b/tools/libs/light/libxl_types.idl
@@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
 
     ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
                                ("vuart", libxl_vuart_type),
+                               ("vpci", libxl_defbool),
                               ])),
     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
                               ])),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 17dddb4cd5..ffafbeffb4 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1497,6 +1497,8 @@ void parse_config_data(const char *config_source,
         }
         if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
             libxl_defbool_set(&b_info->u.pv.e820_host, true);
+        if (d_config->num_pcidevs)
+            libxl_defbool_set(&b_info->arch_arm.vpci, true);
     }
 
     if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 0a9749e768..01d13e669e 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -426,6 +426,17 @@ typedef uint64_t xen_callback_t;
 #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
 #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
 
+/* PCI-PCIe memory space types */
+#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x42000000)
+#define GUEST_VPCI_ADDR_TYPE_MEM          xen_mk_ullong(0x02000000)
+
+/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
+#define GUEST_VPCI_PREFETCH_MEM_ADDR  xen_mk_ullong(0x900000000)
+#define GUEST_VPCI_MEM_ADDR           xen_mk_ullong(0x20000000)
+
+#define GUEST_VPCI_PREFETCH_MEM_SIZE      xen_mk_ullong(0x2000000000)
+#define GUEST_VPCI_MEM_SIZE               xen_mk_ullong(0x04000000)
+
 /*
  * 16MB == 4096 pages reserved for guest to use as a region to map its
  * grant table in.
diff --git a/xen/include/public/device_tree_defs.h b/xen/include/public/device_tree_defs.h
index 209d43de3f..d24d86b1fc 100644
--- a/xen/include/public/device_tree_defs.h
+++ b/xen/include/public/device_tree_defs.h
@@ -7,6 +7,7 @@
  * onwards. Reserve a high value for the GIC phandle.
  */
 #define GUEST_PHANDLE_GIC (65000)
+#define GUEST_PHANDLE_ITS (64999)
 
 #define GUEST_ROOT_ADDRESS_CELLS 2
 #define GUEST_ROOT_SIZE_CELLS 2
-- 
2.17.1

On Thu, 19 Aug 2021, Rahul Singh wrote:
> libxl will create an emulated PCI device tree node in the device tree to
> enable the guest OS to discover the virtual PCI during guest boot.
> Emulated PCI device tree node will only be created when there is any
> device assigned to guest.
> 
> A new area has been reserved in the arm guest physical map at
> which the VPCI bus is declared in the device tree (reg and ranges
> parameters of the node).
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>  tools/libs/light/libxl_arm.c          | 109 ++++++++++++++++++++++++++
>  tools/libs/light/libxl_types.idl      |   1 +
>  tools/xl/xl_parse.c                   |   2 +
>  xen/include/public/arch-arm.h         |  11 +++
>  xen/include/public/device_tree_defs.h |   1 +
>  5 files changed, 124 insertions(+)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6e00..a091e97e76 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -269,6 +269,58 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
>      return fdt_property(fdt, "reg", regs, sizeof(regs));
>  }
>  
> +static int fdt_property_values(libxl__gc *gc, void *fdt,
> +        const char *name, unsigned num_cells, ...)
> +{
> +    uint32_t prop[num_cells];
> +    be32 *cells = &prop[0];
> +    int i;
> +    va_list ap;
> +    uint32_t arg;
> +
> +    va_start(ap, num_cells);
> +    for (i = 0 ; i < num_cells; i++) {
> +        arg = va_arg(ap, uint32_t);
> +        set_cell(&cells, 1, arg);
> +    }
> +    va_end(ap);
> +
> +    return fdt_property(fdt, name, prop, sizeof(prop));
> +}
> +
> +static int fdt_property_vpci_ranges(libxl__gc *gc, void *fdt,
> +                                    unsigned addr_cells,
> +                                    unsigned size_cells,
> +                                    unsigned num_regs, ...)
> +{
> +    uint32_t regs[num_regs*((addr_cells*2)+size_cells+1)];
> +    be32 *cells = &regs[0];
> +    int i;
> +    va_list ap;
> +    uint64_t arg;
> +
> +    va_start(ap, num_regs);
> +    for (i = 0 ; i < num_regs; i++) {
> +        /* Set the memory bit field */
> +        arg = va_arg(ap, uint64_t);
> +        set_cell(&cells, 1, arg);

Shouldn't this be uint32_t given that it is 1 cell exactly?


> +        /* Set the vpci bus address */
> +        arg = addr_cells ? va_arg(ap, uint64_t) : 0;
> +        set_cell(&cells, addr_cells , arg);
> +
> +        /* Set the cpu bus address where vpci address is mapped */
> +        set_cell(&cells, addr_cells, arg);
> +
> +        /* Set the vpci size requested */
> +        arg = size_cells ? va_arg(ap, uint64_t) : 0;
> +        set_cell(&cells, size_cells,arg);
                                       ^ space


> +    }
> +    va_end(ap);
> +
> +    return fdt_property(fdt, "ranges", regs, sizeof(regs));
> +}
> +
>  static int make_root_properties(libxl__gc *gc,
>                                  const libxl_version_info *vers,
>                                  void *fdt)
> @@ -668,6 +720,57 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +static int make_vpci_node(libxl__gc *gc, void *fdt,
> +        const struct arch_info *ainfo,
> +        struct xc_dom_image *dom)
> +{
> +    int res;
> +    const uint64_t vpci_ecam_base = GUEST_VPCI_ECAM_BASE;
> +    const uint64_t vpci_ecam_size = GUEST_VPCI_ECAM_SIZE;
> +    const char *name = GCSPRINTF("pcie@%"PRIx64, vpci_ecam_base);
> +
> +    res = fdt_begin_node(fdt, name);
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, "pci-host-ecam-generic");
> +    if (res) return res;
> +
> +    res = fdt_property_string(fdt, "device_type", "pci");
> +    if (res) return res;
> +
> +    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> +            GUEST_ROOT_SIZE_CELLS, 1, vpci_ecam_base, vpci_ecam_size);
> +    if (res) return res;
> +
> +    res = fdt_property_values(gc, fdt, "bus-range", 2, 0,17);
                                                            ^ space


> +    if (res) return res;
> +
> +    res = fdt_property_cell(fdt, "#address-cells", 3);
> +    if (res) return res;
> +
> +    res = fdt_property_cell(fdt, "#size-cells", 2);
> +    if (res) return res;
> +
> +    res = fdt_property_string(fdt, "status", "okay");
> +    if (res) return res;
> +
> +    res = fdt_property_vpci_ranges(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> +        GUEST_ROOT_SIZE_CELLS, 2,
> +        GUEST_VPCI_ADDR_TYPE_MEM, GUEST_VPCI_MEM_ADDR, GUEST_VPCI_MEM_SIZE,
> +        GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM, GUEST_VPCI_PREFETCH_MEM_ADDR,
> +        GUEST_VPCI_PREFETCH_MEM_SIZE);
> +    if (res) return res;
> +
> +    res = fdt_property_values(gc, fdt, "msi-map", 4, 0, GUEST_PHANDLE_ITS,
> +                              0, 0x10000);
> +    if (res) return res;

I agree with Julien that we shouldn't add it now if it is not working.

One question: what about legacy interrupts? If they are supported,
shouldn't we have interrupts and interrupt-parent properties?


> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return 0;
> +}
> +
>  static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                               const struct xc_dom_image *dom)
>  {
> @@ -971,6 +1074,9 @@ next_resize:
>          if (info->tee == LIBXL_TEE_TYPE_OPTEE)
>              FDT( make_optee_node(gc, fdt) );
>  
> +        if (libxl_defbool_val(info->arch_arm.vpci))
> +            FDT( make_vpci_node(gc, fdt, ainfo, dom) );
> +
>          if (pfdt)
>              FDT( copy_partial_fdt(gc, fdt, pfdt) );
>  
> @@ -1189,6 +1295,9 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>      /* ACPI is disabled by default */
>      libxl_defbool_setdefault(&b_info->acpi, false);
>  
> +    /* VPCI is disabled by default */
> +    libxl_defbool_setdefault(&b_info->arch_arm.vpci, false);
> +
>      if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
>          return;
>  
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 3f9fff653a..78b1ddf0b8 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>  
>      ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>                                 ("vuart", libxl_vuart_type),
> +                               ("vpci", libxl_defbool),
>                                ])),
>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>                                ])),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 17dddb4cd5..ffafbeffb4 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1497,6 +1497,8 @@ void parse_config_data(const char *config_source,
>          }
>          if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
>              libxl_defbool_set(&b_info->u.pv.e820_host, true);
> +        if (d_config->num_pcidevs)
> +            libxl_defbool_set(&b_info->arch_arm.vpci, true);
>      }
>  
>      if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 0a9749e768..01d13e669e 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -426,6 +426,17 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
>  #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
>  
> +/* PCI-PCIe memory space types */
> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x42000000)
> +#define GUEST_VPCI_ADDR_TYPE_MEM          xen_mk_ullong(0x02000000)
> +
> +/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
> +#define GUEST_VPCI_PREFETCH_MEM_ADDR  xen_mk_ullong(0x900000000)
> +#define GUEST_VPCI_MEM_ADDR           xen_mk_ullong(0x20000000)
> +
> +#define GUEST_VPCI_PREFETCH_MEM_SIZE      xen_mk_ullong(0x2000000000)
> +#define GUEST_VPCI_MEM_SIZE               xen_mk_ullong(0x04000000)

Now I get it: GUEST_VPCI_ECAM_BASE was just for the config space, not
the aperture. The apertures are starting at 0x20000000 and 0x900000000.

Speaking of which, isn't GUEST_VPCI_MEM_SIZE a bit too small? There
could be PCI devices with a <4GB requirement. Maybe 256MB?

Hi Stefano,

> On 10 Sep 2021, at 1:51 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Thu, 19 Aug 2021, Rahul Singh wrote:
>> libxl will create an emulated PCI device tree node in the device tree to
>> enable the guest OS to discover the virtual PCI during guest boot.
>> Emulated PCI device tree node will only be created when there is any
>> device assigned to guest.
>> 
>> A new area has been reserved in the arm guest physical map at
>> which the VPCI bus is declared in the device tree (reg and ranges
>> parameters of the node).
>> 
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> tools/libs/light/libxl_arm.c          | 109 ++++++++++++++++++++++++++
>> tools/libs/light/libxl_types.idl      |   1 +
>> tools/xl/xl_parse.c                   |   2 +
>> xen/include/public/arch-arm.h         |  11 +++
>> xen/include/public/device_tree_defs.h |   1 +
>> 5 files changed, 124 insertions(+)
>> 
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index e3140a6e00..a091e97e76 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -269,6 +269,58 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
>>     return fdt_property(fdt, "reg", regs, sizeof(regs));
>> }
>> 
>> +static int fdt_property_values(libxl__gc *gc, void *fdt,
>> +        const char *name, unsigned num_cells, ...)
>> +{
>> +    uint32_t prop[num_cells];
>> +    be32 *cells = &prop[0];
>> +    int i;
>> +    va_list ap;
>> +    uint32_t arg;
>> +
>> +    va_start(ap, num_cells);
>> +    for (i = 0 ; i < num_cells; i++) {
>> +        arg = va_arg(ap, uint32_t);
>> +        set_cell(&cells, 1, arg);
>> +    }
>> +    va_end(ap);
>> +
>> +    return fdt_property(fdt, name, prop, sizeof(prop));
>> +}
>> +
>> +static int fdt_property_vpci_ranges(libxl__gc *gc, void *fdt,
>> +                                    unsigned addr_cells,
>> +                                    unsigned size_cells,
>> +                                    unsigned num_regs, ...)
>> +{
>> +    uint32_t regs[num_regs*((addr_cells*2)+size_cells+1)];
>> +    be32 *cells = &regs[0];
>> +    int i;
>> +    va_list ap;
>> +    uint64_t arg;
>> +
>> +    va_start(ap, num_regs);
>> +    for (i = 0 ; i < num_regs; i++) {
>> +        /* Set the memory bit field */
>> +        arg = va_arg(ap, uint64_t);
>> +        set_cell(&cells, 1, arg);
> 
> Shouldn't this be uint32_t given that it is 1 cell exactly?

Yes I will change it to uint32_t.
> 
> 
>> +        /* Set the vpci bus address */
>> +        arg = addr_cells ? va_arg(ap, uint64_t) : 0;
>> +        set_cell(&cells, addr_cells , arg);
>> +
>> +        /* Set the cpu bus address where vpci address is mapped */
>> +        set_cell(&cells, addr_cells, arg);
>> +
>> +        /* Set the vpci size requested */
>> +        arg = size_cells ? va_arg(ap, uint64_t) : 0;
>> +        set_cell(&cells, size_cells,arg);
>                                       ^ space

Ack.
> 
> 
>> +    }
>> +    va_end(ap);
>> +
>> +    return fdt_property(fdt, "ranges", regs, sizeof(regs));
>> +}
>> +
>> static int make_root_properties(libxl__gc *gc,
>>                                 const libxl_version_info *vers,
>>                                 void *fdt)
>> @@ -668,6 +720,57 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
>>     return 0;
>> }
>> 
>> +static int make_vpci_node(libxl__gc *gc, void *fdt,
>> +        const struct arch_info *ainfo,
>> +        struct xc_dom_image *dom)
>> +{
>> +    int res;
>> +    const uint64_t vpci_ecam_base = GUEST_VPCI_ECAM_BASE;
>> +    const uint64_t vpci_ecam_size = GUEST_VPCI_ECAM_SIZE;
>> +    const char *name = GCSPRINTF("pcie@%"PRIx64, vpci_ecam_base);
>> +
>> +    res = fdt_begin_node(fdt, name);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_compat(gc, fdt, 1, "pci-host-ecam-generic");
>> +    if (res) return res;
>> +
>> +    res = fdt_property_string(fdt, "device_type", "pci");
>> +    if (res) return res;
>> +
>> +    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>> +            GUEST_ROOT_SIZE_CELLS, 1, vpci_ecam_base, vpci_ecam_size);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_values(gc, fdt, "bus-range", 2, 0,17);
>                                                            ^ space
> 
Ack. 
> 
>> +    if (res) return res;
>> +
>> +    res = fdt_property_cell(fdt, "#address-cells", 3);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_cell(fdt, "#size-cells", 2);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_string(fdt, "status", "okay");
>> +    if (res) return res;
>> +
>> +    res = fdt_property_vpci_ranges(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>> +        GUEST_ROOT_SIZE_CELLS, 2,
>> +        GUEST_VPCI_ADDR_TYPE_MEM, GUEST_VPCI_MEM_ADDR, GUEST_VPCI_MEM_SIZE,
>> +        GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM, GUEST_VPCI_PREFETCH_MEM_ADDR,
>> +        GUEST_VPCI_PREFETCH_MEM_SIZE);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_values(gc, fdt, "msi-map", 4, 0, GUEST_PHANDLE_ITS,
>> +                              0, 0x10000);
>> +    if (res) return res;
> 
> I agree with Julien that we shouldn't add it now if it is not working.
Ack.
> 
> One question: what about legacy interrupts? If they are supported,
> shouldn't we have interrupts and interrupt-parent properties?

As per current design legacy interrupt will not be supported.
> 
> 
>> +    res = fdt_end_node(fdt);
>> +    if (res) return res;
>> +
>> +    return 0;
>> +}
>> +
>> static const struct arch_info *get_arch_info(libxl__gc *gc,
>>                                              const struct xc_dom_image *dom)
>> {
>> @@ -971,6 +1074,9 @@ next_resize:
>>         if (info->tee == LIBXL_TEE_TYPE_OPTEE)
>>             FDT( make_optee_node(gc, fdt) );
>> 
>> +        if (libxl_defbool_val(info->arch_arm.vpci))
>> +            FDT( make_vpci_node(gc, fdt, ainfo, dom) );
>> +
>>         if (pfdt)
>>             FDT( copy_partial_fdt(gc, fdt, pfdt) );
>> 
>> @@ -1189,6 +1295,9 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>>     /* ACPI is disabled by default */
>>     libxl_defbool_setdefault(&b_info->acpi, false);
>> 
>> +    /* VPCI is disabled by default */
>> +    libxl_defbool_setdefault(&b_info->arch_arm.vpci, false);
>> +
>>     if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
>>         return;
>> 
>> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index 3f9fff653a..78b1ddf0b8 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>> 
>>     ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>                                ("vuart", libxl_vuart_type),
>> +                               ("vpci", libxl_defbool),
>>                               ])),
>>     ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>                               ])),
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 17dddb4cd5..ffafbeffb4 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -1497,6 +1497,8 @@ void parse_config_data(const char *config_source,
>>         }
>>         if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
>>             libxl_defbool_set(&b_info->u.pv.e820_host, true);
>> +        if (d_config->num_pcidevs)
>> +            libxl_defbool_set(&b_info->arch_arm.vpci, true);
>>     }
>> 
>>     if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 0a9749e768..01d13e669e 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -426,6 +426,17 @@ typedef uint64_t xen_callback_t;
>> #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
>> #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
>> 
>> +/* PCI-PCIe memory space types */
>> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x42000000)
>> +#define GUEST_VPCI_ADDR_TYPE_MEM          xen_mk_ullong(0x02000000)
>> +
>> +/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
>> +#define GUEST_VPCI_PREFETCH_MEM_ADDR  xen_mk_ullong(0x900000000)
>> +#define GUEST_VPCI_MEM_ADDR           xen_mk_ullong(0x20000000)
>> +
>> +#define GUEST_VPCI_PREFETCH_MEM_SIZE      xen_mk_ullong(0x2000000000)
>> +#define GUEST_VPCI_MEM_SIZE               xen_mk_ullong(0x04000000)
> 
> Now I get it: GUEST_VPCI_ECAM_BASE was just for the config space, not
> the aperture. The apertures are starting at 0x20000000 and 0x900000000.
Yes right.
> 
> Speaking of which, isn't GUEST_VPCI_MEM_SIZE a bit too small? There
> could be PCI devices with a <4GB requirement. Maybe 256MB?

Yes I will allocate the 256 MB.

Regards,
Rahul

Hi Rahul,

On 19/08/2021 13:02, Rahul Singh wrote:
> libxl will create an emulated PCI device tree node in the device tree to
> enable the guest OS to discover the virtual PCI during guest boot.
> Emulated PCI device tree node will only be created when there is any
> device assigned to guest.
> 
> A new area has been reserved in the arm guest physical map at
> which the VPCI bus is declared in the device tree (reg and ranges
> parameters of the node).
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
>   tools/libs/light/libxl_arm.c          | 109 ++++++++++++++++++++++++++
>   tools/libs/light/libxl_types.idl      |   1 +
>   tools/xl/xl_parse.c                   |   2 +
>   xen/include/public/arch-arm.h         |  11 +++
>   xen/include/public/device_tree_defs.h |   1 +
>   5 files changed, 124 insertions(+)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6e00..a091e97e76 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -269,6 +269,58 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
>       return fdt_property(fdt, "reg", regs, sizeof(regs));
>   }
>   
> +static int fdt_property_values(libxl__gc *gc, void *fdt,
> +        const char *name, unsigned num_cells, ...)
> +{
> +    uint32_t prop[num_cells];
> +    be32 *cells = &prop[0];
> +    int i;
> +    va_list ap;
> +    uint32_t arg;
> +
> +    va_start(ap, num_cells);
> +    for (i = 0 ; i < num_cells; i++) {
> +        arg = va_arg(ap, uint32_t);
> +        set_cell(&cells, 1, arg);
> +    }
> +    va_end(ap);
> +
> +    return fdt_property(fdt, name, prop, sizeof(prop));
> +}
> +
> +static int fdt_property_vpci_ranges(libxl__gc *gc, void *fdt,
> +                                    unsigned addr_cells,
> +                                    unsigned size_cells,
> +                                    unsigned num_regs, ...)
> +{
> +    uint32_t regs[num_regs*((addr_cells*2)+size_cells+1)];
> +    be32 *cells = &regs[0];
> +    int i;
> +    va_list ap;
> +    uint64_t arg;
> +
> +    va_start(ap, num_regs);
> +    for (i = 0 ; i < num_regs; i++) {
> +        /* Set the memory bit field */
> +        arg = va_arg(ap, uint64_t);
> +        set_cell(&cells, 1, arg);
> +
> +        /* Set the vpci bus address */
> +        arg = addr_cells ? va_arg(ap, uint64_t) : 0;
> +        set_cell(&cells, addr_cells , arg);
> +
> +        /* Set the cpu bus address where vpci address is mapped */
> +        set_cell(&cells, addr_cells, arg);
> +
> +        /* Set the vpci size requested */
> +        arg = size_cells ? va_arg(ap, uint64_t) : 0;
> +        set_cell(&cells, size_cells,arg);
> +    }
> +    va_end(ap);
> +
> +    return fdt_property(fdt, "ranges", regs, sizeof(regs));
> +}
> +
>   static int make_root_properties(libxl__gc *gc,
>                                   const libxl_version_info *vers,
>                                   void *fdt)
> @@ -668,6 +720,57 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
>       return 0;
>   }
>   
> +static int make_vpci_node(libxl__gc *gc, void *fdt,
> +        const struct arch_info *ainfo,
> +        struct xc_dom_image *dom)
> +{
> +    int res;
> +    const uint64_t vpci_ecam_base = GUEST_VPCI_ECAM_BASE;
> +    const uint64_t vpci_ecam_size = GUEST_VPCI_ECAM_SIZE;
> +    const char *name = GCSPRINTF("pcie@%"PRIx64, vpci_ecam_base);
> +
> +    res = fdt_begin_node(fdt, name);
> +    if (res) return res;
> +
> +    res = fdt_property_compat(gc, fdt, 1, "pci-host-ecam-generic");
> +    if (res) return res;
> +
> +    res = fdt_property_string(fdt, "device_type", "pci");
> +    if (res) return res;
> +
> +    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> +            GUEST_ROOT_SIZE_CELLS, 1, vpci_ecam_base, vpci_ecam_size);
> +    if (res) return res;
> +
> +    res = fdt_property_values(gc, fdt, "bus-range", 2, 0,17);

AFAICT, the "bus-range" is optional. Can you explain why we need it?

> +    if (res) return res;
> +
> +    res = fdt_property_cell(fdt, "#address-cells", 3);
> +    if (res) return res;
> +
> +    res = fdt_property_cell(fdt, "#size-cells", 2);
> +    if (res) return res;
> +
> +    res = fdt_property_string(fdt, "status", "okay");
> +    if (res) return res;
> +
> +    res = fdt_property_vpci_ranges(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
> +        GUEST_ROOT_SIZE_CELLS, 2,
> +        GUEST_VPCI_ADDR_TYPE_MEM, GUEST_VPCI_MEM_ADDR, GUEST_VPCI_MEM_SIZE,
> +        GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM, GUEST_VPCI_PREFETCH_MEM_ADDR,
> +        GUEST_VPCI_PREFETCH_MEM_SIZE);
> +    if (res) return res;
> +
> +    res = fdt_property_values(gc, fdt, "msi-map", 4, 0, GUEST_PHANDLE_ITS,
> +                              0, 0x10000);

The ITS is not exposed to the guest so far. So I think this wants to be 
moved to the future series that will add ITS support.

> +    if (res) return res;
> +
> +    res = fdt_end_node(fdt);
> +    if (res) return res;
> +
> +    return 0;
> +}
> +
>   static const struct arch_info *get_arch_info(libxl__gc *gc,
>                                                const struct xc_dom_image *dom)
>   {
> @@ -971,6 +1074,9 @@ next_resize:
>           if (info->tee == LIBXL_TEE_TYPE_OPTEE)
>               FDT( make_optee_node(gc, fdt) );
>   
> +        if (libxl_defbool_val(info->arch_arm.vpci))
> +            FDT( make_vpci_node(gc, fdt, ainfo, dom) );
> +
>           if (pfdt)
>               FDT( copy_partial_fdt(gc, fdt, pfdt) );
>   
> @@ -1189,6 +1295,9 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>       /* ACPI is disabled by default */
>       libxl_defbool_setdefault(&b_info->acpi, false);
>   
> +    /* VPCI is disabled by default */
> +    libxl_defbool_setdefault(&b_info->arch_arm.vpci, false);

Here you say that vPCI is disabled by default. However, AFAICT, in the 
previous patch you will unconditionally enable vPCI if Xen has been 
built with VPCI.

Rather than unconditionally enable VPCI for a domain, I think we should 
introduce a new flag that is passed to Xen at the domain creation. The 
flag will be set based on arch_arm.vpci.

> +
>       if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
>           return;
>   
> diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
> index 3f9fff653a..78b1ddf0b8 100644
> --- a/tools/libs/light/libxl_types.idl
> +++ b/tools/libs/light/libxl_types.idl
> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>   
>       ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>                                  ("vuart", libxl_vuart_type),
> +                               ("vpci", libxl_defbool),

Any new addition in the structure should be accompanied with a 
LIBXL_HAVE_* in the libxl.h header.

>                                 ])),
>       ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>                                 ])),
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index 17dddb4cd5..ffafbeffb4 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1497,6 +1497,8 @@ void parse_config_data(const char *config_source,
>           }
>           if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
>               libxl_defbool_set(&b_info->u.pv.e820_host, true);
> +        if (d_config->num_pcidevs)
> +            libxl_defbool_set(&b_info->arch_arm.vpci, true);
>       }
>   
>       if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
> index 0a9749e768..01d13e669e 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -426,6 +426,17 @@ typedef uint64_t xen_callback_t;
>   #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
>   #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
>   
> +/* PCI-PCIe memory space types */
> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x42000000)
> +#define GUEST_VPCI_ADDR_TYPE_MEM          xen_mk_ullong(0x02000000)

What the size of those regions?

> +
> +/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
> +#define GUEST_VPCI_PREFETCH_MEM_ADDR  xen_mk_ullong(0x900000000)
> +#define GUEST_VPCI_MEM_ADDR           xen_mk_ullong(0x20000000)
So far the memory layout defines the address in ascending order. So 
please add that after GUEST_RAM_BANK_BASES_*.

However, if I am not mistaken that base address you provide will clash 
with RAM bank 1. It also seem to be pretty high which means that this 
will not work for 32-bit domain or on CPUs that don't allow offer large 
IPA bits.

I think we need to start making the guest layout more dynamic. The VPCI 
memory space would have to go right after the end of the RAM allocated 
for a given guest.

> +
> +#define GUEST_VPCI_PREFETCH_MEM_SIZE      xen_mk_ullong(0x2000000000)
> +#define GUEST_VPCI_MEM_SIZE               xen_mk_ullong(0x04000000)

It would be better if the size for each region is defined right after 
each base.

Also, how did you decide the size of each region?

> +
>   /*
>    * 16MB == 4096 pages reserved for guest to use as a region to map its
>    * grant table in.
> diff --git a/xen/include/public/device_tree_defs.h b/xen/include/public/device_tree_defs.h
> index 209d43de3f..d24d86b1fc 100644
> --- a/xen/include/public/device_tree_defs.h
> +++ b/xen/include/public/device_tree_defs.h
> @@ -7,6 +7,7 @@
>    * onwards. Reserve a high value for the GIC phandle.
>    */
>   #define GUEST_PHANDLE_GIC (65000)
> +#define GUEST_PHANDLE_ITS (64999)
>   
>   #define GUEST_ROOT_ADDRESS_CELLS 2
>   #define GUEST_ROOT_SIZE_CELLS 2
> 

Cheers,

-- 
Julien Grall

Hi Julien,

> On 19 Aug 2021, at 2:00 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 19/08/2021 13:02, Rahul Singh wrote:
>> libxl will create an emulated PCI device tree node in the device tree to
>> enable the guest OS to discover the virtual PCI during guest boot.
>> Emulated PCI device tree node will only be created when there is any
>> device assigned to guest.
>> A new area has been reserved in the arm guest physical map at
>> which the VPCI bus is declared in the device tree (reg and ranges
>> parameters of the node).
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>>  tools/libs/light/libxl_arm.c          | 109 ++++++++++++++++++++++++++
>>  tools/libs/light/libxl_types.idl      |   1 +
>>  tools/xl/xl_parse.c                   |   2 +
>>  xen/include/public/arch-arm.h         |  11 +++
>>  xen/include/public/device_tree_defs.h |   1 +
>>  5 files changed, 124 insertions(+)
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index e3140a6e00..a091e97e76 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -269,6 +269,58 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
>>      return fdt_property(fdt, "reg", regs, sizeof(regs));
>>  }
>>  +static int fdt_property_values(libxl__gc *gc, void *fdt,
>> +        const char *name, unsigned num_cells, ...)
>> +{
>> +    uint32_t prop[num_cells];
>> +    be32 *cells = &prop[0];
>> +    int i;
>> +    va_list ap;
>> +    uint32_t arg;
>> +
>> +    va_start(ap, num_cells);
>> +    for (i = 0 ; i < num_cells; i++) {
>> +        arg = va_arg(ap, uint32_t);
>> +        set_cell(&cells, 1, arg);
>> +    }
>> +    va_end(ap);
>> +
>> +    return fdt_property(fdt, name, prop, sizeof(prop));
>> +}
>> +
>> +static int fdt_property_vpci_ranges(libxl__gc *gc, void *fdt,
>> +                                    unsigned addr_cells,
>> +                                    unsigned size_cells,
>> +                                    unsigned num_regs, ...)
>> +{
>> +    uint32_t regs[num_regs*((addr_cells*2)+size_cells+1)];
>> +    be32 *cells = &regs[0];
>> +    int i;
>> +    va_list ap;
>> +    uint64_t arg;
>> +
>> +    va_start(ap, num_regs);
>> +    for (i = 0 ; i < num_regs; i++) {
>> +        /* Set the memory bit field */
>> +        arg = va_arg(ap, uint64_t);
>> +        set_cell(&cells, 1, arg);
>> +
>> +        /* Set the vpci bus address */
>> +        arg = addr_cells ? va_arg(ap, uint64_t) : 0;
>> +        set_cell(&cells, addr_cells , arg);
>> +
>> +        /* Set the cpu bus address where vpci address is mapped */
>> +        set_cell(&cells, addr_cells, arg);
>> +
>> +        /* Set the vpci size requested */
>> +        arg = size_cells ? va_arg(ap, uint64_t) : 0;
>> +        set_cell(&cells, size_cells,arg);
>> +    }
>> +    va_end(ap);
>> +
>> +    return fdt_property(fdt, "ranges", regs, sizeof(regs));
>> +}
>> +
>>  static int make_root_properties(libxl__gc *gc,
>>                                  const libxl_version_info *vers,
>>                                  void *fdt)
>> @@ -668,6 +720,57 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
>>      return 0;
>>  }
>>  +static int make_vpci_node(libxl__gc *gc, void *fdt,
>> +        const struct arch_info *ainfo,
>> +        struct xc_dom_image *dom)
>> +{
>> +    int res;
>> +    const uint64_t vpci_ecam_base = GUEST_VPCI_ECAM_BASE;
>> +    const uint64_t vpci_ecam_size = GUEST_VPCI_ECAM_SIZE;
>> +    const char *name = GCSPRINTF("pcie@%"PRIx64, vpci_ecam_base);
>> +
>> +    res = fdt_begin_node(fdt, name);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_compat(gc, fdt, 1, "pci-host-ecam-generic");
>> +    if (res) return res;
>> +
>> +    res = fdt_property_string(fdt, "device_type", "pci");
>> +    if (res) return res;
>> +
>> +    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>> +            GUEST_ROOT_SIZE_CELLS, 1, vpci_ecam_base, vpci_ecam_size);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_values(gc, fdt, "bus-range", 2, 0,17);
> 
> AFAICT, the "bus-range" is optional. Can you explain why we need it?

We need it to implement the function pci_ecam_map_bus().
> 
>> +    if (res) return res;
>> +
>> +    res = fdt_property_cell(fdt, "#address-cells", 3);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_cell(fdt, "#size-cells", 2);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_string(fdt, "status", "okay");
>> +    if (res) return res;
>> +
>> +    res = fdt_property_vpci_ranges(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>> +        GUEST_ROOT_SIZE_CELLS, 2,
>> +        GUEST_VPCI_ADDR_TYPE_MEM, GUEST_VPCI_MEM_ADDR, GUEST_VPCI_MEM_SIZE,
>> +        GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM, GUEST_VPCI_PREFETCH_MEM_ADDR,
>> +        GUEST_VPCI_PREFETCH_MEM_SIZE);
>> +    if (res) return res;
>> +
>> +    res = fdt_property_values(gc, fdt, "msi-map", 4, 0, GUEST_PHANDLE_ITS,
>> +                              0, 0x10000);
> 
> The ITS is not exposed to the guest so far. So I think this wants to be moved to the future series that will add ITS support.

Ok. I will remove this in next version.

>> +    if (res) return res;
>> +
>> +    res = fdt_end_node(fdt);
>> +    if (res) return res;
>> +
>> +    return 0;
>> +}
>> +
>>  static const struct arch_info *get_arch_info(libxl__gc *gc,
>>                                               const struct xc_dom_image *dom)
>>  {
>> @@ -971,6 +1074,9 @@ next_resize:
>>          if (info->tee == LIBXL_TEE_TYPE_OPTEE)
>>              FDT( make_optee_node(gc, fdt) );
>>  +        if (libxl_defbool_val(info->arch_arm.vpci))
>> +            FDT( make_vpci_node(gc, fdt, ainfo, dom) );
>> +
>>          if (pfdt)
>>              FDT( copy_partial_fdt(gc, fdt, pfdt) );
>>  @@ -1189,6 +1295,9 @@ void libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>>      /* ACPI is disabled by default */
>>      libxl_defbool_setdefault(&b_info->acpi, false);
>>  +    /* VPCI is disabled by default */
>> +    libxl_defbool_setdefault(&b_info->arch_arm.vpci, false);
> 
> Here you say that vPCI is disabled by default. However, AFAICT, in the previous patch you will unconditionally enable vPCI if Xen has been built with VPCI.
> 
> Rather than unconditionally enable VPCI for a domain, I think we should introduce a new flag that is passed to Xen at the domain creation. The flag will be set based on arch_arm.vpci.

OK. I will introduce the flag in next version.

> 
>> +
>>      if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
>>          return;
>>  diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>> index 3f9fff653a..78b1ddf0b8 100644
>> --- a/tools/libs/light/libxl_types.idl
>> +++ b/tools/libs/light/libxl_types.idl
>> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>        ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>                                 ("vuart", libxl_vuart_type),
>> +                               ("vpci", libxl_defbool),
> 
> Any new addition in the structure should be accompanied with a LIBXL_HAVE_* in the libxl.h header.

OK.
> 
>>                                ])),
>>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>                                ])),
>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>> index 17dddb4cd5..ffafbeffb4 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -1497,6 +1497,8 @@ void parse_config_data(const char *config_source,
>>          }
>>          if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
>>              libxl_defbool_set(&b_info->u.pv.e820_host, true);
>> +        if (d_config->num_pcidevs)
>> +            libxl_defbool_set(&b_info->arch_arm.vpci, true);
>>      }
>>        if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>> index 0a9749e768..01d13e669e 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -426,6 +426,17 @@ typedef uint64_t xen_callback_t;
>>  #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
>>  #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
>>  +/* PCI-PCIe memory space types */
>> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x42000000)
>> +#define GUEST_VPCI_ADDR_TYPE_MEM          xen_mk_ullong(0x02000000)
> 
> What the size of those regions?

Non Prefetch Memory: Size 64 MB start at 512 MB
Prefetch Memory: Size 128 GB start at 36 GB
> 
>> +
>> +/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
>> +#define GUEST_VPCI_PREFETCH_MEM_ADDR  xen_mk_ullong(0x900000000)
>> +#define GUEST_VPCI_MEM_ADDR           xen_mk_ullong(0x20000000)
> So far the memory layout defines the address in ascending order. So please add that after GUEST_RAM_BANK_BASES_*.

Ok.
> 
> However, if I am not mistaken that base address you provide will clash with RAM bank 1. It also seem to be pretty high which means that this will not work for 32-bit domain or on CPUs that don't allow offer large IPA bits.

Yes I also checked that now that it is having clash with RAM bank 1.
There is unused space is guest memory that we can use for Non Prefetch Memory as per below guest memory map.
https://gitlab.com/xen-project/fusa/fusa-docs/-/blob/master/high-level/guest-memory-layout-arm.rst

Proposed value:
Non Prefetch Memory: Size 64 MB start at 0x22001000
Prefetch Memory: Size 4 GB start at 4 GB.

> 
> I think we need to start making the guest layout more dynamic. The VPCI memory space would have to go right after the end of the RAM allocated for a given guest.
> 
>> +
>> +#define GUEST_VPCI_PREFETCH_MEM_SIZE      xen_mk_ullong(0x2000000000)
>> +#define GUEST_VPCI_MEM_SIZE               xen_mk_ullong(0x04000000)
> 
> It would be better if the size for each region is defined right after each base.
OK. 

> 
> Also, how did you decide the size of each region?

I thought 64 MB will be sufficient. I think it should be based on number of devices we can assign to the guest. 

Regards,
Rahul

> 
>> +
>>  /*
>>   * 16MB == 4096 pages reserved for guest to use as a region to map its
>>   * grant table in.
>> diff --git a/xen/include/public/device_tree_defs.h b/xen/include/public/device_tree_defs.h
>> index 209d43de3f..d24d86b1fc 100644
>> --- a/xen/include/public/device_tree_defs.h
>> +++ b/xen/include/public/device_tree_defs.h
>> @@ -7,6 +7,7 @@
>>   * onwards. Reserve a high value for the GIC phandle.
>>   */
>>  #define GUEST_PHANDLE_GIC (65000)
>> +#define GUEST_PHANDLE_ITS (64999)
>>    #define GUEST_ROOT_ADDRESS_CELLS 2
>>  #define GUEST_ROOT_SIZE_CELLS 2
> 
> Cheers,
> 
> -- 
> Julien Grall

On 20/08/2021 17:03, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

>> On 19 Aug 2021, at 2:00 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi Rahul,
>>
>> On 19/08/2021 13:02, Rahul Singh wrote:
>>> libxl will create an emulated PCI device tree node in the device tree to
>>> enable the guest OS to discover the virtual PCI during guest boot.
>>> Emulated PCI device tree node will only be created when there is any
>>> device assigned to guest.
>>> A new area has been reserved in the arm guest physical map at
>>> which the VPCI bus is declared in the device tree (reg and ranges
>>> parameters of the node).
>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>>> ---
>>>   tools/libs/light/libxl_arm.c          | 109 ++++++++++++++++++++++++++
>>>   tools/libs/light/libxl_types.idl      |   1 +
>>>   tools/xl/xl_parse.c                   |   2 +
>>>   xen/include/public/arch-arm.h         |  11 +++
>>>   xen/include/public/device_tree_defs.h |   1 +
>>>   5 files changed, 124 insertions(+)
>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>> index e3140a6e00..a091e97e76 100644
>>> --- a/tools/libs/light/libxl_arm.c
>>> +++ b/tools/libs/light/libxl_arm.c
>>> @@ -269,6 +269,58 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
>>>       return fdt_property(fdt, "reg", regs, sizeof(regs));
>>>   }
>>>   +static int fdt_property_values(libxl__gc *gc, void *fdt,
>>> +        const char *name, unsigned num_cells, ...)
>>> +{
>>> +    uint32_t prop[num_cells];
>>> +    be32 *cells = &prop[0];
>>> +    int i;
>>> +    va_list ap;
>>> +    uint32_t arg;
>>> +
>>> +    va_start(ap, num_cells);
>>> +    for (i = 0 ; i < num_cells; i++) {
>>> +        arg = va_arg(ap, uint32_t);
>>> +        set_cell(&cells, 1, arg);
>>> +    }
>>> +    va_end(ap);
>>> +
>>> +    return fdt_property(fdt, name, prop, sizeof(prop));
>>> +}
>>> +
>>> +static int fdt_property_vpci_ranges(libxl__gc *gc, void *fdt,
>>> +                                    unsigned addr_cells,
>>> +                                    unsigned size_cells,
>>> +                                    unsigned num_regs, ...)
>>> +{
>>> +    uint32_t regs[num_regs*((addr_cells*2)+size_cells+1)];
>>> +    be32 *cells = &regs[0];
>>> +    int i;
>>> +    va_list ap;
>>> +    uint64_t arg;
>>> +
>>> +    va_start(ap, num_regs);
>>> +    for (i = 0 ; i < num_regs; i++) {
>>> +        /* Set the memory bit field */
>>> +        arg = va_arg(ap, uint64_t);
>>> +        set_cell(&cells, 1, arg);
>>> +
>>> +        /* Set the vpci bus address */
>>> +        arg = addr_cells ? va_arg(ap, uint64_t) : 0;
>>> +        set_cell(&cells, addr_cells , arg);
>>> +
>>> +        /* Set the cpu bus address where vpci address is mapped */
>>> +        set_cell(&cells, addr_cells, arg);
>>> +
>>> +        /* Set the vpci size requested */
>>> +        arg = size_cells ? va_arg(ap, uint64_t) : 0;
>>> +        set_cell(&cells, size_cells,arg);
>>> +    }
>>> +    va_end(ap);
>>> +
>>> +    return fdt_property(fdt, "ranges", regs, sizeof(regs));
>>> +}
>>> +
>>>   static int make_root_properties(libxl__gc *gc,
>>>                                   const libxl_version_info *vers,
>>>                                   void *fdt)
>>> @@ -668,6 +720,57 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
>>>       return 0;
>>>   }
>>>   +static int make_vpci_node(libxl__gc *gc, void *fdt,
>>> +        const struct arch_info *ainfo,
>>> +        struct xc_dom_image *dom)
>>> +{
>>> +    int res;
>>> +    const uint64_t vpci_ecam_base = GUEST_VPCI_ECAM_BASE;
>>> +    const uint64_t vpci_ecam_size = GUEST_VPCI_ECAM_SIZE;
>>> +    const char *name = GCSPRINTF("pcie@%"PRIx64, vpci_ecam_base);
>>> +
>>> +    res = fdt_begin_node(fdt, name);
>>> +    if (res) return res;
>>> +
>>> +    res = fdt_property_compat(gc, fdt, 1, "pci-host-ecam-generic");
>>> +    if (res) return res;
>>> +
>>> +    res = fdt_property_string(fdt, "device_type", "pci");
>>> +    if (res) return res;
>>> +
>>> +    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>>> +            GUEST_ROOT_SIZE_CELLS, 1, vpci_ecam_base, vpci_ecam_size);
>>> +    if (res) return res;
>>> +
>>> +    res = fdt_property_values(gc, fdt, "bus-range", 2, 0,17);
>>
>> AFAICT, the "bus-range" is optional. Can you explain why we need it?
> 
> We need it to implement the function pci_ecam_map_bus().

Ok. Then why next question is what does the 17 mean? Is it tie to how we
implement the vPCI in Xen or the region we reserved?

[...]

>>
>>> +
>>>       if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
>>>           return;
>>>   diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>>> index 3f9fff653a..78b1ddf0b8 100644
>>> --- a/tools/libs/light/libxl_types.idl
>>> +++ b/tools/libs/light/libxl_types.idl
>>> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>>         ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>>                                  ("vuart", libxl_vuart_type),
>>> +                               ("vpci", libxl_defbool),
>>
>> Any new addition in the structure should be accompanied with a LIBXL_HAVE_* in the libxl.h header.
> 
> OK.
>>
>>>                                 ])),
>>>       ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>>                                 ])),
>>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>>> index 17dddb4cd5..ffafbeffb4 100644
>>> --- a/tools/xl/xl_parse.c
>>> +++ b/tools/xl/xl_parse.c
>>> @@ -1497,6 +1497,8 @@ void parse_config_data(const char *config_source,
>>>           }
>>>           if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
>>>               libxl_defbool_set(&b_info->u.pv.e820_host, true);
>>> +        if (d_config->num_pcidevs)
>>> +            libxl_defbool_set(&b_info->arch_arm.vpci, true);
>>>       }
>>>         if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>> index 0a9749e768..01d13e669e 100644
>>> --- a/xen/include/public/arch-arm.h
>>> +++ b/xen/include/public/arch-arm.h
>>> @@ -426,6 +426,17 @@ typedef uint64_t xen_callback_t;
>>>   #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
>>>   #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
>>>   +/* PCI-PCIe memory space types */
>>> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x42000000)
>>> +#define GUEST_VPCI_ADDR_TYPE_MEM          xen_mk_ullong(0x02000000)
>>
>> What the size of those regions?
> 
> Non Prefetch Memory: Size 64 MB start at 512 MB
> Prefetch Memory: Size 128 GB start at 36 GB
>>
>>> +
>>> +/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
>>> +#define GUEST_VPCI_PREFETCH_MEM_ADDR  xen_mk_ullong(0x900000000)
>>> +#define GUEST_VPCI_MEM_ADDR           xen_mk_ullong(0x20000000)
>> So far the memory layout defines the address in ascending order. So please add that after GUEST_RAM_BANK_BASES_*.
> 
> Ok.
>>
>> However, if I am not mistaken that base address you provide will clash with RAM bank 1. It also seem to be pretty high which means that this will not work for 32-bit domain or on CPUs that don't allow offer large IPA bits.
> 
> Yes I also checked that now that it is having clash with RAM bank 1.
> There is unused space is guest memory that we can use for Non Prefetch Memory as per below guest memory map.
> https://gitlab.com/xen-project/fusa/fusa-docs/-/blob/master/high-level/guest-memory-layout-arm.rst
> 
> Proposed value:
> Non Prefetch Memory: Size 64 MB start at 0x22001000
> Prefetch Memory: Size 4 GB start at 4 GB.

The base address looks fine to me. However, the sizes are much smaller 
to what you initially suggested. Would you be able to clarify why the 
smaller sizes are fine?

> 
>>
>> I think we need to start making the guest layout more dynamic. The VPCI memory space would have to go right after the end of the RAM allocated for a given guest.
>>
>>> +
>>> +#define GUEST_VPCI_PREFETCH_MEM_SIZE      xen_mk_ullong(0x2000000000)
>>> +#define GUEST_VPCI_MEM_SIZE               xen_mk_ullong(0x04000000)
>>
>> It would be better if the size for each region is defined right after each base.
> OK.
> 
>>
>> Also, how did you decide the size of each region?
> 
> I thought 64 MB will be sufficient. I think it should be based on number of devices we can assign to the guest.

We don't have to get the size right now. What I am more interested is to 
have a trace about how those values were decided (even if it just saying 
random). This will help to make any decision if in the future we need to 
resize (in particular downsize) the regions.

Cheers,

-- 
Julien Grall

Hi Julien,

> On 9 Sep 2021, at 2:59 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 20/08/2021 17:03, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>>> On 19 Aug 2021, at 2:00 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi Rahul,
>>> 
>>> On 19/08/2021 13:02, Rahul Singh wrote:
>>>> libxl will create an emulated PCI device tree node in the device tree to
>>>> enable the guest OS to discover the virtual PCI during guest boot.
>>>> Emulated PCI device tree node will only be created when there is any
>>>> device assigned to guest.
>>>> A new area has been reserved in the arm guest physical map at
>>>> which the VPCI bus is declared in the device tree (reg and ranges
>>>> parameters of the node).
>>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>>>> ---
>>>>  tools/libs/light/libxl_arm.c          | 109 ++++++++++++++++++++++++++
>>>>  tools/libs/light/libxl_types.idl      |   1 +
>>>>  tools/xl/xl_parse.c                   |   2 +
>>>>  xen/include/public/arch-arm.h         |  11 +++
>>>>  xen/include/public/device_tree_defs.h |   1 +
>>>>  5 files changed, 124 insertions(+)
>>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>>> index e3140a6e00..a091e97e76 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -269,6 +269,58 @@ static int fdt_property_regs(libxl__gc *gc, void *fdt,
>>>>      return fdt_property(fdt, "reg", regs, sizeof(regs));
>>>>  }
>>>>  +static int fdt_property_values(libxl__gc *gc, void *fdt,
>>>> +        const char *name, unsigned num_cells, ...)
>>>> +{
>>>> +    uint32_t prop[num_cells];
>>>> +    be32 *cells = &prop[0];
>>>> +    int i;
>>>> +    va_list ap;
>>>> +    uint32_t arg;
>>>> +
>>>> +    va_start(ap, num_cells);
>>>> +    for (i = 0 ; i < num_cells; i++) {
>>>> +        arg = va_arg(ap, uint32_t);
>>>> +        set_cell(&cells, 1, arg);
>>>> +    }
>>>> +    va_end(ap);
>>>> +
>>>> +    return fdt_property(fdt, name, prop, sizeof(prop));
>>>> +}
>>>> +
>>>> +static int fdt_property_vpci_ranges(libxl__gc *gc, void *fdt,
>>>> +                                    unsigned addr_cells,
>>>> +                                    unsigned size_cells,
>>>> +                                    unsigned num_regs, ...)
>>>> +{
>>>> +    uint32_t regs[num_regs*((addr_cells*2)+size_cells+1)];
>>>> +    be32 *cells = &regs[0];
>>>> +    int i;
>>>> +    va_list ap;
>>>> +    uint64_t arg;
>>>> +
>>>> +    va_start(ap, num_regs);
>>>> +    for (i = 0 ; i < num_regs; i++) {
>>>> +        /* Set the memory bit field */
>>>> +        arg = va_arg(ap, uint64_t);
>>>> +        set_cell(&cells, 1, arg);
>>>> +
>>>> +        /* Set the vpci bus address */
>>>> +        arg = addr_cells ? va_arg(ap, uint64_t) : 0;
>>>> +        set_cell(&cells, addr_cells , arg);
>>>> +
>>>> +        /* Set the cpu bus address where vpci address is mapped */
>>>> +        set_cell(&cells, addr_cells, arg);
>>>> +
>>>> +        /* Set the vpci size requested */
>>>> +        arg = size_cells ? va_arg(ap, uint64_t) : 0;
>>>> +        set_cell(&cells, size_cells,arg);
>>>> +    }
>>>> +    va_end(ap);
>>>> +
>>>> +    return fdt_property(fdt, "ranges", regs, sizeof(regs));
>>>> +}
>>>> +
>>>>  static int make_root_properties(libxl__gc *gc,
>>>>                                  const libxl_version_info *vers,
>>>>                                  void *fdt)
>>>> @@ -668,6 +720,57 @@ static int make_vpl011_uart_node(libxl__gc *gc, void *fdt,
>>>>      return 0;
>>>>  }
>>>>  +static int make_vpci_node(libxl__gc *gc, void *fdt,
>>>> +        const struct arch_info *ainfo,
>>>> +        struct xc_dom_image *dom)
>>>> +{
>>>> +    int res;
>>>> +    const uint64_t vpci_ecam_base = GUEST_VPCI_ECAM_BASE;
>>>> +    const uint64_t vpci_ecam_size = GUEST_VPCI_ECAM_SIZE;
>>>> +    const char *name = GCSPRINTF("pcie@%"PRIx64, vpci_ecam_base);
>>>> +
>>>> +    res = fdt_begin_node(fdt, name);
>>>> +    if (res) return res;
>>>> +
>>>> +    res = fdt_property_compat(gc, fdt, 1, "pci-host-ecam-generic");
>>>> +    if (res) return res;
>>>> +
>>>> +    res = fdt_property_string(fdt, "device_type", "pci");
>>>> +    if (res) return res;
>>>> +
>>>> +    res = fdt_property_regs(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>>>> +            GUEST_ROOT_SIZE_CELLS, 1, vpci_ecam_base, vpci_ecam_size);
>>>> +    if (res) return res;
>>>> +
>>>> +    res = fdt_property_values(gc, fdt, "bus-range", 2, 0,17);
>>> 
>>> AFAICT, the "bus-range" is optional. Can you explain why we need it?
>> We need it to implement the function pci_ecam_map_bus().
> 
> Ok. Then why next question is what does the 17 mean? Is it tie to how we
> implement the vPCI in Xen or the region we reserved?
> 

Ah. I forgot to change it to 0xff. It should be 0xff. 

> [...]
> 
>>> 
>>>> +
>>>>      if (b_info->type != LIBXL_DOMAIN_TYPE_PV)
>>>>          return;
>>>>  diff --git a/tools/libs/light/libxl_types.idl b/tools/libs/light/libxl_types.idl
>>>> index 3f9fff653a..78b1ddf0b8 100644
>>>> --- a/tools/libs/light/libxl_types.idl
>>>> +++ b/tools/libs/light/libxl_types.idl
>>>> @@ -644,6 +644,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>>>>        ("arch_arm", Struct(None, [("gic_version", libxl_gic_version),
>>>>                                 ("vuart", libxl_vuart_type),
>>>> +                               ("vpci", libxl_defbool),
>>> 
>>> Any new addition in the structure should be accompanied with a LIBXL_HAVE_* in the libxl.h header.
>> OK.
>>> 
>>>>                                ])),
>>>>      ("arch_x86", Struct(None, [("msr_relaxed", libxl_defbool),
>>>>                                ])),
>>>> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
>>>> index 17dddb4cd5..ffafbeffb4 100644
>>>> --- a/tools/xl/xl_parse.c
>>>> +++ b/tools/xl/xl_parse.c
>>>> @@ -1497,6 +1497,8 @@ void parse_config_data(const char *config_source,
>>>>          }
>>>>          if (d_config->num_pcidevs && c_info->type == LIBXL_DOMAIN_TYPE_PV)
>>>>              libxl_defbool_set(&b_info->u.pv.e820_host, true);
>>>> +        if (d_config->num_pcidevs)
>>>> +            libxl_defbool_set(&b_info->arch_arm.vpci, true);
>>>>      }
>>>>        if (!xlu_cfg_get_list (config, "dtdev", &dtdevs, 0, 0)) {
>>>> diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
>>>> index 0a9749e768..01d13e669e 100644
>>>> --- a/xen/include/public/arch-arm.h
>>>> +++ b/xen/include/public/arch-arm.h
>>>> @@ -426,6 +426,17 @@ typedef uint64_t xen_callback_t;
>>>>  #define GUEST_VPCI_ECAM_BASE    xen_mk_ullong(0x10000000)
>>>>  #define GUEST_VPCI_ECAM_SIZE    xen_mk_ullong(0x10000000)
>>>>  +/* PCI-PCIe memory space types */
>>>> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM xen_mk_ullong(0x42000000)
>>>> +#define GUEST_VPCI_ADDR_TYPE_MEM          xen_mk_ullong(0x02000000)
>>> 
>>> What the size of those regions?
>> Non Prefetch Memory: Size 64 MB start at 512 MB
>> Prefetch Memory: Size 128 GB start at 36 GB
>>> 
>>>> +
>>>> +/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
>>>> +#define GUEST_VPCI_PREFETCH_MEM_ADDR  xen_mk_ullong(0x900000000)
>>>> +#define GUEST_VPCI_MEM_ADDR           xen_mk_ullong(0x20000000)
>>> So far the memory layout defines the address in ascending order. So please add that after GUEST_RAM_BANK_BASES_*.
>> Ok.
>>> 
>>> However, if I am not mistaken that base address you provide will clash with RAM bank 1. It also seem to be pretty high which means that this will not work for 32-bit domain or on CPUs that don't allow offer large IPA bits.
>> Yes I also checked that now that it is having clash with RAM bank 1.
>> There is unused space is guest memory that we can use for Non Prefetch Memory as per below guest memory map.
>> https://gitlab.com/xen-project/fusa/fusa-docs/-/blob/master/high-level/guest-memory-layout-arm.rst
>> Proposed value:
>> Non Prefetch Memory: Size 64 MB start at 0x22001000
>> Prefetch Memory: Size 4 GB start at 4 GB.
> 
> The base address looks fine to me. However, the sizes are much smaller to what you initially suggested. Would you be able to clarify why the smaller sizes are fine?

As per current design we can have 32 device in guest therefore I thought 64 MB is sufficient ( 2 MB per device ).
> 
>>> 
>>> I think we need to start making the guest layout more dynamic. The VPCI memory space would have to go right after the end of the RAM allocated for a given guest.
>>> 
>>>> +
>>>> +#define GUEST_VPCI_PREFETCH_MEM_SIZE      xen_mk_ullong(0x2000000000)
>>>> +#define GUEST_VPCI_MEM_SIZE               xen_mk_ullong(0x04000000)
>>> 
>>> It would be better if the size for each region is defined right after each base.
>> OK.
>>> 
>>> Also, how did you decide the size of each region?
>> I thought 64 MB will be sufficient. I think it should be based on number of devices we can assign to the guest.
> 
> We don't have to get the size right now. What I am more interested is to have a trace about how those values were decided (even if it just saying random). This will help to make any decision if in the future we need to resize (in particular downsize) the regions.

As per Stefano suggestion in another mail he suggested 256 MB. I also think 256 MB will be sufficient ( 8 MB per device) . I will comment this in code.

Regards,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall