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

[Oleksandr <olekstysh@xxxxxxxxx>]

On 05.08.22 18:43, Rahul Singh wrote:


Hello Rahul


Thank you very much for that patch!


> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
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@xxxxxxxxx/

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