Hi Stefano, > On 16 Dec 2021, at 9:48 pm, Stefano Stabellini <[email protected]> wrote: > > On Thu, 16 Dec 2021, Rahul Singh wrote: >> Hi Stefano, >> >>> On 16 Dec 2021, at 2:33 am, Stefano Stabellini <[email protected]> >>> 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 <[email protected]> >>>> --- >>>> 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
