| Message ID | d1db7461eb4ae3f9423814877c5ab6d946dae4e7.1662548825.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/pci: implement is_memory_hole for ARM | expand |
On 07.09.2022 13:09, Rahul Singh wrote: > is_memory_hole was implemented for x86 and not for ARM when introduced. > Replace is_memory_hole call to pci_check_bar as function should check > if device BAR is in defined memory range. Also, add an implementation > for ARM which is required for PCI passthrough. > > On x86, pci_check_bar will call is_memory_hole which will check if BAR > is not overlapping with any memory region defined in the memory map. > > On ARM, pci_check_bar will go through the host bridge ranges and check > if the BAR is in the range of defined ranges. > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > --- > Changes in v4: > - check "s <= e" before callback > - Add TODO comment for revisiting the function pci_check_bar() when > ACPI PCI passthrough support is added. > - Not Added the Jan Acked-by as patch is modified. Hmm, I don't see any change to the parts the ack covers (x86 and common), so please re-instate. I'm not in the position to ack Arm changes; things would be different for a Reviewed-by without scope restriction. Jan
Hi Jan, On 07/09/2022 13:06, Jan Beulich wrote: > On 07.09.2022 13:09, Rahul Singh wrote: >> is_memory_hole was implemented for x86 and not for ARM when introduced. >> Replace is_memory_hole call to pci_check_bar as function should check >> if device BAR is in defined memory range. Also, add an implementation >> for ARM which is required for PCI passthrough. >> >> On x86, pci_check_bar will call is_memory_hole which will check if BAR >> is not overlapping with any memory region defined in the memory map. >> >> On ARM, pci_check_bar will go through the host bridge ranges and check >> if the BAR is in the range of defined ranges. >> >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> --- >> Changes in v4: >> - check "s <= e" before callback >> - Add TODO comment for revisiting the function pci_check_bar() when >> ACPI PCI passthrough support is added. >> - Not Added the Jan Acked-by as patch is modified. > > Hmm, I don't see any change to the parts the ack covers (x86 and common), > so please re-instate. I'm not in the position to ack Arm changes; things > would be different for a Reviewed-by without scope restriction. I would like the question about the BAR alignment to be resolved before giving my Reviewed-by/Acked-by here. Hopefully Stefano can clarify it. Cheers,
Hi Jan, > On 7 Sep 2022, at 13:06, Jan Beulich <jbeulich@suse.com> wrote: > > On 07.09.2022 13:09, Rahul Singh wrote: >> is_memory_hole was implemented for x86 and not for ARM when introduced. >> Replace is_memory_hole call to pci_check_bar as function should check >> if device BAR is in defined memory range. Also, add an implementation >> for ARM which is required for PCI passthrough. >> >> On x86, pci_check_bar will call is_memory_hole which will check if BAR >> is not overlapping with any memory region defined in the memory map. >> >> On ARM, pci_check_bar will go through the host bridge ranges and check >> if the BAR is in the range of defined ranges. >> >> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >> --- >> Changes in v4: >> - check "s <= e" before callback >> - Add TODO comment for revisiting the function pci_check_bar() when >> ACPI PCI passthrough support is added. >> - Not Added the Jan Acked-by as patch is modified. > > Hmm, I don't see any change to the parts the ack covers (x86 and common), > so please re-instate. I'm not in the position to ack Arm changes; things > would be different for a Reviewed-by without scope restriction. This might have been clear for you but your ack was not mentioning anything. As the newer version is modified anyway, we remove it. But I understand from your answer that your ack is still valid for this version. Thanks for this. Bertrand > > Jan
On 07.09.2022 15:13, Bertrand Marquis wrote: > Hi Jan, > >> On 7 Sep 2022, at 13:06, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 07.09.2022 13:09, Rahul Singh wrote: >>> is_memory_hole was implemented for x86 and not for ARM when introduced. >>> Replace is_memory_hole call to pci_check_bar as function should check >>> if device BAR is in defined memory range. Also, add an implementation >>> for ARM which is required for PCI passthrough. >>> >>> On x86, pci_check_bar will call is_memory_hole which will check if BAR >>> is not overlapping with any memory region defined in the memory map. >>> >>> On ARM, pci_check_bar will go through the host bridge ranges and check >>> if the BAR is in the range of defined ranges. >>> >>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>> --- >>> Changes in v4: >>> - check "s <= e" before callback >>> - Add TODO comment for revisiting the function pci_check_bar() when >>> ACPI PCI passthrough support is added. >>> - Not Added the Jan Acked-by as patch is modified. >> >> Hmm, I don't see any change to the parts the ack covers (x86 and common), >> so please re-instate. I'm not in the position to ack Arm changes; things >> would be different for a Reviewed-by without scope restriction. > > This might have been clear for you but your ack was not mentioning anything. > As the newer version is modified anyway, we remove it. An ack can only ever apply to changes to files the person offering the tag is a maintainer of. Hence there's rarely a reason to scope-restrict it. As opposed to Reviewed-by, where someone may indeed have reviewed only part of a patch. > But I understand from your answer that your ack is still valid for this version. That's correct. Jan
Hi, > On 7 Sep 2022, at 13:48, Julien Grall <julien@xen.org> wrote: > > Hi Jan, > > On 07/09/2022 13:06, Jan Beulich wrote: >> On 07.09.2022 13:09, Rahul Singh wrote: >>> is_memory_hole was implemented for x86 and not for ARM when introduced. >>> Replace is_memory_hole call to pci_check_bar as function should check >>> if device BAR is in defined memory range. Also, add an implementation >>> for ARM which is required for PCI passthrough. >>> >>> On x86, pci_check_bar will call is_memory_hole which will check if BAR >>> is not overlapping with any memory region defined in the memory map. >>> >>> On ARM, pci_check_bar will go through the host bridge ranges and check >>> if the BAR is in the range of defined ranges. >>> >>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>> --- >>> Changes in v4: >>> - check "s <= e" before callback >>> - Add TODO comment for revisiting the function pci_check_bar() when >>> ACPI PCI passthrough support is added. >>> - Not Added the Jan Acked-by as patch is modified. >> Hmm, I don't see any change to the parts the ack covers (x86 and common), >> so please re-instate. I'm not in the position to ack Arm changes; things >> would be different for a Reviewed-by without scope restriction. > > I would like the question about the BAR alignment to be resolved before giving my Reviewed-by/Acked-by here. In here we have a range specified by the host controller and the bar should be strictly inside that range. Now if the controller range is not properly aligned, there is an error in the device tree or ACPI table or in the firmware and we should have detected that earlier. The itself BAR should fit inside the controller range boundary. If the BAR start and end are not properly aligned we cannot map them so this should be an error (if we have to map them). I am not entirely sure we should try to check this here or maybe would it be easier to fail if addr is not aligned or size is not aligned if we want to check something. Bertrand > > Hopefully Stefano can clarify it. > > Cheers, > > -- > Julien Grall
Hi, > On 7 Sep 2022, at 14:21, Jan Beulich <jbeulich@suse.com> wrote: > > On 07.09.2022 15:13, Bertrand Marquis wrote: >> Hi Jan, >> >>> On 7 Sep 2022, at 13:06, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 07.09.2022 13:09, Rahul Singh wrote: >>>> is_memory_hole was implemented for x86 and not for ARM when introduced. >>>> Replace is_memory_hole call to pci_check_bar as function should check >>>> if device BAR is in defined memory range. Also, add an implementation >>>> for ARM which is required for PCI passthrough. >>>> >>>> On x86, pci_check_bar will call is_memory_hole which will check if BAR >>>> is not overlapping with any memory region defined in the memory map. >>>> >>>> On ARM, pci_check_bar will go through the host bridge ranges and check >>>> if the BAR is in the range of defined ranges. >>>> >>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com> >>>> --- >>>> Changes in v4: >>>> - check "s <= e" before callback >>>> - Add TODO comment for revisiting the function pci_check_bar() when >>>> ACPI PCI passthrough support is added. >>>> - Not Added the Jan Acked-by as patch is modified. >>> >>> Hmm, I don't see any change to the parts the ack covers (x86 and common), >>> so please re-instate. I'm not in the position to ack Arm changes; things >>> would be different for a Reviewed-by without scope restriction. >> >> This might have been clear for you but your ack was not mentioning anything. >> As the newer version is modified anyway, we remove it. > > An ack can only ever apply to changes to files the person offering the > tag is a maintainer of. Hence there's rarely a reason to scope-restrict > it. As opposed to Reviewed-by, where someone may indeed have reviewed > only part of a patch. Distinction of scope handling between ack and R-b here would require to check in the MAINTAINERS which parts are in the scope. Maybe explicitly putting the scope with the ack could be useful here. > >> But I understand from your answer that your ack is still valid for this version. > > That's correct. > Thanks Bertrand > Jan
On Wed, 7 Sep 2022, Julien Grall wrote: > Hi Jan, > > On 07/09/2022 13:06, Jan Beulich wrote: > > On 07.09.2022 13:09, Rahul Singh wrote: > > > is_memory_hole was implemented for x86 and not for ARM when introduced. > > > Replace is_memory_hole call to pci_check_bar as function should check > > > if device BAR is in defined memory range. Also, add an implementation > > > for ARM which is required for PCI passthrough. > > > > > > On x86, pci_check_bar will call is_memory_hole which will check if BAR > > > is not overlapping with any memory region defined in the memory map. > > > > > > On ARM, pci_check_bar will go through the host bridge ranges and check > > > if the BAR is in the range of defined ranges. > > > > > > Signed-off-by: Rahul Singh <rahul.singh@arm.com> > > > --- > > > Changes in v4: > > > - check "s <= e" before callback > > > - Add TODO comment for revisiting the function pci_check_bar() when > > > ACPI PCI passthrough support is added. > > > - Not Added the Jan Acked-by as patch is modified. > > > > Hmm, I don't see any change to the parts the ack covers (x86 and common), > > so please re-instate. I'm not in the position to ack Arm changes; things > > would be different for a Reviewed-by without scope restriction. > > I would like the question about the BAR alignment to be resolved before giving > my Reviewed-by/Acked-by here. > > Hopefully Stefano can clarify it. We had a discussion this morning and this is the summary. BAR addresses and Root Complex window addresses are not guaranteed to be page aligned. If they are not guaranteed to be page aligned we should check for alignment, however, this is not the right location for the alignment check. It is a good idea to add a TODO comment in the code. So that we don't relax the check in case the addresses are not aligned, we should drop the use of PFN_UP and PFN_DOWN in this check.
diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h index 80a2431804..8cb46f6b71 100644 --- a/xen/arch/arm/include/asm/pci.h +++ b/xen/arch/arm/include/asm/pci.h @@ -126,6 +126,8 @@ int pci_host_iterate_bridges_and_count(struct domain *d, int pci_host_bridge_mappings(struct domain *d); +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end); + #else /*!CONFIG_HAS_PCI*/ struct arch_pci_dev { }; diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c index 89ef30028e..13d419aa45 100644 --- a/xen/arch/arm/pci/pci-host-common.c +++ b/xen/arch/arm/pci/pci-host-common.c @@ -24,6 +24,16 @@ #include <asm/setup.h> +/* + * struct to hold pci device bar. + */ +struct pdev_bar_check +{ + unsigned long start; + unsigned long end; + bool is_valid; +}; + /* * List for all the pci host bridges. */ @@ -363,6 +373,45 @@ int __init pci_host_bridge_mappings(struct domain *d) return 0; } +static int is_bar_valid(const struct dt_device_node *dev, + uint64_t addr, uint64_t len, void *data) +{ + struct pdev_bar_check *bar_data = data; + unsigned long s = bar_data->start; + unsigned long e = bar_data->end; + + if ( (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) + bar_data->is_valid = true; + + return 0; +} + +/* TODO: Revisit this function when ACPI PCI passthrough support is added. */ +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end) +{ + int ret; + const struct dt_device_node *dt_node; + unsigned long s = mfn_x(start); + unsigned long e = mfn_x(end); + struct pdev_bar_check bar_data = { + .start = s, + .end = e, + .is_valid = false + }; + + if ( s >= e ) + return false; + + dt_node = pci_find_host_bridge_node(pdev); + if ( !dt_node ) + return false; + + ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data); + if ( ret < 0 ) + return false; + + return bar_data.is_valid; +} /* * Local variables: * mode: C diff --git a/xen/arch/x86/include/asm/pci.h b/xen/arch/x86/include/asm/pci.h index c8e1a9ecdb..f4a58c8acf 100644 --- a/xen/arch/x86/include/asm/pci.h +++ b/xen/arch/x86/include/asm/pci.h @@ -57,4 +57,14 @@ static always_inline bool is_pci_passthrough_enabled(void) void arch_pci_init_pdev(struct pci_dev *pdev); +static inline bool pci_check_bar(const struct pci_dev *pdev, + mfn_t start, mfn_t end) +{ + /* + * Check if BAR is not overlapping with any memory region defined + * in the memory map. + */ + return is_memory_hole(start, end); +} + #endif /* __X86_PCI_H__ */ diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c index cdaf5c247f..149f68bb6e 100644 --- a/xen/drivers/passthrough/pci.c +++ b/xen/drivers/passthrough/pci.c @@ -304,8 +304,8 @@ static void check_pdev(const struct pci_dev *pdev) if ( rc < 0 ) /* Unable to size, better leave memory decoding disabled. */ return; - if ( size && !is_memory_hole(maddr_to_mfn(addr), - maddr_to_mfn(addr + size - 1)) ) + if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr), + maddr_to_mfn(addr + size - 1)) ) { /* * Return without enabling memory decoding if BAR position is not @@ -331,8 +331,8 @@ static void check_pdev(const struct pci_dev *pdev) if ( rc < 0 ) return; - if ( size && !is_memory_hole(maddr_to_mfn(addr), - maddr_to_mfn(addr + size - 1)) ) + if ( size && !pci_check_bar(pdev, maddr_to_mfn(addr), + maddr_to_mfn(addr + size - 1)) ) { printk(warn, &pdev->sbdf, "ROM ", PFN_DOWN(addr), PFN_DOWN(addr + size - 1));
is_memory_hole was implemented for x86 and not for ARM when introduced. Replace is_memory_hole call to pci_check_bar as function should check if device BAR is in defined memory range. Also, add an implementation for ARM which is required for PCI passthrough. On x86, pci_check_bar will call is_memory_hole which will check if BAR is not overlapping with any memory region defined in the memory map. On ARM, pci_check_bar will go through the host bridge ranges and check if the BAR is in the range of defined ranges. Signed-off-by: Rahul Singh <rahul.singh@arm.com> --- Changes in v4: - check "s <= e" before callback - Add TODO comment for revisiting the function pci_check_bar() when ACPI PCI passthrough support is added. - Not Added the Jan Acked-by as patch is modified. Changes in v3: - fix minor comments --- xen/arch/arm/include/asm/pci.h | 2 ++ xen/arch/arm/pci/pci-host-common.c | 49 ++++++++++++++++++++++++++++++ xen/arch/x86/include/asm/pci.h | 10 ++++++ xen/drivers/passthrough/pci.c | 8 ++--- 4 files changed, 65 insertions(+), 4 deletions(-)