From: Rahul Singh <rahul.singh@arm.com>

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).

b_info.arch_arm.vpci is set by default to false
and it is not set to true anywhere. This way the
vpci node is not going to be created and we are only
introducing functions to create vpci DT node to be used
in the future.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Signed-off-by: Michal Orzel <michal.orzel@arm.com>
---
Changes in v6:
According to https://marc.info/?l=xen-devel&m=163415838129479&w=2:
-do not set XEN_DOMCTL_CDF_vpci
-do not enable VPCI support (by setting b_info.arch_arm.vpci)
-do not define LIBXL_HAVE_BUILDINFO_ARM_VPCI
-keep b_info.arch_arm.vpci, make_vpci_node and its helpers
Change in v5:
- Move setting the arch_arm.vpci and XEN_DOMCTL_CDF_vpci to libxl_arm.c
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/libs/light/libxl_arm.c     | 105 +++++++++++++++++++++++++++++++
 tools/libs/light/libxl_types.idl |   1 +
 xen/include/public/arch-arm.h    |  10 +++
 3 files changed, 116 insertions(+)

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_types.idl b/tools/libs/light/libxl_types.idl
index b96fb5c47e..61418ef6eb 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/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

(Replying to both the earlier subthread on v5 and to the new v6
patch.)

Bertrand Marquis writes ("Re: [PATCH v5 10/11] arm/libxl: Emulated PCI device tree node in libxl"):
> Now you suggest to add a new field arm_vpci in libxl__domain_create_state.

Hi.  I was handwaving, hence "probably" :-).  I hadn't actually looked
at the existing code to see precisely how it would fit.

> Once we have done that I will need to access this structure to know if I need
> to add the DT part and somehow to give it a value depending something which
> for now would the number of pcidevs as there will be no user parameter anymore.

Right.

I looked at libxl_arm.c again:

It seems that the main entrypoint to all this is libxl__prepare_dtb,
and it is that function you want to do new stuff.  That gets
libxl_domain_build_info (which is specified by the IDL and comes from
outside libxl, subject to libxl's default-filling machinery) and
libxl__domain_build_state (which is the general state struct).

The information you need is is libxl_domain_create_info.
libxl__domain_config_setdefault already arranges to set
c_info->passthrough based on the number of PCI PT devices
(search for `need_pt` in libxl_create.c).

That is, as I understand it on ARM vpci should be enabled if
passthrough is enabled and not otherwise.  That is precisely what
the variable c_info->passthrough is.

There is a slight issue because of the distinction between create_info
and build_info and domain_config (and, the corresponding _state)
structs.  Currently the DT code ony gets b_info, not the whole
libxl_domain_config.  These distinctions largely historical nowadays.
Certainly there is no reason not to pass a pointer to the whole
libxl_domain_config, rather than just libxl_domain_build_info, into
libxl__arch_domain_init_hw_description.

So I think the right approach for this looks something like this:

1. Change libxl__arch_domain_init_hw_description to take
   libxl_domain_config* rather than libxl_domain_build_info*.
   libxl_domain_config contains libxl_domain_build_info so
   this is easy.

   If you put in a convenience alias variable for the
   libxl_domain_build_info* you can avoid extra typing in the function
   body.  (If you call the convenience alias `info` you won't need to
   change the body at all, but maybe `info` isn't the best name so you
   could rename it to `b_info` maybe; up to you.)

2. Make the same change to libxl__prepare_dtb.

3. Now you can use d_config->c_info.passthrough to gate the addition
   of the additional stuff to the DT.  Ie, that is a respin of this
   patch 3/3.

1 and 2 need to be separated out from 3, in a different patch (or two
different patches) added to this series before what is now 3/3.  Ie
1+2, or 1 and 2 separately, should be pure plumbing changes with no
functional change.

I hope this is helpful.

Thanks,
Ian.

On Thu, 14 Oct 2021, Ian Jackson wrote:
> (Replying to both the earlier subthread on v5 and to the new v6
> patch.)
> 
> Bertrand Marquis writes ("Re: [PATCH v5 10/11] arm/libxl: Emulated PCI device tree node in libxl"):
> > Now you suggest to add a new field arm_vpci in libxl__domain_create_state.
> 
> Hi.  I was handwaving, hence "probably" :-).  I hadn't actually looked
> at the existing code to see precisely how it would fit.
> 
> > Once we have done that I will need to access this structure to know if I need
> > to add the DT part and somehow to give it a value depending something which
> > for now would the number of pcidevs as there will be no user parameter anymore.
> 
> Right.
> 
> I looked at libxl_arm.c again:
> 
> It seems that the main entrypoint to all this is libxl__prepare_dtb,
> and it is that function you want to do new stuff.  That gets
> libxl_domain_build_info (which is specified by the IDL and comes from
> outside libxl, subject to libxl's default-filling machinery) and
> libxl__domain_build_state (which is the general state struct).
> 
> The information you need is is libxl_domain_create_info.
> libxl__domain_config_setdefault already arranges to set
> c_info->passthrough based on the number of PCI PT devices
> (search for `need_pt` in libxl_create.c).
> 
> That is, as I understand it on ARM vpci should be enabled if
> passthrough is enabled and not otherwise.  That is precisely what
> the variable c_info->passthrough is.
> 
> There is a slight issue because of the distinction between create_info
> and build_info and domain_config (and, the corresponding _state)
> structs.  Currently the DT code ony gets b_info, not the whole
> libxl_domain_config.  These distinctions largely historical nowadays.
> Certainly there is no reason not to pass a pointer to the whole
> libxl_domain_config, rather than just libxl_domain_build_info, into
> libxl__arch_domain_init_hw_description.
> 
> So I think the right approach for this looks something like this:
> 
> 1. Change libxl__arch_domain_init_hw_description to take
>    libxl_domain_config* rather than libxl_domain_build_info*.
>    libxl_domain_config contains libxl_domain_build_info so
>    this is easy.
> 
>    If you put in a convenience alias variable for the
>    libxl_domain_build_info* you can avoid extra typing in the function
>    body.  (If you call the convenience alias `info` you won't need to
>    change the body at all, but maybe `info` isn't the best name so you
>    could rename it to `b_info` maybe; up to you.)
> 
> 2. Make the same change to libxl__prepare_dtb.
> 
> 3. Now you can use d_config->c_info.passthrough to gate the addition
>    of the additional stuff to the DT.  Ie, that is a respin of this
>    patch 3/3.
> 
> 1 and 2 need to be separated out from 3, in a different patch (or two
> different patches) added to this series before what is now 3/3.  Ie
> 1+2, or 1 and 2 separately, should be pure plumbing changes with no
> functional change.
> 
> I hope this is helpful.

The explanation is clear and makes sense to me too.

FYI I suggested something similar and Rahul agreed on it; in fact, he
seemed to have already made the change and tested it:
https://marc.info/?l=xen-devel&m=163362066215155

I am just mentioning it in case Bertrand might be able to find Rahul's
updated patch somewhere and it might be useful as a base for his work.

The rest looks good, including the new changes.

Hi Ian,

On 14/10/2021 18:54, Ian Jackson wrote:
> (Replying to both the earlier subthread on v5 and to the new v6
> patch.)
> 
> Bertrand Marquis writes ("Re: [PATCH v5 10/11] arm/libxl: Emulated PCI device tree node in libxl"):
>> Now you suggest to add a new field arm_vpci in libxl__domain_create_state.
> 
> Hi.  I was handwaving, hence "probably" :-).  I hadn't actually looked
> at the existing code to see precisely how it would fit.
> 
>> Once we have done that I will need to access this structure to know if I need
>> to add the DT part and somehow to give it a value depending something which
>> for now would the number of pcidevs as there will be no user parameter anymore.
> 
> Right.
> 
> I looked at libxl_arm.c again:
> 
> It seems that the main entrypoint to all this is libxl__prepare_dtb,
> and it is that function you want to do new stuff.  That gets
> libxl_domain_build_info (which is specified by the IDL and comes from
> outside libxl, subject to libxl's default-filling machinery) and
> libxl__domain_build_state (which is the general state struct).
> 
> The information you need is is libxl_domain_create_info.
> libxl__domain_config_setdefault already arranges to set
> c_info->passthrough based on the number of PCI PT devices
> (search for `need_pt` in libxl_create.c).
> 
> That is, as I understand it on ARM vpci should be enabled if
> passthrough is enabled and not otherwise.  That is precisely what
> the variable c_info->passthrough is.

On Arm, c_info->passthrough is also set when assigning platform devives 
(e.g. a non-PCI network card). At least for now, we don't want to create 
a node for vCPI in the Device-Tree because we don't enable the 
emulation. So...

> 
> There is a slight issue because of the distinction between create_info
> and build_info and domain_config (and, the corresponding _state)
> structs.  Currently the DT code ony gets b_info, not the whole
> libxl_domain_config.  These distinctions largely historical nowadays.
> Certainly there is no reason not to pass a pointer to the whole
> libxl_domain_config, rather than just libxl_domain_build_info, into
> libxl__arch_domain_init_hw_description.
> 
> So I think the right approach for this looks something like this:
> 
> 1. Change libxl__arch_domain_init_hw_description to take
>     libxl_domain_config* rather than libxl_domain_build_info*.
>     libxl_domain_config contains libxl_domain_build_info so
>     this is easy.
> 
>     If you put in a convenience alias variable for the
>     libxl_domain_build_info* you can avoid extra typing in the function
>     body.  (If you call the convenience alias `info` you won't need to
>     change the body at all, but maybe `info` isn't the best name so you
>     could rename it to `b_info` maybe; up to you.)
> 
> 2. Make the same change to libxl__prepare_dtb.
> 
> 3. Now you can use d_config->c_info.passthrough to gate the addition
>     of the additional stuff to the DT.  Ie, that is a respin of this
>     patch 3/3.

... we will need to check d_config->num_pcidevs for the time being.

Cheers,

-- 
Julien Grall

Hi guys,

On 15.10.2021 09:28, Julien Grall wrote:
> Hi Ian,
> 
> On 14/10/2021 18:54, Ian Jackson wrote:
>> (Replying to both the earlier subthread on v5 and to the new v6
>> patch.)
>>
>> Bertrand Marquis writes ("Re: [PATCH v5 10/11] arm/libxl: Emulated PCI device tree node in libxl"):
>>> Now you suggest to add a new field arm_vpci in libxl__domain_create_state.
>>
>> Hi.  I was handwaving, hence "probably" :-).  I hadn't actually looked
>> at the existing code to see precisely how it would fit.
>>
>>> Once we have done that I will need to access this structure to know if I need
>>> to add the DT part and somehow to give it a value depending something which
>>> for now would the number of pcidevs as there will be no user parameter anymore.
>>
>> Right.
>>
>> I looked at libxl_arm.c again:
>>
>> It seems that the main entrypoint to all this is libxl__prepare_dtb,
>> and it is that function you want to do new stuff.  That gets
>> libxl_domain_build_info (which is specified by the IDL and comes from
>> outside libxl, subject to libxl's default-filling machinery) and
>> libxl__domain_build_state (which is the general state struct).
>>
>> The information you need is is libxl_domain_create_info.
>> libxl__domain_config_setdefault already arranges to set
>> c_info->passthrough based on the number of PCI PT devices
>> (search for `need_pt` in libxl_create.c).
>>
>> That is, as I understand it on ARM vpci should be enabled if
>> passthrough is enabled and not otherwise.  That is precisely what
>> the variable c_info->passthrough is.
> 
> On Arm, c_info->passthrough is also set when assigning platform devives (e.g. a non-PCI network card). At least for now, we don't want to create a node for vCPI in the Device-Tree because we don't enable the emulation. So...
> 
>>
>> There is a slight issue because of the distinction between create_info
>> and build_info and domain_config (and, the corresponding _state)
>> structs.  Currently the DT code ony gets b_info, not the whole
>> libxl_domain_config.  These distinctions largely historical nowadays.
>> Certainly there is no reason not to pass a pointer to the whole
>> libxl_domain_config, rather than just libxl_domain_build_info, into
>> libxl__arch_domain_init_hw_description.
>>
>> So I think the right approach for this looks something like this:
>>
>> 1. Change libxl__arch_domain_init_hw_description to take
>>     libxl_domain_config* rather than libxl_domain_build_info*.
>>     libxl_domain_config contains libxl_domain_build_info so
>>     this is easy.
>>
>>     If you put in a convenience alias variable for the
>>     libxl_domain_build_info* you can avoid extra typing in the function
>>     body.  (If you call the convenience alias `info` you won't need to
>>     change the body at all, but maybe `info` isn't the best name so you
>>     could rename it to `b_info` maybe; up to you.)
>>
>> 2. Make the same change to libxl__prepare_dtb.
>>
>> 3. Now you can use d_config->c_info.passthrough to gate the addition
>>     of the additional stuff to the DT.  Ie, that is a respin of this
>>     patch 3/3.
> 
> ... we will need to check d_config->num_pcidevs for the time being.
> 
> Cheers,
> 

Thanks Julien for a good catch.

Do you agree on the following approach:
1. Modify argument of libxl__arch_domain_init_hw_description to libxl_domain_config (patch 1)
2. Modify argument of libxl__prepare_dtb to libxl_domain_config (patch 2)
3. Remove arch_arm.vpci completely and in libxl__prepare_dtb add a check (patch 3):
if (d_config->num_pcidevs)
    FDT( make_vpci_node(gc, fdt, ainfo, dom) );
+ make_vpci_node implementation

Does it sound ok for you?

Cheers,
Michal

Hi Michal,

On 15/10/2021 08:41, Michal Orzel wrote:
> Do you agree on the following approach:
> 1. Modify argument of libxl__arch_domain_init_hw_description to libxl_domain_config (patch 1)
> 2. Modify argument of libxl__prepare_dtb to libxl_domain_config (patch 2)
> 3. Remove arch_arm.vpci completely and in libxl__prepare_dtb add a check (patch 3):
> if (d_config->num_pcidevs)
>      FDT( make_vpci_node(gc, fdt, ainfo, dom) );
> + make_vpci_node implementation
> 
> Does it sound ok for you?

This sounds ok to me.

Cheers,

-- 
Julien Grall

Julien Grall writes ("Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]"):
> On 14/10/2021 18:54, Ian Jackson wrote:
...
> > That is, as I understand it on ARM vpci should be enabled if
> > passthrough is enabled and not otherwise.  That is precisely what
> > the variable c_info->passthrough is.
> 
> On Arm, c_info->passthrough is also set when assigning platform devives 
> (e.g. a non-PCI network card). At least for now, we don't want to create 
> a node for vCPI in the Device-Tree because we don't enable the 
> emulation. So...

Oh.

> > 3. Now you can use d_config->c_info.passthrough to gate the addition
> >     of the additional stuff to the DT.  Ie, that is a respin of this
> >     patch 3/3.
> 
> ... we will need to check d_config->num_pcidevs for the time being.

OK.

Can you leave a comment somewhere (near where c_info.passthrough is
set) pointing to this use of num_pcidevs ?  That might save someone
some future confusion.

Thanks,
Ian.

Hi,

On 15.10.2021 12:02, Ian Jackson wrote:
> Julien Grall writes ("Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]"):
>> On 14/10/2021 18:54, Ian Jackson wrote:
> ...
>>> That is, as I understand it on ARM vpci should be enabled if
>>> passthrough is enabled and not otherwise.  That is precisely what
>>> the variable c_info->passthrough is.
>>
>> On Arm, c_info->passthrough is also set when assigning platform devives 
>> (e.g. a non-PCI network card). At least for now, we don't want to create 
>> a node for vCPI in the Device-Tree because we don't enable the 
>> emulation. So...
> 
> Oh.
> 
>>> 3. Now you can use d_config->c_info.passthrough to gate the addition
>>>     of the additional stuff to the DT.  Ie, that is a respin of this
>>>     patch 3/3.
>>
>> ... we will need to check d_config->num_pcidevs for the time being.
> 
> OK.
> 
> Can you leave a comment somewhere (near where c_info.passthrough is
> set) pointing to this use of num_pcidevs ?  That might save someone
> some future confusion.
> 
Here, c_info->passthrough is set to enabled if either d_config->num_pcidevs or
d_config->num_dtdevs is set. Do you think we need to add there additional comment?
If so can you please help with what should I write there?

BTW. None of the patch I'm preparing with regards to this discussion modifies libxl_create
where c_info.passthrough i set. Do you still want me to add some comment there?
> Thanks,
> Ian.
> 
Cheers,
Michal

On 15.10.2021 12:58, Michal Orzel wrote:
> Hi,
> 
> On 15.10.2021 12:02, Ian Jackson wrote:
>> Julien Grall writes ("Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]"):
>>> On 14/10/2021 18:54, Ian Jackson wrote:
>> ...
>>>> That is, as I understand it on ARM vpci should be enabled if
>>>> passthrough is enabled and not otherwise.  That is precisely what
>>>> the variable c_info->passthrough is.
>>>
>>> On Arm, c_info->passthrough is also set when assigning platform devives 
>>> (e.g. a non-PCI network card). At least for now, we don't want to create 
>>> a node for vCPI in the Device-Tree because we don't enable the 
>>> emulation. So...
>>
>> Oh.
>>
>>>> 3. Now you can use d_config->c_info.passthrough to gate the addition
>>>>     of the additional stuff to the DT.  Ie, that is a respin of this
>>>>     patch 3/3.
>>>
>>> ... we will need to check d_config->num_pcidevs for the time being.
>>
>> OK.
>>
>> Can you leave a comment somewhere (near where c_info.passthrough is
>> set) pointing to this use of num_pcidevs ?  That might save someone
>> some future confusion.
>>
> Here, c_info->passthrough is set to enabled if either d_config->num_pcidevs or
> d_config->num_dtdevs is set. Do you think we need to add there additional comment?
> If so can you please help with what should I write there?
> 
I forgot to add a link:
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=tools/libs/light/libxl_create.c;h=6ebb2bfc768d060fb898619be907fc973375cce5;hb=HEAD#l1099
> BTW. None of the patch I'm preparing with regards to this discussion modifies libxl_create
> where c_info.passthrough i set. Do you still want me to add some comment there?
>> Thanks,
>> Ian.
>>
> Cheers,
> Michal
> 

Michal Orzel writes ("Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]"):
> On 15.10.2021 12:02, Ian Jackson wrote:
> > Can you leave a comment somewhere (near where c_info.passthrough is
> > set) pointing to this use of num_pcidevs ?  That might save someone
> > some future confusion.
> 
> Here, c_info->passthrough is set to enabled if either d_config->num_pcidevs or
> d_config->num_dtdevs is set. Do you think we need to add there additional comment?
> If so can you please help with what should I write there?
> 
> BTW. None of the patch I'm preparing with regards to this discussion modifies libxl_create
> where c_info.passthrough i set. Do you still want me to add some comment there?

Yes, I think so.  I won't insisting on it if you feel it doesn't make
sense.

Maybe something like

  // NB, on ARM, libxl__arch_blah directly examines num_pcidevs to
  // decide whether to enable vpci, rather than using c_info->passthrough.
  // This will be insufficient if and when ARM does PCI hotplug.

?

Ian.

On 15.10.2021 13:46, Ian Jackson wrote:
> Michal Orzel writes ("Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]"):
>> On 15.10.2021 12:02, Ian Jackson wrote:
>>> Can you leave a comment somewhere (near where c_info.passthrough is
>>> set) pointing to this use of num_pcidevs ?  That might save someone
>>> some future confusion.
>>
>> Here, c_info->passthrough is set to enabled if either d_config->num_pcidevs or
>> d_config->num_dtdevs is set. Do you think we need to add there additional comment?
>> If so can you please help with what should I write there?
>>
>> BTW. None of the patch I'm preparing with regards to this discussion modifies libxl_create
>> where c_info.passthrough i set. Do you still want me to add some comment there?
> 
> Yes, I think so.  I won't insisting on it if you feel it doesn't make
> sense.
> 
> Maybe something like
> 
>   // NB, on ARM, libxl__arch_blah directly examines num_pcidevs to
>   // decide whether to enable vpci, rather than using c_info->passthrough.
>   // This will be insufficient if and when ARM does PCI hotplug.
> 
Well we are not enabling vpci. We are creating a DT node for it.
So either I will write:
/*
 * Note: libxl_arm directly examines num_pcidevs to decide whether
 * to create a vPCI DT node, rather than using c_info->passthrough.
 * This will be insufficient if and when ARM does PCI hotplug.
 */

or I will not add any comment (it can be add in the future when "enabling" vpci).
@Julien?

> ?
> 
> Ian.
> 

Cheers,
Michal

Hi Michal,

On 15/10/2021 12:53, Michal Orzel wrote:
> 
> 
> On 15.10.2021 13:46, Ian Jackson wrote:
>> Michal Orzel writes ("Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]"):
>>> On 15.10.2021 12:02, Ian Jackson wrote:
>>>> Can you leave a comment somewhere (near where c_info.passthrough is
>>>> set) pointing to this use of num_pcidevs ?  That might save someone
>>>> some future confusion.
>>>
>>> Here, c_info->passthrough is set to enabled if either d_config->num_pcidevs or
>>> d_config->num_dtdevs is set. Do you think we need to add there additional comment?
>>> If so can you please help with what should I write there?
>>>
>>> BTW. None of the patch I'm preparing with regards to this discussion modifies libxl_create
>>> where c_info.passthrough i set. Do you still want me to add some comment there?
>>
>> Yes, I think so.  I won't insisting on it if you feel it doesn't make
>> sense.
>>
>> Maybe something like
>>
>>    // NB, on ARM, libxl__arch_blah directly examines num_pcidevs to
>>    // decide whether to enable vpci, rather than using c_info->passthrough.
>>    // This will be insufficient if and when ARM does PCI hotplug.
>>
> Well we are not enabling vpci. We are creating a DT node for it.
> So either I will write:
> /*
>   * Note: libxl_arm directly examines num_pcidevs to decide whether
>   * to create a vPCI DT node, rather than using c_info->passthrough.
>   * This will be insufficient if and when ARM does PCI hotplug.
>   */
> 
> or I will not add any comment (it can be add in the future when "enabling" vpci).
> @Julien?

I would prefer if we had a comment. Your proposal makes more sense as we 
only create the DT.

Long term, I would expect a similar check to be necessary to set the 
vCPI flag at the domain creation. So it would be best to introduce an 
internal field 'vpci' to avoid duplicating that check. (Note I am not 
requesting this change for Xen 4.16).

Cheers,

-- 
Julien Grall

Julien Grall writes ("Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]"):
> Long term, I would expect a similar check to be necessary to set the 
> vCPI flag at the domain creation. So it would be best to introduce an 
> internal field 'vpci' to avoid duplicating that check. (Note I am not 
> requesting this change for Xen 4.16).

Yes, I agree with all of that.

Ian.

Michal Orzel writes ("Re: [PATCH v6 3/3] arm/libxl: Emulated PCI device tree node in libxl [and 1 more messages]"):
> On 15.10.2021 13:46, Ian Jackson wrote:
> > Maybe something like
> > 
> >   // NB, on ARM, libxl__arch_blah directly examines num_pcidevs to
> >   // decide whether to enable vpci, rather than using c_info->passthrough.
> >   // This will be insufficient if and when ARM does PCI hotplug.
> 
> Well we are not enabling vpci. We are creating a DT node for it.
> So either I will write:
> /*
>  * Note: libxl_arm directly examines num_pcidevs to decide whether
>  * to create a vPCI DT node, rather than using c_info->passthrough.
>  * This will be insufficient if and when ARM does PCI hotplug.
>  */

That sounds good to me.  Thank you for correcting me :-).

Ian.