|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 13/14] arm/libxl: Emulated PCI device tree node in libxl
On Tue, 5 Oct 2021, Rahul Singh wrote:
> > On 5 Oct 2021, at 1:38 am, Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > 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@xxxxxxx>
> >> ---
> >> 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 = ®s[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.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |