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> # x86, common
---
Changes in v5:
- drop use of PFN_UP and PF_DOWN in case addresses are not aligned.
- As we drop the PFN_UP and PFN_DOWN we need to use the mfn_to_maddr()
to get the BAR address without page shift.
- Add TODO comment for address alignment check for ranges.
- Added Jan Acked-by for x86 and common code.
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 | 54 ++++++++++++++++++++++++++++++
xen/arch/x86/include/asm/pci.h | 10 ++++++
xen/drivers/passthrough/pci.c | 8 ++---
4 files changed, 70 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..d51cfdf352 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,50 @@ int __init pci_host_bridge_mappings(struct domain *d)
return 0;
}
+/*
+ * TODO: BAR addresses and Root Complex window addresses are not guaranteed
+ * to be page aligned. We should check for alignment but this is not the
+ * right place for alignment check.
+ */
+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 >= addr) && (e <= (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_to_maddr(start);
+ unsigned long e = mfn_to_maddr(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));
--
2.25.1On Thu, 8 Sep 2022, 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> # x86, common
> ---
> Changes in v5:
> - drop use of PFN_UP and PF_DOWN in case addresses are not aligned.
> - As we drop the PFN_UP and PFN_DOWN we need to use the mfn_to_maddr()
> to get the BAR address without page shift.
> - Add TODO comment for address alignment check for ranges.
> - Added Jan Acked-by for x86 and common code.
> 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 | 54 ++++++++++++++++++++++++++++++
> xen/arch/x86/include/asm/pci.h | 10 ++++++
> xen/drivers/passthrough/pci.c | 8 ++---
> 4 files changed, 70 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..d51cfdf352 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,50 @@ int __init pci_host_bridge_mappings(struct domain *d)
> return 0;
> }
>
> +/*
> + * TODO: BAR addresses and Root Complex window addresses are not guaranteed
> + * to be page aligned. We should check for alignment but this is not the
> + * right place for alignment check.
> + */
> +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 >= addr) && (e <= (addr + len - 1)) )
> + bar_data->is_valid = true;
"s" and "e" are "unsigned long" while "addr" and "len" are uint64_t. Is
that OK?
Specifically, considering a potential arm32 case, shouldn't "s" and "e"
be uint64_t as well? Which means pdev_bar_check.start and end should be
uint64_t?
> + 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_to_maddr(start);
> + unsigned long e = mfn_to_maddr(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));
> --
> 2.25.1
>
Hi Stefano,
On 08/09/2022 21:59, Stefano Stabellini wrote:
>> +/*
>> + * TODO: BAR addresses and Root Complex window addresses are not guaranteed
>> + * to be page aligned. We should check for alignment but this is not the
>> + * right place for alignment check.
>> + */
>> +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 >= addr) && (e <= (addr + len - 1)) )
>> + bar_data->is_valid = true;
>
> "s" and "e" are "unsigned long" while "addr" and "len" are uint64_t. Is
> that OK?
Good catch. No, physical address on Arm32 can be up to 40 bits.
>
> Specifically, considering a potential arm32 case, shouldn't "s" and "e"
> be uint64_t as well? Which means pdev_bar_check.start and end should be
> uint64_t?
They should be paddr_t which will be 64-bit on both arm32 and arm64.
Cheers,
--
Julien Grall
Hi,
> On 8 Sep 2022, at 22:41, Julien Grall <julien@xen.org> wrote:
>
> Hi Stefano,
>
> On 08/09/2022 21:59, Stefano Stabellini wrote:
>>> +/*
>>> + * TODO: BAR addresses and Root Complex window addresses are not guaranteed
>>> + * to be page aligned. We should check for alignment but this is not the
>>> + * right place for alignment check.
>>> + */
>>> +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 >= addr) && (e <= (addr + len - 1)) )
>>> + bar_data->is_valid = true;
>> "s" and "e" are "unsigned long" while "addr" and "len" are uint64_t. Is
>> that OK?
>
> Good catch. No, physical address on Arm32 can be up to 40 bits.
>
>> Specifically, considering a potential arm32 case, shouldn't "s" and "e"
>> be uint64_t as well? Which means pdev_bar_check.start and end should be
>> uint64_t?
>
> They should be paddr_t which will be 64-bit on both arm32 and arm64.
paddr_t sounds a lot better here.
@Rahul: Can you send a v6 fixing this ?
Thanks
Bertrand
>
> Cheers,
>
> --
> Julien Grall
On 08.09.2022 13:49, 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> # x86, common FTAOD: I object to this tagging, and I did not provide the ack with such tags. Quoting docs/process/sending-patches.pandoc: "The `Acked-by:` tag can only be given by a **maintainer** of the modified code, and it only covers the code the maintainer is responsible for." The doc provides for tagging here, yes, but such should only be used for the unusual case of an ack restricted to less than what a person's maintainership covers. Otherwise we'd end up seeing overly many tagged acks. (Recall that tagged R-b is also expected to be the exception, not the common case.) Jan
Hi Jan, On 08/09/2022 13:03, Jan Beulich wrote: > On 08.09.2022 13:49, 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> # x86, common > > FTAOD: I object to this tagging, and I did not provide the ack with > such tags. Quoting docs/process/sending-patches.pandoc: "The > `Acked-by:` tag can only be given by a **maintainer** of the modified > code, and it only covers the code the maintainer is responsible for." You are part of the REST. So technically, one could assume that your acked-by would also cover the Arm. I agree this would be uncommon but this has been used before when the more specialized maintainer was unresponsive. At least with # x86, common it is clear what you are happy with. > The doc provides for tagging here, yes, but such should only be used > for the unusual case of an ack restricted to less than what a > person's maintainership covers. Otherwise we'd end up seeing overly > many tagged acks. I appreciate you may not like them but it helps while committing because you can cross-check easily whether all the files have been covered with the tags. Cheers, -- Julien Grall
Hi Jan, > On 8 Sep 2022, at 1:03 pm, Jan Beulich <jbeulich@suse.com> wrote: > > On 08.09.2022 13:49, 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> # x86, common > > FTAOD: I object to this tagging, and I did not provide the ack with > such tags. Quoting docs/process/sending-patches.pandoc: "The > `Acked-by:` tag can only be given by a **maintainer** of the modified > code, and it only covers the code the maintainer is responsible for." > The doc provides for tagging here, yes, but such should only be used > for the unusual case of an ack restricted to less than what a > person's maintainership covers. Otherwise we'd end up seeing overly > many tagged acks. (Recall that tagged R-b is also expected to be the > exception, not the common case.) > Ok. I will remove “# x86, common” if I get comments on this patch and there is a need for the the next version, otherwise, I suggest that the committer can remove this while committing the patch. Regards, Rahul
© 2016 - 2026 Red Hat, Inc.