Re: [PATCH v7 5/5] arm/libxl: Emulated PCI device tree node in libxl

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Julien,

> On 15 Oct 2021, at 15:52, Julien Grall <julien@xxxxxxx> wrote:
> 
> Hi Bertrand,
> 
> On 15/10/2021 14:59, Bertrand Marquis wrote:
>> From: Rahul Singh <rahul.singh@xxxxxxx>
>> 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).
>> Note that currently we are using num_pcidevs instead of
>> c_info->passthrough to decide whether to create a vPCI DT node.
>> This will be insufficient if and when ARM does PCI hotplug.
>> Add this note inside libxl_create.c where c_info->passthrough
>> is set.
>> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> 
> I believe you need to add your signed-off-by here as you sent the new version 
> on xen-devel.

I packed it together but the changes where done by Michal so I did not add it.

Could it be added on commit ?

> 
> With one remark below (not to be handled for 4.16):
> 
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

Thanks

> 
>> +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,
> 
> From my understanding, the prefetch memory region is optional. Is it going to 
> be a problem to expose one for the vCPI when the host PCI hostbridge may not 
> support it?

Good point but I would guess no as the host bridge should not use it.
I keep that as a point to check with Rahul.

Cheers
Bertrand

> 
> Cheers,
> 
> -- 
> Julien Grall