[PATCH v4 0/2] xen/pci: implement is_memory_hole for ARM

[PATCH v4 0/2] xen/pci: implement is_memory_hole for ARM

[Rahul Singh]

This patch series is to implement something like is_memory_hole function for
ARM.

Rahul Singh (2):
  xen/arm: pci: modify pci_find_host_bridge_node argument to const pdev
  xen/pci: replace call to is_memory_hole to pci_check_bar

 xen/arch/arm/include/asm/pci.h     |  5 ++-
 xen/arch/arm/pci/pci-host-common.c | 53 ++++++++++++++++++++++++++++--
 xen/arch/x86/include/asm/pci.h     | 10 ++++++
 xen/drivers/passthrough/pci.c      |  8 ++---
 4 files changed, 69 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v4 1/2] xen/arm: pci: modify pci_find_host_bridge_node argument to const pdev

[Rahul Singh]

Modify pci_find_host_bridge_node argument to const pdev to avoid
converting the dev to pdev in pci_find_host_bridge_node and also
constify the return.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes in v4:
 - no changes
Changes in v3:
 - no changes
---
 xen/arch/arm/include/asm/pci.h     | 3 ++-
 xen/arch/arm/pci/pci-host-common.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7c7449d64f..80a2431804 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -106,7 +106,8 @@ bool pci_ecam_need_p2m_hwdom_mapping(struct domain *d,
                                      struct pci_host_bridge *bridge,
                                      uint64_t addr);
 struct pci_host_bridge *pci_find_host_bridge(uint16_t segment, uint8_t bus);
-struct dt_device_node *pci_find_host_bridge_node(struct device *dev);
+const struct dt_device_node *
+pci_find_host_bridge_node(const struct pci_dev *pdev);
 int pci_get_host_bridge_segment(const struct dt_device_node *node,
                                 uint16_t *segment);
 
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index fd8c0f837a..89ef30028e 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -243,10 +243,10 @@ err_exit:
 /*
  * Get host bridge node given a device attached to it.
  */
-struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
+const struct dt_device_node *
+pci_find_host_bridge_node(const struct pci_dev *pdev)
 {
     struct pci_host_bridge *bridge;
-    struct pci_dev *pdev = dev_to_pci(dev);
 
     bridge = pci_find_host_bridge(pdev->seg, pdev->bus);
     if ( unlikely(!bridge) )
-- 
2.25.1

[PATCH v4 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Rahul Singh]

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(-)

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));
-- 
2.25.1

Re: [PATCH v4 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Jan Beulich]

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

Re: [PATCH v4 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Julien Grall]

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,

-- 
Julien Grall

Re: [PATCH v4 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Bertrand Marquis]

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

Re: [PATCH v4 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Stefano Stabellini]

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. 

Re: [PATCH v4 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Bertrand Marquis]

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

Re: [PATCH v4 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Jan Beulich]

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

Re: [PATCH v4 2/2] xen/pci: replace call to is_memory_hole to pci_check_bar

[Bertrand Marquis]

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