| Message ID | e30beac1480f03b51933d8016ad9aed8855ffc18.1662024325.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/pci: implement is_memory_hole for ARM | expand |
On 01.09.22 12:29, Rahul Singh wrote: Hello Rahul > 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> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com> Thanks! > --- > Changes in v3: > - fix minor comments > --- > xen/arch/arm/include/asm/pci.h | 2 ++ > xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++ > xen/arch/x86/include/asm/pci.h | 10 +++++++ > xen/drivers/passthrough/pci.c | 8 +++--- > 4 files changed, 59 insertions(+), 4 deletions(-) > > 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..0eb121666d 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 > +{ > + mfn_t start; > + mfn_t end; > + bool is_valid; > +}; > + > /* > * List for all the pci host bridges. > */ > @@ -363,6 +373,39 @@ 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 *bar_data = data; > + unsigned long s = mfn_x(bar_data->start); > + unsigned long e = mfn_x(bar_data->end); > + > + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) > + bar_data->is_valid = true; > + > + return 0; > +} > + > +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end) > +{ > + int ret; > + const struct dt_device_node *dt_node; > + struct pdev_bar bar_data = { > + .start = start, > + .end = end, > + .is_valid = 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));
On Thu, 1 Sep 2022, Rahul Singh wrote: > 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 v3: > - fix minor comments > --- > xen/arch/arm/include/asm/pci.h | 2 ++ > xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++ > xen/arch/x86/include/asm/pci.h | 10 +++++++ > xen/drivers/passthrough/pci.c | 8 +++--- > 4 files changed, 59 insertions(+), 4 deletions(-) > > 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..0eb121666d 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 > +{ > + mfn_t start; > + mfn_t end; > + bool is_valid; > +}; > + > /* > * List for all the pci host bridges. > */ > @@ -363,6 +373,39 @@ 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 *bar_data = data; > + unsigned long s = mfn_x(bar_data->start); > + unsigned long e = mfn_x(bar_data->end); > + > + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) > + bar_data->is_valid = true; This patch looks good and you addressed all Jan's comment well. Before I ack it, one question. I know that you made this change to address Jan's comment but using PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <= PFN_UP(addr + len - 1)) check means that we are relaxing the requirements, aren't we? I know that this discussion is a bit pointless because addr and len should always be page aligned, and if they weren't it would be a mistake. But assuming that they are not page aligned, wouldn't we want this check to be a strict as possible? Wouldn't we want to ensure that the [s,e] range is a strict subset of [addr,addr+len-1] ? If so we would need to do the following instead: if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) ) bar_data->is_valid = true;
Hi Rahul, On 01/09/2022 10:29, 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 v3: > - fix minor comments > --- > xen/arch/arm/include/asm/pci.h | 2 ++ > xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++ > xen/arch/x86/include/asm/pci.h | 10 +++++++ > xen/drivers/passthrough/pci.c | 8 +++--- > 4 files changed, 59 insertions(+), 4 deletions(-) > > 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..0eb121666d 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. > + */ I find this comment a bit misleading. What you are storing is a candidate region. IOW, this may or may not be a PCI device bar. Given the current use below, I would rename the structure to something more specific like: pdev_bar_check. > +struct pdev_bar > +{ > + mfn_t start; > + mfn_t end; > + bool is_valid; > +}; > + > /* > * List for all the pci host bridges. > */ > @@ -363,6 +373,39 @@ 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 *bar_data = data; > + unsigned long s = mfn_x(bar_data->start); > + unsigned long e = mfn_x(bar_data->end); > + > + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) AFAICT 's' and 'e' are provided by pci_check_bar() and will never change. So can we move the check 's <= e' outside of the callback? > + bar_data->is_valid = true; > + > + return 0; > +} > + > +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end) > +{ Other than the current calls in check_pdev(), do you have plan to use it in more places? The reason I am asking it is this function is non-trivial on Arm (dt_for_each_range() is quite complex). Cheers,
On 03.09.2022 02:24, Stefano Stabellini wrote: > On Thu, 1 Sep 2022, Rahul Singh wrote: >> @@ -363,6 +373,39 @@ 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 *bar_data = data; >> + unsigned long s = mfn_x(bar_data->start); >> + unsigned long e = mfn_x(bar_data->end); >> + >> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) >> + bar_data->is_valid = true; > > > This patch looks good and you addressed all Jan's comment well. Before I > ack it, one question. > > I know that you made this change to address Jan's comment but using > PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <= > PFN_UP(addr + len - 1)) check means that we are relaxing the > requirements, aren't we? > > I know that this discussion is a bit pointless because addr and len should > always be page aligned, and if they weren't it would be a mistake. But > assuming that they are not page aligned, wouldn't we want this check to > be a strict as possible? > > Wouldn't we want to ensure that the [s,e] range is a strict subset of > [addr,addr+len-1] ? If so we would need to do the following instead: > > if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) ) > bar_data->is_valid = true; But that might mean (in theory at least) a partial overlap, which has to be avoided. The only alternative that I see to Rahul's original code is to omit use of PFN_DOWN() and PFN_UP() in this construct altogether. Assuming that's correct for the passed in (addr,len) tuple. Jan
On 01.09.2022 11:29, 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> Acked-by: Jan Beulich <jbeulich@suse.com>
Hi Julien, > On 3 Sep 2022, at 8:18 am, Julien Grall <julien@xen.org> wrote: > > Hi Rahul, > > On 01/09/2022 10:29, 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 v3: >> - fix minor comments >> --- >> xen/arch/arm/include/asm/pci.h | 2 ++ >> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++ >> xen/arch/x86/include/asm/pci.h | 10 +++++++ >> xen/drivers/passthrough/pci.c | 8 +++--- >> 4 files changed, 59 insertions(+), 4 deletions(-) >> 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..0eb121666d 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. >> + */ > > I find this comment a bit misleading. What you are storing is a > candidate region. IOW, this may or may not be a PCI device bar. > > Given the current use below, I would rename the structure to something more specific like: pdev_bar_check. Ack. > >> +struct pdev_bar >> +{ >> + mfn_t start; >> + mfn_t end; >> + bool is_valid; >> +}; >> + >> /* >> * List for all the pci host bridges. >> */ >> @@ -363,6 +373,39 @@ 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 *bar_data = data; >> + unsigned long s = mfn_x(bar_data->start); >> + unsigned long e = mfn_x(bar_data->end); >> + >> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) > > AFAICT 's' and 'e' are provided by pci_check_bar() and will never change. So can we move the check 's <= e' outside of the callback? Yes, We can move the check outside the callback but I feel that if we check here then it is more readable that we are checking for all possible values in one statement. Let me know your view on this. > >> + bar_data->is_valid = true; >> + >> + return 0; >> +} >> + >> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end) >> +{ > > Other than the current calls in check_pdev(), do you have plan to use it in more places? The reason I am asking it is this function is non-trivial on Arm (dt_for_each_range() is quite complex). I don’t see any use of this function in more places. As this function will be called during dom0 boot when the PCI devices are added I don’t see any performance issues. We may need to revisit this function when we add ACPI PCI passthrough support. I will add TODO that we need to revisit this function for ACPI PCI passthrough support. Regards, Rahul
On 06/09/2022 10:39, Rahul Singh wrote: > Hi Julien, > >> On 3 Sep 2022, at 8:18 am, Julien Grall <julien@xen.org> wrote: >> >> Hi Rahul, >> >> On 01/09/2022 10:29, 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 v3: >>> - fix minor comments >>> --- >>> xen/arch/arm/include/asm/pci.h | 2 ++ >>> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++ >>> xen/arch/x86/include/asm/pci.h | 10 +++++++ >>> xen/drivers/passthrough/pci.c | 8 +++--- >>> 4 files changed, 59 insertions(+), 4 deletions(-) >>> 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..0eb121666d 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. >>> + */ >> >> I find this comment a bit misleading. What you are storing is a >> candidate region. IOW, this may or may not be a PCI device bar. >> >> Given the current use below, I would rename the structure to something more specific like: pdev_bar_check. > > Ack. >> >>> +struct pdev_bar >>> +{ >>> + mfn_t start; >>> + mfn_t end; >>> + bool is_valid; >>> +}; >>> + >>> /* >>> * List for all the pci host bridges. >>> */ >>> @@ -363,6 +373,39 @@ 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 *bar_data = data; >>> + unsigned long s = mfn_x(bar_data->start); >>> + unsigned long e = mfn_x(bar_data->end); >>> + >>> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) >> >> AFAICT 's' and 'e' are provided by pci_check_bar() and will never change. So can we move the check 's <= e' outside of the callback? > > Yes, We can move the check outside the callback but I feel that if we check here then it is more > readable that we are checking for all possible values in one statement. Let me know your view on this. The readability is really a matter of taste here. But my point is more on the number of time a check is done. It seems pointless to do the same check N times when you know the values are not going to change. Admittedly, the operation is fast (this is a comparison) and N should be small (?). However, I think it raises the question on where do you draw the line? Personally, I think all invariant should be checked outside of callbacks. So the line is very clear. > >> >>> + bar_data->is_valid = true; >>> + >>> + return 0; >>> +} >>> + >>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end) >>> +{ >> >> Other than the current calls in check_pdev(), do you have plan to use it in more places? The reason I am asking it is this function is non-trivial on Arm (dt_for_each_range() is quite complex). > > I don’t see any use of this function in more places. As this function will be called during dom0 boot when the PCI devices are > added I don’t see any performance issues. We may need to revisit this function when we add ACPI PCI passthrough support. > I will add TODO that we need to revisit this function for ACPI PCI passthrough support. Thanks. Cheers,
Hi Julien, > On 6 Sep 2022, at 10:53 am, Julien Grall <julien@xen.org> wrote: > > > > On 06/09/2022 10:39, Rahul Singh wrote: >> Hi Julien, >>> On 3 Sep 2022, at 8:18 am, Julien Grall <julien@xen.org> wrote: >>> >>> Hi Rahul, >>> >>> On 01/09/2022 10:29, 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 v3: >>>> - fix minor comments >>>> --- >>>> xen/arch/arm/include/asm/pci.h | 2 ++ >>>> xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++ >>>> xen/arch/x86/include/asm/pci.h | 10 +++++++ >>>> xen/drivers/passthrough/pci.c | 8 +++--- >>>> 4 files changed, 59 insertions(+), 4 deletions(-) >>>> 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..0eb121666d 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. >>>> + */ >>> >>> I find this comment a bit misleading. What you are storing is a >>> candidate region. IOW, this may or may not be a PCI device bar. >>> >>> Given the current use below, I would rename the structure to something more specific like: pdev_bar_check. >> Ack. >>> >>>> +struct pdev_bar >>>> +{ >>>> + mfn_t start; >>>> + mfn_t end; >>>> + bool is_valid; >>>> +}; >>>> + >>>> /* >>>> * List for all the pci host bridges. >>>> */ >>>> @@ -363,6 +373,39 @@ 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 *bar_data = data; >>>> + unsigned long s = mfn_x(bar_data->start); >>>> + unsigned long e = mfn_x(bar_data->end); >>>> + >>>> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) >>> >>> AFAICT 's' and 'e' are provided by pci_check_bar() and will never change. So can we move the check 's <= e' outside of the callback? >> Yes, We can move the check outside the callback but I feel that if we check here then it is more >> readable that we are checking for all possible values in one statement. Let me know your view on this. > The readability is really a matter of taste here. But my point is more on the number of time a check is done. > > It seems pointless to do the same check N times when you know the values are not going to change. Admittedly, the operation is fast (this is a comparison) and N should be small (?). > > However, I think it raises the question on where do you draw the line? > > Personally, I think all invariant should be checked outside of callbacks. So the line is very clear. > I will move the check for "s <=e” outside the callback and will send it for review. Regards, Rahul
Hi Jan & Stefano, On 06/09/2022 09:53, Jan Beulich wrote: > On 03.09.2022 02:24, Stefano Stabellini wrote: >> On Thu, 1 Sep 2022, Rahul Singh wrote: >>> @@ -363,6 +373,39 @@ 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 *bar_data = data; >>> + unsigned long s = mfn_x(bar_data->start); >>> + unsigned long e = mfn_x(bar_data->end); >>> + >>> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) >>> + bar_data->is_valid = true; >> >> >> This patch looks good and you addressed all Jan's comment well. Before I >> ack it, one question. >> >> I know that you made this change to address Jan's comment but using >> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <= >> PFN_UP(addr + len - 1)) check means that we are relaxing the >> requirements, aren't we? >> >> I know that this discussion is a bit pointless because addr and len should >> always be page aligned, and if they weren't it would be a mistake. Hmmm.... Is that requirement written down somewhere? The reason I am asking is "page-aligned" will vary depending on the software. In the past we had a couple of cases where the region would be 4KB-aligned but not necessarily 64KB-aligned. If the answer is no to my question then... > But >> assuming that they are not page aligned, wouldn't we want this check to >> be a strict as possible? >> >> Wouldn't we want to ensure that the [s,e] range is a strict subset of >> [addr,addr+len-1] ? If so we would need to do the following instead: >> >> if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) ) >> bar_data->is_valid = true; > > But that might mean (in theory at least) a partial overlap, which has > to be avoided. The only alternative that I see to Rahul's original > code is to omit use of PFN_DOWN() and PFN_UP() in this construct > altogether. Assuming that's correct for the passed in (addr,len) > tuple. ... I think we would want to drop PFN_DOWN/PFN_UP here. Cheers,
On 07.09.2022 10:58, Julien Grall wrote: > Hi Jan & Stefano, > > On 06/09/2022 09:53, Jan Beulich wrote: >> On 03.09.2022 02:24, Stefano Stabellini wrote: >>> On Thu, 1 Sep 2022, Rahul Singh wrote: >>>> @@ -363,6 +373,39 @@ 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 *bar_data = data; >>>> + unsigned long s = mfn_x(bar_data->start); >>>> + unsigned long e = mfn_x(bar_data->end); >>>> + >>>> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) >>>> + bar_data->is_valid = true; >>> >>> >>> This patch looks good and you addressed all Jan's comment well. Before I >>> ack it, one question. >>> >>> I know that you made this change to address Jan's comment but using >>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <= >>> PFN_UP(addr + len - 1)) check means that we are relaxing the >>> requirements, aren't we? >>> >>> I know that this discussion is a bit pointless because addr and len should >>> always be page aligned, and if they weren't it would be a mistake. > > Hmmm.... Is that requirement written down somewhere? What do you mean here? Isn't it quite obvious that every byte in the address space may only be used for a single purpose? I.e. if a byte is covered by a BAR, it cannot also be covered by a RAM region or yet something else (e.g. MMIO beyond BARs of PCI devices). What happens if BAR and RAM indeed overlap depends on fabric and chipset, but it'll either result in chaos if two parties respond to a single request on the bus, or it'll be (hopefully) deterministic (for any individual system) which of the two takes "precedence". I think we've had a similar discussion a little while ago already in the context of vPCI with guest address space in mind. The same (imo obvious) "rule" spelled out above applies there and here. Jan > The reason I am > asking is "page-aligned" will vary depending on the software. In the > past we had a couple of cases where the region would be 4KB-aligned but > not necessarily 64KB-aligned. > > If the answer is no to my question then... > >> But >>> assuming that they are not page aligned, wouldn't we want this check to >>> be a strict as possible? >>> >>> Wouldn't we want to ensure that the [s,e] range is a strict subset of >>> [addr,addr+len-1] ? If so we would need to do the following instead: >>> >>> if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) ) >>> bar_data->is_valid = true; >> >> But that might mean (in theory at least) a partial overlap, which has >> to be avoided. The only alternative that I see to Rahul's original >> code is to omit use of PFN_DOWN() and PFN_UP() in this construct >> altogether. Assuming that's correct for the passed in (addr,len) >> tuple. > > ... I think we would want to drop PFN_DOWN/PFN_UP here. > > Cheers, >
Hi Jan, On 07/09/2022 10:07, Jan Beulich wrote: > On 07.09.2022 10:58, Julien Grall wrote: >> Hi Jan & Stefano, >> >> On 06/09/2022 09:53, Jan Beulich wrote: >>> On 03.09.2022 02:24, Stefano Stabellini wrote: >>>> On Thu, 1 Sep 2022, Rahul Singh wrote: >>>>> @@ -363,6 +373,39 @@ 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 *bar_data = data; >>>>> + unsigned long s = mfn_x(bar_data->start); >>>>> + unsigned long e = mfn_x(bar_data->end); >>>>> + >>>>> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) >>>>> + bar_data->is_valid = true; >>>> >>>> >>>> This patch looks good and you addressed all Jan's comment well. Before I >>>> ack it, one question. >>>> >>>> I know that you made this change to address Jan's comment but using >>>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <= >>>> PFN_UP(addr + len - 1)) check means that we are relaxing the >>>> requirements, aren't we? >>>> >>>> I know that this discussion is a bit pointless because addr and len should >>>> always be page aligned, and if they weren't it would be a mistake. >> >> Hmmm.... Is that requirement written down somewhere? > > What do you mean here? Isn't it quite obvious that every byte in the > address space may only be used for a single purpose? I.e. if a byte > is covered by a BAR, it cannot also be covered by a RAM region or > yet something else (e.g. MMIO beyond BARs of PCI devices). What > happens if BAR and RAM indeed overlap depends on fabric and chipset, > but it'll either result in chaos if two parties respond to a single > request on the bus, or it'll be (hopefully) deterministic (for any > individual system) which of the two takes "precedence". I am well aware about that and I am not sure how you implied this is what I was referring to from what I wrote (in particular if you read the next sentence). Stefano wrote that it would be a mistake if the address/length is not page-aligned. However, I am not aware from such requirement written down. It seems to be more an expected common sense that was IIRC not always respected on HW supporting multiple page-granularity. Cheers,
On 07.09.2022 12:00, Julien Grall wrote: > On 07/09/2022 10:07, Jan Beulich wrote: >> On 07.09.2022 10:58, Julien Grall wrote: >>> On 06/09/2022 09:53, Jan Beulich wrote: >>>> On 03.09.2022 02:24, Stefano Stabellini wrote: >>>>> On Thu, 1 Sep 2022, Rahul Singh wrote: >>>>>> @@ -363,6 +373,39 @@ 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 *bar_data = data; >>>>>> + unsigned long s = mfn_x(bar_data->start); >>>>>> + unsigned long e = mfn_x(bar_data->end); >>>>>> + >>>>>> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) >>>>>> + bar_data->is_valid = true; >>>>> >>>>> >>>>> This patch looks good and you addressed all Jan's comment well. Before I >>>>> ack it, one question. >>>>> >>>>> I know that you made this change to address Jan's comment but using >>>>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <= >>>>> PFN_UP(addr + len - 1)) check means that we are relaxing the >>>>> requirements, aren't we? >>>>> >>>>> I know that this discussion is a bit pointless because addr and len should >>>>> always be page aligned, and if they weren't it would be a mistake. >>> >>> Hmmm.... Is that requirement written down somewhere? >> >> What do you mean here? Isn't it quite obvious that every byte in the >> address space may only be used for a single purpose? I.e. if a byte >> is covered by a BAR, it cannot also be covered by a RAM region or >> yet something else (e.g. MMIO beyond BARs of PCI devices). What >> happens if BAR and RAM indeed overlap depends on fabric and chipset, >> but it'll either result in chaos if two parties respond to a single >> request on the bus, or it'll be (hopefully) deterministic (for any >> individual system) which of the two takes "precedence". > > I am well aware about that and I am not sure how you implied this is > what I was referring to from what I wrote (in particular if you read the > next sentence). > > Stefano wrote that it would be a mistake if the address/length is not > page-aligned. However, I am not aware from such requirement written > down. It seems to be more an expected common sense that was IIRC not > always respected on HW supporting multiple page-granularity. I guess the question was then solely directed at Stefano? I have no idea, after all, what guarantees there are for addr and len, which presumably originate from DT somewhere. Yet if there aren't any guarantees, then aligning the incoming range may be necessary: BARs can be sub-page size (even more so when considering page sizes above 4k), and so a not-really-overlap (at byte granularity) could still be an issue if there's an overlap at 4k or system page size granularity. Jan
On 07/09/2022 13:01, Jan Beulich wrote: > On 07.09.2022 12:00, Julien Grall wrote: >> On 07/09/2022 10:07, Jan Beulich wrote: >>> On 07.09.2022 10:58, Julien Grall wrote: >>>> On 06/09/2022 09:53, Jan Beulich wrote: >>>>> On 03.09.2022 02:24, Stefano Stabellini wrote: >>>>>> On Thu, 1 Sep 2022, Rahul Singh wrote: >>>>>>> @@ -363,6 +373,39 @@ 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 *bar_data = data; >>>>>>> + unsigned long s = mfn_x(bar_data->start); >>>>>>> + unsigned long e = mfn_x(bar_data->end); >>>>>>> + >>>>>>> + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) >>>>>>> + bar_data->is_valid = true; >>>>>> >>>>>> >>>>>> This patch looks good and you addressed all Jan's comment well. Before I >>>>>> ack it, one question. >>>>>> >>>>>> I know that you made this change to address Jan's comment but using >>>>>> PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <= >>>>>> PFN_UP(addr + len - 1)) check means that we are relaxing the >>>>>> requirements, aren't we? >>>>>> >>>>>> I know that this discussion is a bit pointless because addr and len should >>>>>> always be page aligned, and if they weren't it would be a mistake. >>>> >>>> Hmmm.... Is that requirement written down somewhere? >>> >>> What do you mean here? Isn't it quite obvious that every byte in the >>> address space may only be used for a single purpose? I.e. if a byte >>> is covered by a BAR, it cannot also be covered by a RAM region or >>> yet something else (e.g. MMIO beyond BARs of PCI devices). What >>> happens if BAR and RAM indeed overlap depends on fabric and chipset, >>> but it'll either result in chaos if two parties respond to a single >>> request on the bus, or it'll be (hopefully) deterministic (for any >>> individual system) which of the two takes "precedence". >> >> I am well aware about that and I am not sure how you implied this is >> what I was referring to from what I wrote (in particular if you read the >> next sentence). >> >> Stefano wrote that it would be a mistake if the address/length is not >> page-aligned. However, I am not aware from such requirement written >> down. It seems to be more an expected common sense that was IIRC not >> always respected on HW supporting multiple page-granularity. > > I guess the question was then solely directed at Stefano? This question yes. The rest was a reply to your suggestion. I will wait for Stefano to answer. Cheers,
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..0eb121666d 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 +{ + mfn_t start; + mfn_t end; + bool is_valid; +}; + /* * List for all the pci host bridges. */ @@ -363,6 +373,39 @@ 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 *bar_data = data; + unsigned long s = mfn_x(bar_data->start); + unsigned long e = mfn_x(bar_data->end); + + if ( (s <= e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) ) + bar_data->is_valid = true; + + return 0; +} + +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end) +{ + int ret; + const struct dt_device_node *dt_node; + struct pdev_bar bar_data = { + .start = start, + .end = end, + .is_valid = 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 v3: - fix minor comments --- xen/arch/arm/include/asm/pci.h | 2 ++ xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++ xen/arch/x86/include/asm/pci.h | 10 +++++++ xen/drivers/passthrough/pci.c | 8 +++--- 4 files changed, 59 insertions(+), 4 deletions(-)