Re: [PATCH] xen/arm: vpci: Remove PCI I/O ranges property value

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 14 Dec 2021, Rahul Singh wrote:
> IO ports on ARM don't exist so all IO ports related hypercalls are going
> to fail on ARM when we passthrough a PCI device.
> Failure of xc_domain_ioport_permission(..) would turn into a critical
> failure at domain creation. We need to avoid this outcome, instead we
> want to continue with domain creation as normal even if
> xc_domain_ioport_permission(..) fails. XEN_DOMCTL_ioport_permission
> is not implemented on ARM so it would return -ENOSYS.
> 
> To solve above issue remove PCI I/O ranges property value from dom0
> device tree node so that dom0 linux will not allocate I/O space for PCI
> devices if pci-passthrough is enabled.
> 
> Another valid reason to remove I/O ranges is that PCI I/O space are not
> mapped to dom0 when PCI passthrough is enabled, also there is no vpci
> trap handler register for IO bar.
> 
> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> ---
>  xen/arch/arm/domain_build.c   | 14 +++++++
>  xen/common/device_tree.c      | 72 +++++++++++++++++++++++++++++++++++
>  xen/include/xen/device_tree.h | 10 +++++
>  3 files changed, 96 insertions(+)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index d02bacbcd1..60f6b2c73b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -749,6 +749,11 @@ static int __init write_properties(struct domain *d, 
> struct kernel_info *kinfo,
>                  continue;
>          }
>  
> +        if ( is_pci_passthrough_enabled() &&
> +                dt_device_type_is_equal(node, "pci") )
> +            if ( dt_property_name_is_equal(prop, "ranges") )
> +                continue;

It looks like we are skipping the "ranges" property entirely for the PCI
node, is that right? Wouldn't that also remove the other (not ioports)
address ranges?


>          res = fdt_property(kinfo->fdt, prop->name, prop_data, prop_len);
>  
>          if ( res )
> @@ -769,6 +774,15 @@ static int __init write_properties(struct domain *d, 
> struct kernel_info *kinfo,
>              if ( res )
>                  return res;
>          }
> +
> +        /*
> +         * PCI IO bar are not mapped to dom0 when PCI passthrough is enabled,
> +         * also there is no trap handler registered for IO bar therefor 
> remove
> +         * the IO range property from the device tree node for dom0.
> +         */
> +        res = dt_pci_remove_io_ranges(kinfo->fdt, node);
> +        if ( res )
> +            return res;

I tried to apply this patch to staging to make it easier to review but I
think this chuck got applied wrongly: I can see
dt_pci_remove_io_ranges() added to the function "handle_prop_pfdt" which
is for guest partial DTBs and not for dom0.

Is dt_pci_remove_io_ranges() meant to be called from write_properties
instead? It looks like it would be best to call it from
write_properties, maybe it could be combined with the other new check
just above in this patch?


>      /*
> diff --git a/xen/common/device_tree.c b/xen/common/device_tree.c
> index 4aae281e89..9fa25f6723 100644
> --- a/xen/common/device_tree.c
> +++ b/xen/common/device_tree.c
> @@ -2195,6 +2195,78 @@ int dt_get_pci_domain_nr(struct dt_device_node *node)
>      return (u16)domain;
>  }
>  
> +int dt_pci_remove_io_ranges(void *fdt, const struct dt_device_node *dev)
> +{
> +    const struct dt_device_node *parent = NULL;
> +    const struct dt_bus *bus, *pbus;
> +    unsigned int rlen;
> +    int na, ns, pna, pns, rone, ret;
> +    const __be32 *ranges;
> +    __be32 regs[((GUEST_ROOT_ADDRESS_CELLS * 2) + GUEST_ROOT_SIZE_CELLS + 1)
> +               * 2];
> +    __be32 *addr = &regs[0];
> +
> +    bus = dt_match_bus(dev);
> +    if ( !bus )
> +        return 0; /* device is not a bus */
> +
> +    parent = dt_get_parent(dev);
> +    if ( parent == NULL )
> +        return -EINVAL;
> +
> +    ranges = dt_get_property(dev, "ranges", &rlen);
> +    if ( ranges == NULL )
> +    {
> +        printk(XENLOG_ERR "DT: no ranges; cannot enumerate %s\n",
> +               dev->full_name);
> +        return -EINVAL;
> +    }
> +    if ( rlen == 0 ) /* Nothing to do */
> +        return 0;
> +
> +    bus->count_cells(dev, &na, &ns);
> +    if ( !DT_CHECK_COUNTS(na, ns) )
> +    {
> +        printk(XENLOG_ERR "dt_parse: Bad cell count for device %s\n",
> +                  dev->full_name);
> +        return -EINVAL;
> +    }
> +    pbus = dt_match_bus(parent);
> +    if ( pbus == NULL )
> +    {
> +        printk("DT: %s is not a valid bus\n", parent->full_name);
> +        return -EINVAL;
> +    }
> +
> +    pbus->count_cells(dev, &pna, &pns);
> +    if ( !DT_CHECK_COUNTS(pna, pns) )
> +    {
> +        printk(XENLOG_ERR "dt_parse: Bad cell count for parent %s\n",
> +               dev->full_name);
> +        return -EINVAL;
> +    }
> +    /* Now walk through the ranges */
> +    rlen /= 4;
> +    rone = na + pna + ns;
> +
> +    for ( ; rlen >= rone; rlen -= rone, ranges += rone )
> +    {
> +        unsigned int flags = bus->get_flags(ranges);
> +        if ( flags & IORESOURCE_IO )
> +            continue;
> +
> +        memcpy(addr, ranges, 4 * rone);
> +
> +        addr += rone;
> +    }
> +
> +    ret = fdt_property(fdt, "ranges", regs, sizeof(regs));
> +    if ( ret )
> +        return ret;
> +
> +    return 0;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h
> index fd6cd00b43..ad2e905595 100644
> --- a/xen/include/xen/device_tree.h
> +++ b/xen/include/xen/device_tree.h
> @@ -849,6 +849,16 @@ int dt_count_phandle_with_args(const struct 
> dt_device_node *np,
>   */
>  int dt_get_pci_domain_nr(struct dt_device_node *node);
>  
> +/**
> + * dt_get_remove_io_range - Remove the PCI I/O range property value.
> + * @fdt: Pointer to the file descriptor tree.
> + * @node: Device tree node.
> + *
> + * This function will remove the PCI IO range property from the PCI device 
> tree
> + * node.
> + */
> +int dt_pci_remove_io_ranges(void *fdt, const struct dt_device_node *node);
> +
>  struct dt_device_node *dt_find_node_by_phandle(dt_phandle handle);
>  
>  #ifdef CONFIG_DEVICE_TREE_DEBUG
> -- 
> 2.25.1
>