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 v2:
- fixed minor comment
- modify pci_find_host_bridge_node argument to const pdev
---
xen/arch/arm/include/asm/pci.h | 2 ++
xen/arch/arm/pci/pci-host-common.c | 46 ++++++++++++++++++++++++++++++
xen/arch/x86/include/asm/pci.h | 10 +++++++
xen/drivers/passthrough/pci.c | 8 +++---
4 files changed, 62 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..04f9d8580b 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,42 @@ int __init pci_host_bridge_mappings(struct domain *d)
return 0;
}
+static int is_bar_valid(const struct dt_device_node *dev,
+ u64 addr, u64 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_UP(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;
+
+ if ( !bar_data.is_valid )
+ return false;
+
+ return true;
+}
/*
* 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 17.08.2022 16:45, Rahul Singh wrote:
> @@ -363,6 +373,42 @@ int __init pci_host_bridge_mappings(struct domain *d)
> return 0;
> }
>
> +static int is_bar_valid(const struct dt_device_node *dev,
> + u64 addr, u64 len, void *data)
s/u64/uint64_t/g please.
> +{
> + 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_UP(addr)) && (e <= PFN_UP(addr + len - 1)) )
Doesn't this need to be s >= PFN_DOWN(addr)? Or else why is e checked
against PFN_UP()?
> + 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);
Just as a side note - I find such (the first instance here) uses of the
unary & operator odd. The same effect will be had without it. So all it
does (in my opinion) is make things harder to read (just very slightly,
of course).
> + if ( ret < 0 )
> + return false;
> +
> + if ( !bar_data.is_valid )
> + return false;
> +
> + return true;
Simply "return bar_data.is_valid;"?
Jan
Hello Jan,
> On 17 Aug 2022, at 4:18 pm, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 17.08.2022 16:45, Rahul Singh wrote:
>> @@ -363,6 +373,42 @@ int __init pci_host_bridge_mappings(struct domain *d)
>> return 0;
>> }
>>
>> +static int is_bar_valid(const struct dt_device_node *dev,
>> + u64 addr, u64 len, void *data)
>
> s/u64/uint64_t/g please.
Ack.
>
>> +{
>> + 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_UP(addr)) && (e <= PFN_UP(addr + len - 1)) )
>
> Doesn't this need to be s >= PFN_DOWN(addr)? Or else why is e checked
> against PFN_UP()?
Ack. I will modify as 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);
>
> Just as a side note - I find such (the first instance here) uses of the
> unary & operator odd. The same effect will be had without it. So all it
> does (in my opinion) is make things harder to read (just very slightly,
> of course).
I follow the same calling method as used in map_device_children() function in
file arch/arm/domain_build.c
>
>> + if ( ret < 0 )
>> + return false;
>> +
>> + if ( !bar_data.is_valid )
>> + return false;
>> +
>> + return true;
>
> Simply "return bar_data.is_valid;"?
Ack.
Regards,
Rahul
On 18.08.2022 16:58, Rahul Singh wrote:
>> On 17 Aug 2022, at 4:18 pm, Jan Beulich <jbeulich@suse.com> wrote:
>> On 17.08.2022 16:45, Rahul Singh wrote:
>>> @@ -363,6 +373,42 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>> return 0;
>>> }
>>>
>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>> + u64 addr, u64 len, void *data)
>>
>> s/u64/uint64_t/g please.
>
> Ack.
>>
>>> +{
>>> + struct pdev_bar *bar_data = data;
const?
>>> + unsigned long s = mfn_x(bar_data->start);
>>> + unsigned long e = mfn_x(bar_data->end);
>>> +
>>> + if ( (s < e) && (s >= PFN_UP(addr)) && (e <= PFN_UP(addr + len - 1)) )
>>
>> Doesn't this need to be s >= PFN_DOWN(addr)? Or else why is e checked
>> against PFN_UP()?
>
> Ack. I will modify as if ( (s < e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) )
Hmm, doesn't it further need to be s <= e, seeing that the range passed
to pci_check_bar() is an inclusive one?
Jan
Hi Jan,
> On 19 Aug 2022, at 7:07 am, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 18.08.2022 16:58, Rahul Singh wrote:
>>> On 17 Aug 2022, at 4:18 pm, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 17.08.2022 16:45, Rahul Singh wrote:
>>>> @@ -363,6 +373,42 @@ int __init pci_host_bridge_mappings(struct domain *d)
>>>> return 0;
>>>> }
>>>>
>>>> +static int is_bar_valid(const struct dt_device_node *dev,
>>>> + u64 addr, u64 len, void *data)
>>>
>>> s/u64/uint64_t/g please.
>>
>> Ack.
>>>
>>>> +{
>>>> + struct pdev_bar *bar_data = data;
>
> const?
Ack.
>
>>>> + unsigned long s = mfn_x(bar_data->start);
>>>> + unsigned long e = mfn_x(bar_data->end);
>>>> +
>>>> + if ( (s < e) && (s >= PFN_UP(addr)) && (e <= PFN_UP(addr + len - 1)) )
>>>
>>> Doesn't this need to be s >= PFN_DOWN(addr)? Or else why is e checked
>>> against PFN_UP()?
>>
>> Ack. I will modify as if ( (s < e) && (s >= PFN_DOWN(addr)) && (e <= PFN_UP(addr + len - 1)) )
>
> Hmm, doesn't it further need to be s <= e, seeing that the range passed
> to pci_check_bar() is an inclusive one?
Agree, I will do the modification in next version.
Regards,
Rahul
© 2016 - 2026 Red Hat, Inc.