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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

> On 16 Dec 2021, at 9:48 pm, Stefano Stabellini <sstabellini@xxxxxxxxxx> wrote:
> 
> On Thu, 16 Dec 2021, Rahul Singh wrote:
>> 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().
> 
> OK. In that case the only feedback that is I have is that it might be
> possible to avoid the first change of this patch to skip "ranges" by
> moving the call to dt_pci_remove_io_ranges() earlier in the
> write_properties function.

Ok. I will modify the code based on your comment.

Regards,
Rahul