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>
---
Change in v4:
- Gate code for x86 for setting the XEN_DOMCTL_CDF_vpci for x86.
Change in v3:
- Make GUEST_VPCI_MEM_ADDR address 2MB aligned
Change in v2:
- enable doamin_vpci_init() when XEN_DOMCTL_CDF_vpci is set for domain.
---
---
 tools/include/libxl.h            |   6 ++
 tools/libs/light/libxl_arm.c     | 105 +++++++++++++++++++++++++++++++
 tools/libs/light/libxl_create.c  |   9 +++
 tools/libs/light/libxl_types.idl |   1 +
 tools/xl/xl_parse.c              |   8 +++
 xen/include/public/arch-arm.h    |  10 +++
 6 files changed, 139 insertions(+)

diff --git a/tools/include/libxl.h b/tools/include/libxl.h
index b9ba16d698..3362073b21 100644
--- a/tools/include/libxl.h
+++ b/tools/include/libxl.h
@@ -358,6 +358,12 @@
  */
 #define LIBXL_HAVE_BUILDINFO_ARM_VUART 1
 
+/*
+ * LIBXL_HAVE_BUILDINFO_ARM_VPCI indicates that the toolstack supports virtual
+ * PCI for ARM.
+ */
+#define LIBXL_HAVE_BUILDINFO_ARM_VPCI 1
+
 /*
  * LIBXL_HAVE_BUILDINFO_GRANT_LIMITS indicates that libxl_domain_build_info
  * has the max_grant_frames and max_maptrack_frames fields.
diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6e00..52f1ddce48 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, uint32_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,53 @@ 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, 255);
+    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_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 +1070,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 +1291,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_create.c b/tools/libs/light/libxl_create.c
index e356b2106d..9408526036 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -632,6 +632,15 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
         if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
             create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
 
+#if defined(__arm__) || defined(__aarch64__)
+        /*
+         * Enable VPCI support for ARM. VPCI support for DOMU guest is not
+         * supported for x86.
+         */
+        if ( libxl_defbool_val(b_info->arch_arm.vpci) )
+            create.flags |= XEN_DOMCTL_CDF_vpci;
+#endif
+
         /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
         libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
 
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..576af67daa 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1497,6 +1497,14 @@ 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 defined(__arm__) || defined(__aarch64__)
+        /*
+         * Enable VPCI support for ARM. VPCI support for DOMU guest is not
+         * supported for x86.
+         */
+        if (d_config->num_pcidevs)
+            libxl_defbool_set(&b_info->arch_arm.vpci, true);
+#endif
     }
 
     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 44be337dec..45aac5d18f 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -433,6 +433,11 @@ typedef uint64_t xen_callback_t;
 #define GUEST_PL011_BASE    xen_mk_ullong(0x22000000)
 #define GUEST_PL011_SIZE    xen_mk_ullong(0x00001000)
 
+/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
+#define GUEST_VPCI_ADDR_TYPE_MEM            xen_mk_ullong(0x02000000)
+#define GUEST_VPCI_MEM_ADDR                 xen_mk_ullong(0x23000000)
+#define GUEST_VPCI_MEM_SIZE                 xen_mk_ullong(0x10000000)
+
 /*
  * 16MB == 4096 pages reserved for guest to use as a region to map its
  * grant table in.
@@ -448,6 +453,11 @@ typedef uint64_t xen_callback_t;
 #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
 #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
 
+/* 4GB @ 4GB Prefetch Memory for VPCI */
+#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM   xen_mk_ullong(0x42000000)
+#define GUEST_VPCI_PREFETCH_MEM_ADDR        xen_mk_ullong(0x100000000)
+#define GUEST_VPCI_PREFETCH_MEM_SIZE        xen_mk_ullong(0x100000000)
+
 #define GUEST_RAM1_BASE   xen_mk_ullong(0x0200000000) /* 1016GB of RAM @ 8GB */
 #define GUEST_RAM1_SIZE   xen_mk_ullong(0xfe00000000)
 
-- 
2.25.1

On Mon, 4 Oct 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>
> ---
> Change in v4:
> - Gate code for x86 for setting the XEN_DOMCTL_CDF_vpci for x86.
> Change in v3:
> - Make GUEST_VPCI_MEM_ADDR address 2MB aligned
> Change in v2:
> - enable doamin_vpci_init() when XEN_DOMCTL_CDF_vpci is set for domain.
> ---
> ---
>  tools/include/libxl.h            |   6 ++
>  tools/libs/light/libxl_arm.c     | 105 +++++++++++++++++++++++++++++++
>  tools/libs/light/libxl_create.c  |   9 +++
>  tools/libs/light/libxl_types.idl |   1 +
>  tools/xl/xl_parse.c              |   8 +++
>  xen/include/public/arch-arm.h    |  10 +++
>  6 files changed, 139 insertions(+)
> 
> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> index b9ba16d698..3362073b21 100644
> --- a/tools/include/libxl.h
> +++ b/tools/include/libxl.h
> @@ -358,6 +358,12 @@
>   */
>  #define LIBXL_HAVE_BUILDINFO_ARM_VUART 1
>  
> +/*
> + * LIBXL_HAVE_BUILDINFO_ARM_VPCI indicates that the toolstack supports virtual
> + * PCI for ARM.
> + */
> +#define LIBXL_HAVE_BUILDINFO_ARM_VPCI 1
> +
>  /*
>   * LIBXL_HAVE_BUILDINFO_GRANT_LIMITS indicates that libxl_domain_build_info
>   * has the max_grant_frames and max_maptrack_frames fields.
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6e00..52f1ddce48 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, uint32_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,53 @@ 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, 255);
> +    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_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 +1070,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 +1291,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_create.c b/tools/libs/light/libxl_create.c
> index e356b2106d..9408526036 100644
> --- a/tools/libs/light/libxl_create.c
> +++ b/tools/libs/light/libxl_create.c
> @@ -632,6 +632,15 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>          if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
>              create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
>  
> +#if defined(__arm__) || defined(__aarch64__)
> +        /*
> +         * Enable VPCI support for ARM. VPCI support for DOMU guest is not
> +         * supported for x86.
> +         */
> +        if ( libxl_defbool_val(b_info->arch_arm.vpci) )
> +            create.flags |= XEN_DOMCTL_CDF_vpci;
> +#endif

I don't think the #ifdef is required, is it? The check is based on
b_info->arch_arm.vpci which is already ARM-specific and couldn't be
enabled on X86. We have another similar check in libxl_create.c for
d_config->b_info.arch_arm.vuart without #ifdef.

My suggestion would be to just keep the in-code comment, but leave the
libxl_defbool_val check as it was before.


> +
>          /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
>          libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
>  
> 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..576af67daa 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -1497,6 +1497,14 @@ 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 defined(__arm__) || defined(__aarch64__)
> +        /*
> +         * Enable VPCI support for ARM. VPCI support for DOMU guest is not
> +         * supported for x86.
> +         */
> +        if (d_config->num_pcidevs)
> +            libxl_defbool_set(&b_info->arch_arm.vpci, true);
> +#endif
>      }
>  
>      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 44be337dec..45aac5d18f 100644
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -433,6 +433,11 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_PL011_BASE    xen_mk_ullong(0x22000000)
>  #define GUEST_PL011_SIZE    xen_mk_ullong(0x00001000)
>  
> +/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
> +#define GUEST_VPCI_ADDR_TYPE_MEM            xen_mk_ullong(0x02000000)
> +#define GUEST_VPCI_MEM_ADDR                 xen_mk_ullong(0x23000000)
> +#define GUEST_VPCI_MEM_SIZE                 xen_mk_ullong(0x10000000)
> +
>  /*
>   * 16MB == 4096 pages reserved for guest to use as a region to map its
>   * grant table in.
> @@ -448,6 +453,11 @@ typedef uint64_t xen_callback_t;
>  #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
>  #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
>  
> +/* 4GB @ 4GB Prefetch Memory for VPCI */
> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM   xen_mk_ullong(0x42000000)
> +#define GUEST_VPCI_PREFETCH_MEM_ADDR        xen_mk_ullong(0x100000000)
> +#define GUEST_VPCI_PREFETCH_MEM_SIZE        xen_mk_ullong(0x100000000)
> +
>  #define GUEST_RAM1_BASE   xen_mk_ullong(0x0200000000) /* 1016GB of RAM @ 8GB */
>  #define GUEST_RAM1_SIZE   xen_mk_ullong(0xfe00000000)
>  
> -- 
> 2.25.1
> 

Hi Stefano,

> On 5 Oct 2021, at 1:38 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Mon, 4 Oct 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>
>> ---
>> Change in v4:
>> - Gate code for x86 for setting the XEN_DOMCTL_CDF_vpci for x86.
>> Change in v3:
>> - Make GUEST_VPCI_MEM_ADDR address 2MB aligned
>> Change in v2:
>> - enable doamin_vpci_init() when XEN_DOMCTL_CDF_vpci is set for domain.
>> ---
>> ---
>> tools/include/libxl.h            |   6 ++
>> tools/libs/light/libxl_arm.c     | 105 +++++++++++++++++++++++++++++++
>> tools/libs/light/libxl_create.c  |   9 +++
>> tools/libs/light/libxl_types.idl |   1 +
>> tools/xl/xl_parse.c              |   8 +++
>> xen/include/public/arch-arm.h    |  10 +++
>> 6 files changed, 139 insertions(+)
>> 
>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>> index b9ba16d698..3362073b21 100644
>> --- a/tools/include/libxl.h
>> +++ b/tools/include/libxl.h
>> @@ -358,6 +358,12 @@
>>  */
>> #define LIBXL_HAVE_BUILDINFO_ARM_VUART 1
>> 
>> +/*
>> + * LIBXL_HAVE_BUILDINFO_ARM_VPCI indicates that the toolstack supports virtual
>> + * PCI for ARM.
>> + */
>> +#define LIBXL_HAVE_BUILDINFO_ARM_VPCI 1
>> +
>> /*
>>  * LIBXL_HAVE_BUILDINFO_GRANT_LIMITS indicates that libxl_domain_build_info
>>  * has the max_grant_frames and max_maptrack_frames fields.
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index e3140a6e00..52f1ddce48 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, uint32_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,53 @@ 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, 255);
>> +    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_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 +1070,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 +1291,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_create.c b/tools/libs/light/libxl_create.c
>> index e356b2106d..9408526036 100644
>> --- a/tools/libs/light/libxl_create.c
>> +++ b/tools/libs/light/libxl_create.c
>> @@ -632,6 +632,15 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>>         if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
>>             create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
>> 
>> +#if defined(__arm__) || defined(__aarch64__)
>> +        /*
>> +         * Enable VPCI support for ARM. VPCI support for DOMU guest is not
>> +         * supported for x86.
>> +         */
>> +        if ( libxl_defbool_val(b_info->arch_arm.vpci) )
>> +            create.flags |= XEN_DOMCTL_CDF_vpci;
>> +#endif
> 
> I don't think the #ifdef is required, is it? The check is based on
> b_info->arch_arm.vpci which is already ARM-specific and couldn't be
> enabled on X86. We have another similar check in libxl_create.c for
> d_config->b_info.arch_arm.vuart without #ifdef.
> 
> My suggestion would be to just keep the in-code comment, but leave the
> libxl_defbool_val check as it was before.
> 

I also thought the same way that "b_info->arch_arm.vpci|" is arm-specific but somehow it is getting enabled for x86 
when we assign the PCI device to DOMU guests on x86 PV DOM0 once I remove the #ifdef for below code.

#if defined(__arm__) || defined(__aarch64__)                   
    /*                                    
     * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
     * supported for x86.                          
     */                                   
    if (d_config->num_pcidevs)                        
      libxl_defbool_set(&b_info->arch_arm.vpci, true);           
#endif 

Error on x86:
Parsing config from guest.cfg
(XEN) domain.c:667: vPCI cannot be enabled yet
libxl: error: libxl_create.c:683:libxl__domain_make: domain creation fail: Invalid argument
libxl: error: libxl_create.c:1237:initiate_domain_create: cannot make domain: -3

One solution is we can remove the #ifdef from the below code when checking if vpci is enabled…
#if defined(__arm__) || defined(__aarch64__)                   
    /*                                    
     * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
     * supported for x86.                          
     */                                   
    if ( libxl_defbool_val(b_info->arch_arm.vpci) )             
      create.flags |= XEN_DOMCTL_CDF_vpci;                 
#endif

..but not from here when setting the arch_arm.vpci when we assign the PCI device to the guest.

#if defined(__arm__) || defined(__aarch64__)                  
    /*                                    
     * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
     * supported for x86.                          
     */                                   
    if (d_config->num_pcidevs)                        
      libxl_defbool_set(&b_info->arch_arm.vpci, true);           
#endif 


Also if I remove #ifdef as mention above I need to move the 
       "libxl_defbool_setdefault(&b_info->arch_arm.vpci, false); “ 
from 
       libxl__arch_domain_build_info_setdefault(..) 
to common code 
       libxl__domain_build_info_setdefault(..) to avoid error on x86.

Error on x86:
root@dom0:~# xl create -c guest.cfg
Parsing config from guest.cfg
xl: libxl.c:337: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
Aborted

> 
>> +
>>         /* Ultimately, handle is an array of 16 uint8_t, same as uuid */
>>         libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
>> 
>> 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..576af67daa 100644
>> --- a/tools/xl/xl_parse.c
>> +++ b/tools/xl/xl_parse.c
>> @@ -1497,6 +1497,14 @@ 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 defined(__arm__) || defined(__aarch64__)
>> +        /*
>> +         * Enable VPCI support for ARM. VPCI support for DOMU guest is not
>> +         * supported for x86.
>> +         */
>> +        if (d_config->num_pcidevs)
>> +            libxl_defbool_set(&b_info->arch_arm.vpci, true);
>> +#endif
>>     }
>> 
>>     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 44be337dec..45aac5d18f 100644
>> --- a/xen/include/public/arch-arm.h
>> +++ b/xen/include/public/arch-arm.h
>> @@ -433,6 +433,11 @@ typedef uint64_t xen_callback_t;
>> #define GUEST_PL011_BASE    xen_mk_ullong(0x22000000)
>> #define GUEST_PL011_SIZE    xen_mk_ullong(0x00001000)
>> 
>> +/* Guest PCI-PCIe memory space where config space and BAR will be available.*/
>> +#define GUEST_VPCI_ADDR_TYPE_MEM            xen_mk_ullong(0x02000000)
>> +#define GUEST_VPCI_MEM_ADDR                 xen_mk_ullong(0x23000000)
>> +#define GUEST_VPCI_MEM_SIZE                 xen_mk_ullong(0x10000000)
>> +
>> /*
>>  * 16MB == 4096 pages reserved for guest to use as a region to map its
>>  * grant table in.
>> @@ -448,6 +453,11 @@ typedef uint64_t xen_callback_t;
>> #define GUEST_RAM0_BASE   xen_mk_ullong(0x40000000) /* 3GB of low RAM @ 1GB */
>> #define GUEST_RAM0_SIZE   xen_mk_ullong(0xc0000000)
>> 
>> +/* 4GB @ 4GB Prefetch Memory for VPCI */
>> +#define GUEST_VPCI_ADDR_TYPE_PREFETCH_MEM   xen_mk_ullong(0x42000000)
>> +#define GUEST_VPCI_PREFETCH_MEM_ADDR        xen_mk_ullong(0x100000000)
>> +#define GUEST_VPCI_PREFETCH_MEM_SIZE        xen_mk_ullong(0x100000000)
>> +
>> #define GUEST_RAM1_BASE   xen_mk_ullong(0x0200000000) /* 1016GB of RAM @ 8GB */
>> #define GUEST_RAM1_SIZE   xen_mk_ullong(0xfe00000000)
>> 
>> -- 
>> 2.25.1

On Tue, 5 Oct 2021, Rahul Singh wrote:
> > On 5 Oct 2021, at 1:38 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > 
> > On Mon, 4 Oct 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>
> >> ---
> >> Change in v4:
> >> - Gate code for x86 for setting the XEN_DOMCTL_CDF_vpci for x86.
> >> Change in v3:
> >> - Make GUEST_VPCI_MEM_ADDR address 2MB aligned
> >> Change in v2:
> >> - enable doamin_vpci_init() when XEN_DOMCTL_CDF_vpci is set for domain.
> >> ---
> >> ---
> >> tools/include/libxl.h            |   6 ++
> >> tools/libs/light/libxl_arm.c     | 105 +++++++++++++++++++++++++++++++
> >> tools/libs/light/libxl_create.c  |   9 +++
> >> tools/libs/light/libxl_types.idl |   1 +
> >> tools/xl/xl_parse.c              |   8 +++
> >> xen/include/public/arch-arm.h    |  10 +++
> >> 6 files changed, 139 insertions(+)
> >> 
> >> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
> >> index b9ba16d698..3362073b21 100644
> >> --- a/tools/include/libxl.h
> >> +++ b/tools/include/libxl.h
> >> @@ -358,6 +358,12 @@
> >>  */
> >> #define LIBXL_HAVE_BUILDINFO_ARM_VUART 1
> >> 
> >> +/*
> >> + * LIBXL_HAVE_BUILDINFO_ARM_VPCI indicates that the toolstack supports virtual
> >> + * PCI for ARM.
> >> + */
> >> +#define LIBXL_HAVE_BUILDINFO_ARM_VPCI 1
> >> +
> >> /*
> >>  * LIBXL_HAVE_BUILDINFO_GRANT_LIMITS indicates that libxl_domain_build_info
> >>  * has the max_grant_frames and max_maptrack_frames fields.
> >> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> >> index e3140a6e00..52f1ddce48 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, uint32_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,53 @@ 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, 255);
> >> +    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_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 +1070,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 +1291,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_create.c b/tools/libs/light/libxl_create.c
> >> index e356b2106d..9408526036 100644
> >> --- a/tools/libs/light/libxl_create.c
> >> +++ b/tools/libs/light/libxl_create.c
> >> @@ -632,6 +632,15 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
> >>         if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
> >>             create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
> >> 
> >> +#if defined(__arm__) || defined(__aarch64__)
> >> +        /*
> >> +         * Enable VPCI support for ARM. VPCI support for DOMU guest is not
> >> +         * supported for x86.
> >> +         */
> >> +        if ( libxl_defbool_val(b_info->arch_arm.vpci) )
> >> +            create.flags |= XEN_DOMCTL_CDF_vpci;
> >> +#endif
> > 
> > I don't think the #ifdef is required, is it? The check is based on
> > b_info->arch_arm.vpci which is already ARM-specific and couldn't be
> > enabled on X86. We have another similar check in libxl_create.c for
> > d_config->b_info.arch_arm.vuart without #ifdef.
> > 
> > My suggestion would be to just keep the in-code comment, but leave the
> > libxl_defbool_val check as it was before.
> > 
> 
> I also thought the same way that "b_info->arch_arm.vpci|" is arm-specific but somehow it is getting enabled for x86 
> when we assign the PCI device to DOMU guests on x86 PV DOM0 once I remove the #ifdef for below code.
> 
> #if defined(__arm__) || defined(__aarch64__)                   
>     /*                                    
>      * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
>      * supported for x86.                          
>      */                                   
>     if (d_config->num_pcidevs)                        
>       libxl_defbool_set(&b_info->arch_arm.vpci, true);           
> #endif 
> 
> Error on x86:
> Parsing config from guest.cfg
> (XEN) domain.c:667: vPCI cannot be enabled yet
> libxl: error: libxl_create.c:683:libxl__domain_make: domain creation fail: Invalid argument
> libxl: error: libxl_create.c:1237:initiate_domain_create: cannot make domain: -3
> 
> One solution is we can remove the #ifdef from the below code when checking if vpci is enabled…
> #if defined(__arm__) || defined(__aarch64__)                   
>     /*                                    
>      * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
>      * supported for x86.                          
>      */                                   
>     if ( libxl_defbool_val(b_info->arch_arm.vpci) )             
>       create.flags |= XEN_DOMCTL_CDF_vpci;                 
> #endif
> 
> ..but not from here when setting the arch_arm.vpci when we assign the PCI device to the guest.
> 
> #if defined(__arm__) || defined(__aarch64__)                  
>     /*                                    
>      * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
>      * supported for x86.                          
>      */                                   
>     if (d_config->num_pcidevs)                        
>       libxl_defbool_set(&b_info->arch_arm.vpci, true);           
> #endif 
> 
> 
> Also if I remove #ifdef as mention above I need to move the 
>        "libxl_defbool_setdefault(&b_info->arch_arm.vpci, false); “ 
> from 
>        libxl__arch_domain_build_info_setdefault(..) 
> to common code 
>        libxl__domain_build_info_setdefault(..) to avoid error on x86.
> 
> Error on x86:
> root@dom0:~# xl create -c guest.cfg
> Parsing config from guest.cfg
> xl: libxl.c:337: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
> Aborted

As far as I can tell, the #ifdef in libxl_create.c can be removed by
doing:

        if ( libxl_defbool_val(b_info->arch_arm.vpci) > 0 )
            create.flags |= XEN_DOMCTL_CDF_vpci;

because we need to check for LIBXL__DEFBOOL_TRUE, which is > 0, right?
And vpci should never be set on x86. arch_arm.vpci should be initialized
to zero on x86 which is LIBXL__DEFBOOL_DEFAULT. That should work.


On the other hand you are right that the #ifdef in tools/xl/xl_parse.c
cannot just be removed because otherwise b_info->arch_arm.vpci gets set
on x86, which obviously we don't want.

Hi Stefano,

> On 5 Oct 2021, at 10:32 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> 
> On Tue, 5 Oct 2021, Rahul Singh wrote:
>>> On 5 Oct 2021, at 1:38 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>> 
>>> On Mon, 4 Oct 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>
>>>> ---
>>>> Change in v4:
>>>> - Gate code for x86 for setting the XEN_DOMCTL_CDF_vpci for x86.
>>>> Change in v3:
>>>> - Make GUEST_VPCI_MEM_ADDR address 2MB aligned
>>>> Change in v2:
>>>> - enable doamin_vpci_init() when XEN_DOMCTL_CDF_vpci is set for domain.
>>>> ---
>>>> ---
>>>> tools/include/libxl.h            |   6 ++
>>>> tools/libs/light/libxl_arm.c     | 105 +++++++++++++++++++++++++++++++
>>>> tools/libs/light/libxl_create.c  |   9 +++
>>>> tools/libs/light/libxl_types.idl |   1 +
>>>> tools/xl/xl_parse.c              |   8 +++
>>>> xen/include/public/arch-arm.h    |  10 +++
>>>> 6 files changed, 139 insertions(+)
>>>> 
>>>> diff --git a/tools/include/libxl.h b/tools/include/libxl.h
>>>> index b9ba16d698..3362073b21 100644
>>>> --- a/tools/include/libxl.h
>>>> +++ b/tools/include/libxl.h
>>>> @@ -358,6 +358,12 @@
>>>> */
>>>> #define LIBXL_HAVE_BUILDINFO_ARM_VUART 1
>>>> 
>>>> +/*
>>>> + * LIBXL_HAVE_BUILDINFO_ARM_VPCI indicates that the toolstack supports virtual
>>>> + * PCI for ARM.
>>>> + */
>>>> +#define LIBXL_HAVE_BUILDINFO_ARM_VPCI 1
>>>> +
>>>> /*
>>>> * LIBXL_HAVE_BUILDINFO_GRANT_LIMITS indicates that libxl_domain_build_info
>>>> * has the max_grant_frames and max_maptrack_frames fields.
>>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>>> index e3140a6e00..52f1ddce48 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, uint32_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,53 @@ 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, 255);
>>>> +    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_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 +1070,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 +1291,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_create.c b/tools/libs/light/libxl_create.c
>>>> index e356b2106d..9408526036 100644
>>>> --- a/tools/libs/light/libxl_create.c
>>>> +++ b/tools/libs/light/libxl_create.c
>>>> @@ -632,6 +632,15 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
>>>>        if (info->passthrough == LIBXL_PASSTHROUGH_SYNC_PT)
>>>>            create.iommu_opts |= XEN_DOMCTL_IOMMU_no_sharept;
>>>> 
>>>> +#if defined(__arm__) || defined(__aarch64__)
>>>> +        /*
>>>> +         * Enable VPCI support for ARM. VPCI support for DOMU guest is not
>>>> +         * supported for x86.
>>>> +         */
>>>> +        if ( libxl_defbool_val(b_info->arch_arm.vpci) )
>>>> +            create.flags |= XEN_DOMCTL_CDF_vpci;
>>>> +#endif
>>> 
>>> I don't think the #ifdef is required, is it? The check is based on
>>> b_info->arch_arm.vpci which is already ARM-specific and couldn't be
>>> enabled on X86. We have another similar check in libxl_create.c for
>>> d_config->b_info.arch_arm.vuart without #ifdef.
>>> 
>>> My suggestion would be to just keep the in-code comment, but leave the
>>> libxl_defbool_val check as it was before.
>>> 
>> 
>> I also thought the same way that "b_info->arch_arm.vpci|" is arm-specific but somehow it is getting enabled for x86 
>> when we assign the PCI device to DOMU guests on x86 PV DOM0 once I remove the #ifdef for below code.
>> 
>> #if defined(__arm__) || defined(__aarch64__)                   
>>    /*                                    
>>     * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
>>     * supported for x86.                          
>>     */                                   
>>    if (d_config->num_pcidevs)                        
>>      libxl_defbool_set(&b_info->arch_arm.vpci, true);           
>> #endif 
>> 
>> Error on x86:
>> Parsing config from guest.cfg
>> (XEN) domain.c:667: vPCI cannot be enabled yet
>> libxl: error: libxl_create.c:683:libxl__domain_make: domain creation fail: Invalid argument
>> libxl: error: libxl_create.c:1237:initiate_domain_create: cannot make domain: -3
>> 
>> One solution is we can remove the #ifdef from the below code when checking if vpci is enabled…
>> #if defined(__arm__) || defined(__aarch64__)                   
>>    /*                                    
>>     * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
>>     * supported for x86.                          
>>     */                                   
>>    if ( libxl_defbool_val(b_info->arch_arm.vpci) )             
>>      create.flags |= XEN_DOMCTL_CDF_vpci;                 
>> #endif
>> 
>> ..but not from here when setting the arch_arm.vpci when we assign the PCI device to the guest.
>> 
>> #if defined(__arm__) || defined(__aarch64__)                  
>>    /*                                    
>>     * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
>>     * supported for x86.                          
>>     */                                   
>>    if (d_config->num_pcidevs)                        
>>      libxl_defbool_set(&b_info->arch_arm.vpci, true);           
>> #endif 
>> 
>> 
>> Also if I remove #ifdef as mention above I need to move the 
>>       "libxl_defbool_setdefault(&b_info->arch_arm.vpci, false); “ 
>> from 
>>       libxl__arch_domain_build_info_setdefault(..) 
>> to common code 
>>       libxl__domain_build_info_setdefault(..) to avoid error on x86.
>> 
>> Error on x86:
>> root@dom0:~# xl create -c guest.cfg
>> Parsing config from guest.cfg
>> xl: libxl.c:337: libxl_defbool_val: Assertion `!libxl_defbool_is_default(db)' failed.
>> Aborted
> 
> As far as I can tell, the #ifdef in libxl_create.c can be removed by
> doing:
> 
>        if ( libxl_defbool_val(b_info->arch_arm.vpci) > 0 )
>            create.flags |= XEN_DOMCTL_CDF_vpci;
> 
> because we need to check for LIBXL__DEFBOOL_TRUE, which is > 0, right?

Yes right we have to check  LIBXL__DEFBOOL_TRUE which is 1.

> And vpci should never be set on x86. arch_arm.vpci should be initialized
> to zero on x86 which is LIBXL__DEFBOOL_DEFAULT. That should work.

libxl_defbool_val(libxl_defbool db) is implemented in such a way that it will assert 
if db value is the default. Therefore we have to explicitly set it to LIBXL__DEFBOOL_FALSE for x86.

There are two option either we can have #ifdef or we can move the 
libxl_defbool_setdefault(&b_info->arch_arm.vpci, false) to common code.

Regards,
Rahul
> 
> 
> On the other hand you are right that the #ifdef in tools/xl/xl_parse.c
> cannot just be removed because otherwise b_info->arch_arm.vpci gets set
> on x86, which obviously we don't want.

Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl"):
> libxl_defbool_val(libxl_defbool db) is implemented in such a way that it will assert 
> if db value is the default. Therefore we have to explicitly set it to LIBXL__DEFBOOL_FALSE for x86.
> 
> There are two option either we can have #ifdef or we can move the 
> libxl_defbool_setdefault(&b_info->arch_arm.vpci, false) to common code.

What is wrong with putting it in
libxl__arch_domain_build_info_setdefault
which I think exists precisely for this kind of thing ?

Ian.

Hi Ian	

> On 6 Oct 2021, at 12:07 pm, Ian Jackson <iwj@xenproject.org> wrote:
> 
> Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl"):
>> libxl_defbool_val(libxl_defbool db) is implemented in such a way that it will assert 
>> if db value is the default. Therefore we have to explicitly set it to LIBXL__DEFBOOL_FALSE for x86.
>> 
>> There are two option either we can have #ifdef or we can move the 
>> libxl_defbool_setdefault(&b_info->arch_arm.vpci, false) to common code.
> 
> What is wrong with putting it in
> libxl__arch_domain_build_info_setdefault
> which I think exists precisely for this kind of thing ?

As we have to set the arch_arm.vpci to false for x86 and ARM I thought it is right to move the code to
common code to avoid duplication.

Are you suggesting to put " libxl_defbool_setdefault(&b_info->arch_arm.vpci, false)”in 
libxl__arch_domain_build_info_setdefault() for x86 and ARM differently.

Regards,
Rahul
> 
> Ian.

Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl"):
> Hi Ian	
> > What is wrong with putting it in
> > libxl__arch_domain_build_info_setdefault
> > which I think exists precisely for this kind of thing ?
> 
> As we have to set the arch_arm.vpci to false for x86 and ARM I
> thought it is right to move the code to common code to avoid
> duplication.
> 
> Are you suggesting to put "
> libxl_defbool_setdefault(&b_info->arch_arm.vpci, false)”in
> libxl__arch_domain_build_info_setdefault() for x86 and ARM
> differently.

I've gone back and reread the whole thread, which I probably should
have done to start with....

So:

> >> #if defined(__arm__) || defined(__aarch64__)                  
> >>    /*                                    
> >>     * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
> >>     * supported for x86.                          
> >>     */                                   
> >>    if (d_config->num_pcidevs)                        
> >>      libxl_defbool_set(&b_info->arch_arm.vpci, true);           
> >> #endif 

I think this logic probably ought to be in libxl, not in xl.  We try
to make the libxl API "do the right thing" by default.  In this case I
think that means to enable VPCI (i) on platforms where it's available
(ii) if the guest has PCI passthrough devices.  Is that right ?

Sorry to ask these question now, and please forgive my ignorance:

Is VPCI inherently an ARM-specific ABI or protocol ?  When might an
admin want to turn it on explicitly ?

How does this all relate to the (non-arch-specific) "passthrough"
option ?

Ian.

Hi Ian,

> On 6 Oct 2021, at 12:33 pm, Ian Jackson <iwj@xenproject.org> wrote:
> 
> Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl"):
>> Hi Ian	
>>> What is wrong with putting it in
>>> libxl__arch_domain_build_info_setdefault
>>> which I think exists precisely for this kind of thing ?
>> 
>> As we have to set the arch_arm.vpci to false for x86 and ARM I
>> thought it is right to move the code to common code to avoid
>> duplication.
>> 
>> Are you suggesting to put "
>> libxl_defbool_setdefault(&b_info->arch_arm.vpci, false)ïżœin
>> libxl__arch_domain_build_info_setdefault() for x86 and ARM
>> differently.
> 
> I've gone back and reread the whole thread, which I probably should
> have done to start with....
> 
> So:
> 
>>>> #if defined(__arm__) || defined(__aarch64__)                  
>>>>   /*                                    
>>>>    * Enable VPCI support for ARM. VPCI support for DOMU guests is not    
>>>>    * supported for x86.                          
>>>>    */                                   
>>>>   if (d_config->num_pcidevs)                        
>>>>     libxl_defbool_set(&b_info->arch_arm.vpci, true);           
>>>> #endif 
> 
> I think this logic probably ought to be in libxl, not in xl.

I will move the code to "libxl_arm.c"to avoid #ifdef in common code and also  to avoid setting the vpci for x86

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index e3140a6e00..2be208b99b 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -101,6 +101,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
         return ERROR_FAIL;
     }
 
+    /* Enable VPCI support. */
+    if (d_config->num_pcidevs) {
+        config->flags |= XEN_DOMCTL_CDF_vpci;
+        libxl_defbool_set(&d_config->b_info.arch_arm.vpci, true);
+    }
+
     return 0;
 }

>  We try
> to make the libxl API "do the right thing" by default.  In this case I
> think that means to enable VPCI (i) on platforms where it's available
> (ii) if the guest has PCI passthrough devices.  Is that right ?

Yes you are right VPCI will be enabled for guest when guest has PCI passthrough device 
assigned and VPCI support is available.  
> 
> Sorry to ask these question now, and please forgive my ignorance:
> 
> Is VPCI inherently an ARM-specific ABI or protocol ?

As of now VPCI for DOMU guests is only implemented  for ARM.
 
>  When might an
> admin want to turn it on explicitly ?

It will be enabled dynamically when admin assign any PCI device to guest.

> 
> How does this all relate to the (non-arch-specific) "passthrough"
> option ?

VPCI will be enabled only when there is any PCI device assigned to guest therefore I used 
"d_config->num_pcidevs” to enable VPCI.

Regards,
Rahul

> 
> Ian.

Hi Rahul,

On 06/10/2021 19:34, Rahul Singh wrote:
>> On 6 Oct 2021, at 12:33 pm, Ian Jackson <iwj@xenproject.org> wrote:
>>
>> Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl"):
>>> Hi Ian	
>>>> What is wrong with putting it in
>>>> libxl__arch_domain_build_info_setdefault
>>>> which I think exists precisely for this kind of thing ?
>>>
>>> As we have to set the arch_arm.vpci to false for x86 and ARM I
>>> thought it is right to move the code to common code to avoid
>>> duplication.
>>>
>>> Are you suggesting to put "
>>> libxl_defbool_setdefault(&b_info->arch_arm.vpci, false)ïżœin
>>> libxl__arch_domain_build_info_setdefault() for x86 and ARM
>>> differently.
>>
>> I've gone back and reread the whole thread, which I probably should
>> have done to start with....
>>
>> So:
>>
>>>>> #if defined(__arm__) || defined(__aarch64__)
>>>>>    /*
>>>>>     * Enable VPCI support for ARM. VPCI support for DOMU guests is not
>>>>>     * supported for x86.
>>>>>     */
>>>>>    if (d_config->num_pcidevs)
>>>>>      libxl_defbool_set(&b_info->arch_arm.vpci, true);
>>>>> #endif
>>
>> I think this logic probably ought to be in libxl, not in xl.
> 
> I will move the code to "libxl_arm.c"to avoid #ifdef in common code and also  to avoid setting the vpci for x86
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index e3140a6e00..2be208b99b 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -101,6 +101,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>           return ERROR_FAIL;
>       }
>   
> +    /* Enable VPCI support. */
> +    if (d_config->num_pcidevs) {
> +        config->flags |= XEN_DOMCTL_CDF_vpci;
> +        libxl_defbool_set(&d_config->b_info.arch_arm.vpci, true);
> +    }
> +
>       return 0;
>   }
> 
>>   We try
>> to make the libxl API "do the right thing" by default.  In this case I
>> think that means to enable VPCI (i) on platforms where it's available
>> (ii) if the guest has PCI passthrough devices.  Is that right ?
> 
> Yes you are right VPCI will be enabled for guest when guest has PCI passthrough device
> assigned and VPCI support is available.
>>
>> Sorry to ask these question now, and please forgive my ignorance:
>>
>> Is VPCI inherently an ARM-specific ABI or protocol ?
> 
> As of now VPCI for DOMU guests is only implemented  for ARM.

We need to differentiate between what it is currently implemented and 
how it can be used in the future.

In particular, the layout of b_info is exposed to external toolstack 
(e.g. libvirt). So we can't easily remove an option. In other word, if 
we end up to need it for an other arch then we will have to keep some 
compat code.

In this case, I think this option is not arm specific. So the field 
ought to be outside of arch_arm.

>   
>>   When might an
>> admin want to turn it on explicitly ?
> 
> It will be enabled dynamically when admin assign any PCI device to guest.
> 
>>
>> How does this all relate to the (non-arch-specific) "passthrough"
>> option ?
> 
> VPCI will be enabled only when there is any PCI device assigned to guest therefore I used
> "d_config->num_pcidevs” to enable VPCI.

Ok. So we don't expect 'xl' or another toolstack to effectively touch 
the field for the time being. Is that correct?

If so, then I think this option should be hidden from external toolstack 
until we see a use.

Cheers,

-- 
Julien Grall

Hi Julien,

> On 6 Oct 2021, at 6:53 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> On 06/10/2021 19:34, Rahul Singh wrote:
>>> On 6 Oct 2021, at 12:33 pm, Ian Jackson <iwj@xenproject.org> wrote:
>>> 
>>> Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl"):
>>>> Hi Ian	
>>>>> What is wrong with putting it in
>>>>> libxl__arch_domain_build_info_setdefault
>>>>> which I think exists precisely for this kind of thing ?
>>>> 
>>>> As we have to set the arch_arm.vpci to false for x86 and ARM I
>>>> thought it is right to move the code to common code to avoid
>>>> duplication.
>>>> 
>>>> Are you suggesting to put "
>>>> libxl_defbool_setdefault(&b_info->arch_arm.vpci, false)ïżœin
>>>> libxl__arch_domain_build_info_setdefault() for x86 and ARM
>>>> differently.
>>> 
>>> I've gone back and reread the whole thread, which I probably should
>>> have done to start with....
>>> 
>>> So:
>>> 
>>>>>> #if defined(__arm__) || defined(__aarch64__)
>>>>>>   /*
>>>>>>    * Enable VPCI support for ARM. VPCI support for DOMU guests is not
>>>>>>    * supported for x86.
>>>>>>    */
>>>>>>   if (d_config->num_pcidevs)
>>>>>>     libxl_defbool_set(&b_info->arch_arm.vpci, true);
>>>>>> #endif
>>> 
>>> I think this logic probably ought to be in libxl, not in xl.
>> I will move the code to "libxl_arm.c"to avoid #ifdef in common code and also  to avoid setting the vpci for x86
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index e3140a6e00..2be208b99b 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -101,6 +101,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>          return ERROR_FAIL;
>>      }
>>  +    /* Enable VPCI support. */
>> +    if (d_config->num_pcidevs) {
>> +        config->flags |= XEN_DOMCTL_CDF_vpci;
>> +        libxl_defbool_set(&d_config->b_info.arch_arm.vpci, true);
>> +    }
>> +
>>      return 0;
>>  }
>>>  We try
>>> to make the libxl API "do the right thing" by default.  In this case I
>>> think that means to enable VPCI (i) on platforms where it's available
>>> (ii) if the guest has PCI passthrough devices.  Is that right ?
>> Yes you are right VPCI will be enabled for guest when guest has PCI passthrough device
>> assigned and VPCI support is available.
>>> 
>>> Sorry to ask these question now, and please forgive my ignorance:
>>> 
>>> Is VPCI inherently an ARM-specific ABI or protocol ?
>> As of now VPCI for DOMU guests is only implemented  for ARM.
> 
> We need to differentiate between what it is currently implemented and how it can be used in the future.
> 
> In particular, the layout of b_info is exposed to external toolstack (e.g. libvirt). So we can't easily remove an option. In other word, if we end up to need it for an other arch then we will have to keep some compat code.
> 
> In this case, I think this option is not arm specific. So the field ought to be outside of arch_arm.

As we are discussing whether we need this field or not If we reach to the conclusion the we need this field I will move this outside of arch_arm.
> 
>>  
>>>  When might an
>>> admin want to turn it on explicitly ?
>> It will be enabled dynamically when admin assign any PCI device to guest.
>>> 
>>> How does this all relate to the (non-arch-specific) "passthrough"
>>> option ?
>> VPCI will be enabled only when there is any PCI device assigned to guest therefore I used
>> "d_config->num_pcidevs” to enable VPCI.
> 
> Ok. So we don't expect 'xl' or another toolstack to effectively touch the field for the time being. Is that correct?

Yes it is correct. Only use of this field is used to create the DOMU emulated PCI device tree node when PCI device is assigned to guest ( d_config->num_pcidevs != NULL )
> 
> If so, then I think this option should be hidden from external toolstack until we see a use.

Stefano suggested in another email how we can remove this field. I will work on to remove this field.

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

Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl"):
> On 6 Oct 2021, at 12:33 pm, Ian Jackson <iwj@xenproject.org> wrote:
> >  We try
> > to make the libxl API "do the right thing" by default.  In this case I
> > think that means to enable VPCI (i) on platforms where it's available
> > (ii) if the guest has PCI passthrough devices.  Is that right ?
> 
> Yes you are right VPCI will be enabled for guest when guest has PCI passthrough device 
> assigned and VPCI support is available.  
> > 
> > Sorry to ask these question now, and please forgive my ignorance:
> > 
> > Is VPCI inherently an ARM-specific ABI or protocol ?
> 
> As of now VPCI for DOMU guests is only implemented  for ARM.

I'm sorry.  It appears that the thrust of my questions wasn't
sufficiently clear.  Your replies about details are fine but they
don't seem to address my underlying concerns.

"as of now ... only implemented for ARM" suggests to me that it is
VPCI *not* inherently ARM-specific.  Ie, it is a thing that x86 (or
riscv or whatever) might support in future.  Is that right ?

How does VPCI fit into the whole system architecture ?  Is it
*required* for PCI passthrough on ARM ?  If not, what happens if it is
not enabled ?

If VPCI *is* ARM-specific, how do x86 systems (say) achieve the goals
met on ARM by VPCI ?

On the other hand if VPCI is not inherently ARM-specific it should not
be in the ARM part of the libxl IDL.

> >  When might an
> > admin want to turn it on explicitly ?
> 
> It will be enabled dynamically when admin assign any PCI device to guest.

What about hotplug ?

> > How does this all relate to the (non-arch-specific) "passthrough"
> > option ?
> 
> VPCI will be enabled only when there is any PCI device assigned to
> guest therefore I used "d_config->num_pcidevs” to enable VPCI.

The purpose of the "passthrough" option is to allow the guest admin to
specify that a guest is expected to gain hotplugged PCI devices in
future.  That way, domain features that are required for PCI
passthrough are automatically enabled.

Perhaps this isn't explained clearly enough in the documentation,
which talks about iommu mappings.

Does PCI passthrugh work on ARM without VPCI ?

I think it likely that VPCI should be controlled (or at least, its
default set) from the "passthrough" option.  But I don't understand
enough of the relationship between the pieces.

Ian.

Hi Ian

> On 7 Oct 2021, at 11:50 am, Ian Jackson <iwj@xenproject.org> wrote:
> 
> Rahul Singh writes ("Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl"):
>> On 6 Oct 2021, at 12:33 pm, Ian Jackson <iwj@xenproject.org> wrote:
>>> We try
>>> to make the libxl API "do the right thing" by default.  In this case I
>>> think that means to enable VPCI (i) on platforms where it's available
>>> (ii) if the guest has PCI passthrough devices.  Is that right ?
>> 
>> Yes you are right VPCI will be enabled for guest when guest has PCI passthrough device 
>> assigned and VPCI support is available.  
>>> 
>>> Sorry to ask these question now, and please forgive my ignorance:
>>> 
>>> Is VPCI inherently an ARM-specific ABI or protocol ?
>> 
>> As of now VPCI for DOMU guests is only implemented  for ARM.
> 
> I'm sorry.  It appears that the thrust of my questions wasn't
> sufficiently clear.  Your replies about details are fine but they
> don't seem to address my underlying concerns.
> 
> "as of now ... only implemented for ARM" suggests to me that it is
> VPCI *not* inherently ARM-specific.  Ie, it is a thing that x86 (or
> riscv or whatever) might support in future.  Is that right ?

Sorry for the confusion VPCI is not ARM inherently specific. x86 or RISC might support 
VPCI in the future for DOMU guests.

> 
> How does VPCI fit into the whole system architecture ?  Is it
> *required* for PCI passthrough on ARM ?  If not, what happens if it is
> not enabled ?

VPCI implements the virtual PCI bus topology through IO emulation in XEN. Emulated device tree 
node will be created for the DOMU guests and all the access to the config space will be a trap to 
XEN.I/O memory regions for the device will be mapped to the guest and MSI/MSIX interrupts will 
be redirected to the guest.
 
Yes VPCI is mandatory for PCI passthrough on ARM. If it is not enabled we will not be
 able to pass through any PCI device to the guest.

> 
> If VPCI *is* ARM-specific, how do x86 systems (say) achieve the goals
> met on ARM by VPCI ?

On the x86 system, QEMU emulates the PCI bus topology and PV drivers used for communication.
> 
> On the other hand if VPCI is not inherently ARM-specific it should not
> be in the ARM part of the libxl IDL.
> 
>>> When might an
>>> admin want to turn it on explicitly ?
>> 
>> It will be enabled dynamically when admin assign any PCI device to guest.
> 
> What about hotplug ?

As of now hotplug is not implemented and tested. We might implement 
the hotplug in future..

> 
>>> How does this all relate to the (non-arch-specific) "passthrough"
>>> option ?
>> 
>> VPCI will be enabled only when there is any PCI device assigned to
>> guest therefore I used "d_config->num_pcidevsïżœ to enable VPCI.
> 
> The purpose of the "passthrough" option is to allow the guest admin to
> specify that a guest is expected to gain hotplugged PCI devices in
> future.  That way, domain features that are required for PCI
> passthrough are automatically enabled.

Once we implement the hotplug feature we will use the "passthrough=“ options.
> 
> Perhaps this isn't explained clearly enough in the documentation,
> which talks about iommu mappings.
> 
> Does PCI passthrugh work on ARM without VPCI ?

VPCI is required on ARM for PCI passthrough.

Regards,
Rahul
> 
> I think it likely that VPCI should be controlled (or at least, its
> default set) from the "passthrough" option.  But I don't understand
> enough of the relationship between the pieces.
> 
> Ian.