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

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

-- 
2.25.1

[PATCH v2 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>
---
Changes in v2:
 - this patch is introduced in this version
---
 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 v2 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 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.1

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

[Jan Beulich]

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.

> +{
> +    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

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

[Rahul Singh]

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

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

[Jan Beulich]

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

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

[Rahul Singh]

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

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

[Oleksandr]

On 17.08.22 17:45, Rahul Singh wrote:


Hello Rahul

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


> ---
> Changes in v2:
>   - this patch is introduced in this version
> ---
>   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) )

-- 
Regards,

Oleksandr Tyshchenko

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

[Stefano Stabellini]

On Fri, 19 Aug 2022, Oleksandr wrote:
> On 17.08.22 17:45, Rahul Singh wrote:
> Hello Rahul
> 
> > 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 v2:
> >   - this patch is introduced in this version
> > ---
> >   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) )
> 
> -- 
> Regards,
> 
> Oleksandr Tyshchenko
>