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

Rahul Singh posted 1 patch 1 year, 7 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/d5590d91683f2dddb3836b1afb444f30c2f5a7fb.1659713855.git.rahul.singh@arm.com
xen/arch/arm/include/asm/pci.h     | 12 ++++++++++
xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
xen/arch/x86/include/asm/pci.h     | 10 +++++++++
xen/drivers/passthrough/pci.c      |  8 +++----
4 files changed, 61 insertions(+), 4 deletions(-)
[PATCH] xen/pci: replace call to is_memory_hole to pci_check_bar
Posted by Rahul Singh 1 year, 7 months ago
From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

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>
---
 xen/arch/arm/include/asm/pci.h     | 12 ++++++++++
 xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/pci.h     | 10 +++++++++
 xen/drivers/passthrough/pci.c      |  8 +++----
 4 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 7c7449d64f..5c4ab2c4dc 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -91,6 +91,16 @@ struct pci_ecam_ops {
     int (*init)(struct pci_config_window *);
 };
 
+/*
+ * struct to hold pci device bar.
+ */
+struct pdev_bar
+{
+    mfn_t start;
+    mfn_t end;
+    bool is_valid;
+};
+
 /* Default ECAM ops */
 extern const struct pci_ecam_ops pci_generic_ecam_ops;
 
@@ -125,6 +135,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 fd8c0f837a..8ea1aaeece 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -363,6 +363,41 @@ 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_DOWN(addr)) && (e<= PFN_DOWN(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;
+    struct dt_device_node *dt_node;
+    struct device *dev = (struct device *)pci_to_dev(pdev);
+    struct pdev_bar bar_data =  {
+        .start = start,
+        .end = end,
+        .is_valid = false
+    };
+
+    dt_node = pci_find_host_bridge_node(dev);
+
+    ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
+    if ( ret < 0 )
+        return ret;
+
+    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 29356d59a7..52453a05fe 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] xen/pci: replace call to is_memory_hole to pci_check_bar
Posted by Oleksandr 1 year, 7 months ago
On 05.08.22 18:43, Rahul Singh wrote:


Hello Rahul


Thank you very much for that patch!


> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>

I am not 100% sure regarding that. This is *completely* different patch 
from what Oleksandr initially made here:

https://lore.kernel.org/xen-devel/20220719174253.541965-2-olekstysh@gmail.com/

Copy below for the convenience:


+bool is_memory_hole(mfn_t start, mfn_t end)
+{
+    /* TODO: this needs to be properly implemented. */
+    return true;
+}




Patch looks good, just a couple of minor comments/nits.

>
> 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>
> ---
>   xen/arch/arm/include/asm/pci.h     | 12 ++++++++++
>   xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
>   xen/arch/x86/include/asm/pci.h     | 10 +++++++++
>   xen/drivers/passthrough/pci.c      |  8 +++----
>   4 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 7c7449d64f..5c4ab2c4dc 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -91,6 +91,16 @@ struct pci_ecam_ops {
>       int (*init)(struct pci_config_window *);
>   };
>   
> +/*
> + * struct to hold pci device bar.
> + */
> +struct pdev_bar
> +{
> +    mfn_t start;
> +    mfn_t end;
> +    bool is_valid;
> +};


Nit: This is only used by pci-host-common.c, so I think it could be 
declared there.



> +
>   /* Default ECAM ops */
>   extern const struct pci_ecam_ops pci_generic_ecam_ops;
>   
> @@ -125,6 +135,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 fd8c0f837a..8ea1aaeece 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -363,6 +363,41 @@ 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_DOWN(addr)) && (e<= PFN_DOWN(addr + len - 1)) )


Nit: white space after 'e' is missed in the last part of the check


> +        bar_data->is_valid =  true;
> +
> +    return 0;
> +}
> +
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> +{
> +    int ret;
> +    struct dt_device_node *dt_node;
> +    struct device *dev = (struct device *)pci_to_dev(pdev);


The cast is present here because of the const?

I would consider passing "const struct pci_dev *pdev" instead of "struct 
device *dev" to pci_find_host_bridge_node() and dropping conversion 
(pci<->dev) in both functions.


Something like below (not tested):

diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
index 5c4ab2c4dc..a17ef32252 100644
--- a/xen/arch/arm/include/asm/pci.h
+++ b/xen/arch/arm/include/asm/pci.h
@@ -116,7 +116,7 @@ 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);
+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 8ea1aaeece..3a64a7350f 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -243,10 +243,9 @@ err_exit:
  /*
   * Get host bridge node given a device attached to it.
   */
-struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
+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) )
@@ -380,14 +379,13 @@ bool pci_check_bar(const struct pci_dev *pdev, 
mfn_t start, mfn_t end)
  {
      int ret;
      struct dt_device_node *dt_node;
-    struct device *dev = (struct device *)pci_to_dev(pdev);
      struct pdev_bar bar_data =  {
          .start = start,
          .end = end,
          .is_valid = false
      };

-    dt_node = pci_find_host_bridge_node(dev);
+    dt_node = pci_find_host_bridge_node(pdev);

      ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
      if ( ret < 0 )


> +    struct pdev_bar bar_data =  {
> +        .start = start,
> +        .end = end,
> +        .is_valid = false
> +    };
> +
> +    dt_node = pci_find_host_bridge_node(dev);

     if ( !dt_node )
         return false;


> +
> +    ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
> +    if ( ret < 0 )
> +        return ret;

s/return ret;/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);
> +}


Nit: I would use simple #define instead of static inline here

But I am not 100% sure that x86 maintainers would be happy.


> +
>   #endif /* __X86_PCI_H__ */
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 29356d59a7..52453a05fe 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));

-- 
Regards,

Oleksandr Tyshchenko


Re: [PATCH] xen/pci: replace call to is_memory_hole to pci_check_bar
Posted by Rahul Singh 1 year, 7 months ago
Hi Oleksandr,


> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote:
> 
> 
> On 05.08.22 18:43, Rahul Singh wrote:
> 
> 
> Hello Rahul
> 
> 
> Thank you very much for that patch!
> 
> 
>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> 
> I am not 100% sure regarding that. This is *completely* different patch from what Oleksandr initially made here:
> 
> https://lore.kernel.org/xen-devel/20220719174253.541965-2-olekstysh@gmail.com/
> 
> Copy below for the convenience:
> 
> 
> +bool is_memory_hole(mfn_t start, mfn_t end)
> +{
> +    /* TODO: this needs to be properly implemented. */
> +    return true;
> +}
> 
> 
> 
> 
> Patch looks good, just a couple of minor comments/nits.

Ok. I will remove “From: … “ in next version.
> 
>> 
>> 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>
>> ---
>>  xen/arch/arm/include/asm/pci.h     | 12 ++++++++++
>>  xen/arch/arm/pci/pci-host-common.c | 35 ++++++++++++++++++++++++++++++
>>  xen/arch/x86/include/asm/pci.h     | 10 +++++++++
>>  xen/drivers/passthrough/pci.c      |  8 +++----
>>  4 files changed, 61 insertions(+), 4 deletions(-)
>> 
>> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
>> index 7c7449d64f..5c4ab2c4dc 100644
>> --- a/xen/arch/arm/include/asm/pci.h
>> +++ b/xen/arch/arm/include/asm/pci.h
>> @@ -91,6 +91,16 @@ struct pci_ecam_ops {
>>      int (*init)(struct pci_config_window *);
>>  };
>>  +/*
>> + * struct to hold pci device bar.
>> + */
>> +struct pdev_bar
>> +{
>> +    mfn_t start;
>> +    mfn_t end;
>> +    bool is_valid;
>> +};
> 
> 
> Nit: This is only used by pci-host-common.c, so I think it could be declared there.

Ack.
> 
> 
> 
>> +
>>  /* Default ECAM ops */
>>  extern const struct pci_ecam_ops pci_generic_ecam_ops;
>>  @@ -125,6 +135,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 fd8c0f837a..8ea1aaeece 100644
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -363,6 +363,41 @@ 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_DOWN(addr)) && (e<= PFN_DOWN(addr + len - 1)) )
> 
> 
> Nit: white space after 'e' is missed in the last part of the check

Ack.

> 
> 
>> +        bar_data->is_valid =  true;
>> +
>> +    return 0;
>> +}
>> +
>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>> +{
>> +    int ret;
>> +    struct dt_device_node *dt_node;
>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
> 
> 
> The cast is present here because of the const?

Yes you are right, cast is because of the const.
> 
> I would consider passing "const struct pci_dev *pdev" instead of "struct device *dev" to pci_find_host_bridge_node() and dropping conversion (pci<->dev) in both functions.

Yes make sense. I will do that in next version.
> 
> 
> Something like below (not tested):
> 
> diff --git a/xen/arch/arm/include/asm/pci.h b/xen/arch/arm/include/asm/pci.h
> index 5c4ab2c4dc..a17ef32252 100644
> --- a/xen/arch/arm/include/asm/pci.h
> +++ b/xen/arch/arm/include/asm/pci.h
> @@ -116,7 +116,7 @@ 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);
> +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 8ea1aaeece..3a64a7350f 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -243,10 +243,9 @@ err_exit:
>  /*
>   * Get host bridge node given a device attached to it.
>   */
> -struct dt_device_node *pci_find_host_bridge_node(struct device *dev)
> +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) )
> @@ -380,14 +379,13 @@ bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>  {
>      int ret;
>      struct dt_device_node *dt_node;
> -    struct device *dev = (struct device *)pci_to_dev(pdev);
>      struct pdev_bar bar_data =  {
>          .start = start,
>          .end = end,
>          .is_valid = false
>      };
> 
> -    dt_node = pci_find_host_bridge_node(dev);
> +    dt_node = pci_find_host_bridge_node(pdev);
> 
>      ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
>      if ( ret < 0 )
> 
> 
>> +    struct pdev_bar bar_data =  {
>> +        .start = start,
>> +        .end = end,
>> +        .is_valid = false
>> +    };
>> +
>> +    dt_node = pci_find_host_bridge_node(dev);
> 
>     if ( !dt_node )
>         return false;

Ack. 
> 
> 
>> +
>> +    ret = dt_for_each_range(dt_node, &is_bar_valid, &bar_data);
>> +    if ( ret < 0 )
>> +        return ret;
> 
> s/return ret;/return false;

Ack. 
> 
> 
>> +
>> +    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);
>> +}
> 
> 
> Nit: I would use simple #define instead of static inline here
> 
> But I am not 100% sure that x86 maintainers would be happy.
> 

Jan replied to this and I will check what is suggested by Jan.

Regards,
Rahul
Re: [PATCH] xen/pci: replace call to is_memory_hole to pci_check_bar
Posted by Julien Grall 1 year, 7 months ago
Hi Rahul,

This patch seems to have been sent in-reply-to the SMMUv1 patch. Was it 
intended?

On 09/08/2022 16:22, Rahul Singh wrote:
>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote:
>>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>>> +{
>>> +    int ret;
>>> +    struct dt_device_node *dt_node;
>>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
>>
>>
>> The cast is present here because of the const?
> 
> Yes you are right, cast is because of the const.
>>
>> I would consider passing "const struct pci_dev *pdev" instead of "struct device *dev" to pci_find_host_bridge_node() and dropping conversion (pci<->dev) in both functions.

It looks like this function was added without any callers. The commit 
message claim there will be some. Can you (or Oleksandr) confirm this is 
not going to be problem for future patches?

That said, I agree that the conversion pci -> dev -> pci is pointless. 
So I would say if there are use case where we only have a 'dev' in hand, 
then we could ask the caller to do the conversation or we provide an 
helper if there are too many cases.

> 
> Yes make sense. I will do that in next version.

While you are modifying the prototype for pci_find_host_bridge_node() 
can you consider to also constify the return (it should not be modified)?

In any case, the change suggested by Oleksandr should preferably be 
separate to this patch and added before.

Cheers,

-- 
Julien Grall
Re: [PATCH] xen/pci: replace call to is_memory_hole to pci_check_bar
Posted by Rahul Singh 1 year, 7 months ago
Hi Julien,

> On 9 Aug 2022, at 4:46 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi Rahul,
> 
> This patch seems to have been sent in-reply-to the SMMUv1 patch. Was it intended?

That was by mistake I want to send all the patches independently but somehow I send it 
from single git "send-email” command because of that I think this patch comes in-reply-to 
SMMUv1 patch.

> 
> On 09/08/2022 16:22, Rahul Singh wrote:
>>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote:
>>>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
>>>> +{
>>>> +    int ret;
>>>> +    struct dt_device_node *dt_node;
>>>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
>>> 
>>> 
>>> The cast is present here because of the const?
>> Yes you are right, cast is because of the const.
>>> 
>>> I would consider passing "const struct pci_dev *pdev" instead of "struct device *dev" to pci_find_host_bridge_node() and dropping conversion (pci<->dev) in both functions.
> 
> It looks like this function was added without any callers. The commit message claim there will be some. Can you (or Oleksandr) confirm this is not going to be problem for future patches?

I checked the whole PCI passthrough feature branch this function will be used when
we add iommu support for PCI device.  

> 
> That said, I agree that the conversion pci -> dev -> pci is pointless. So I would say if there are use case where we only have a 'dev' in hand, then we could ask the caller to do the conversation or we provide an helper if there are too many cases.
> 
>> Yes make sense. I will do that in next version.
> 
> While you are modifying the prototype for pci_find_host_bridge_node() can you consider to also constify the return (it should not be modified)?

Agree, I will constify the retrun also. 

> 
> In any case, the change suggested by Oleksandr should preferably be separate to this patch and added before.

Ack. 

Regards,
Rahul

Re: [PATCH] xen/pci: replace call to is_memory_hole to pci_check_bar
Posted by Oleksandr Tyshchenko 1 year, 7 months ago
On Tue, Aug 9, 2022 at 7:26 PM Rahul Singh <Rahul.Singh@arm.com> wrote:

> Hi Julien,
>

Hello Julien, Rahul

[sorry for possible format issues]


[snip]


>
>
> >
> > On 09/08/2022 16:22, Rahul Singh wrote:
> >>> On 8 Aug 2022, at 4:30 pm, Oleksandr <olekstysh@gmail.com> wrote:
> >>>> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t
> end)
> >>>> +{
> >>>> +    int ret;
> >>>> +    struct dt_device_node *dt_node;
> >>>> +    struct device *dev = (struct device *)pci_to_dev(pdev);
> >>>
> >>>
> >>> The cast is present here because of the const?
> >> Yes you are right, cast is because of the const.
> >>>
> >>> I would consider passing "const struct pci_dev *pdev" instead of
> "struct device *dev" to pci_find_host_bridge_node() and dropping conversion
> (pci<->dev) in both functions.
> >
> > It looks like this function was added without any callers. The commit
> message claim there will be some. Can you (or Oleksandr) confirm this is
> not going to be problem for future patches?
>
> I checked the whole PCI passthrough feature branch this function will be
> used when
> we add iommu support for PCI device.



Can confirm that, it will be called by the iommu code, as I understand
there won't be an issue, the more, the exact place where the
pci_find_host_bridge_node() will be called will have "pdev" in hand.


>
>
> >
> > That said, I agree that the conversion pci -> dev -> pci is pointless.
> So I would say if there are use case where we only have a 'dev' in hand,
> then we could ask the caller to do the conversation or we provide an helper
> if there are too many cases.
> >
> >> Yes make sense. I will do that in next version.
> >
> > While you are modifying the prototype for pci_find_host_bridge_node()
> can you consider to also constify the return (it should not be modified)?
>
> Agree, I will constify the retrun also.
>
> >
> > In any case, the change suggested by Oleksandr should preferably be
> separate to this patch and added before.
>
> Ack.
>
> Regards,
> Rahul
>
>

-- 
Regards,

Oleksandr Tyshchenko
Re: [PATCH] xen/pci: replace call to is_memory_hole to pci_check_bar
Posted by Jan Beulich 1 year, 7 months ago
On 08.08.2022 17:30, Oleksandr wrote:
> On 05.08.22 18:43, Rahul Singh wrote:
>> --- a/xen/arch/arm/pci/pci-host-common.c
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -363,6 +363,41 @@ 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)

Nit: No new uses of u64 please use uint64_t instead.

>> --- 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);
>> +}
> 
> 
> Nit: I would use simple #define instead of static inline here
> 
> But I am not 100% sure that x86 maintainers would be happy.

Quite the other way around - when possible we prefer inline functions.
And note that the two functions are strictly aliases of one another
(in which case a simplified

#define pci_check_bar is_memory_hole

might indeed have been worth a consideration, as there's no type
safety to be lost in such cases).

Jan