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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

> On 16 Dec 2021, at 2:33 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> 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?

We are skipping the “ranges” property here to avoid setting the “ranges” 
property when
pci_passthrough is enabled. We will remove only remove IO port and set the 
other ‘ranges” property 
value in dt_pci_remove_io_ranges() in next if() condition.
 

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

Oleksandr’s patch series was merged yesterday because of that there is conflict 
in applying 
this patch. I will rebase the patch and will send it again for review.

> 
> 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?

Yes dt_pci_remove_io_ranges() is to be called from write_properties().

Regards,
Rahul
> 
> 
>>     /*
>> 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