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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.