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

[PATCH v5 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 | 58 ++++++++++++++++++++++++++++--
 xen/arch/x86/include/asm/pci.h     | 10 ++++++
 xen/drivers/passthrough/pci.c      |  8 ++---
 4 files changed, 74 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH v5 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>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Changes in v5:
 - no changes
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 v5 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>
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.1

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

[Jan Beulich]

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

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

[Rahul Singh]

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

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

[Julien Grall]

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

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

[Stefano Stabellini]

On 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
> 

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

[Julien Grall]

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

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

[Bertrand Marquis]

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