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

[Julien Grall <julien@xxxxxxx>]

Hi Rahul,

On 01/09/2022 10:29, 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@xxxxxxx>
> ---
> Changes in v3:
>   - fix minor comments
> ---
>   xen/arch/arm/include/asm/pci.h     |  2 ++
>   xen/arch/arm/pci/pci-host-common.c | 43 ++++++++++++++++++++++++++++++
>   xen/arch/x86/include/asm/pci.h     | 10 +++++++
>   xen/drivers/passthrough/pci.c      |  8 +++---
>   4 files changed, 59 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..0eb121666d 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.
> + */
I find this comment a bit misleading. What you are storing is a
candidate region. IOW, this may or may not be a PCI device bar.

Given the current use below, I would rename the structure to something 
more specific like: pdev_bar_check.
> +struct pdev_bar
> +{
> +    mfn_t start;
> +    mfn_t end;
> +    bool is_valid;
> +};
> +
>   /*
>    * List for all the pci host bridges.
>    */
> @@ -363,6 +373,39 @@ 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 *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_UP(addr + len - 1)) )
AFAICT 's'  and 'e' are provided by pci_check_bar() and will never 
change. So can we move the check 's <= e' outside of the callback?
> +        bar_data->is_valid =  true;
> +
> +    return 0;
> +}
> +
> +bool pci_check_bar(const struct pci_dev *pdev, mfn_t start, mfn_t end)
> +{
Other than the current calls in check_pdev(), do you have plan to use it 
in more places? The reason I am asking it is this function is 
non-trivial on Arm (dt_for_each_range() is quite complex).
Cheers,

--
Julien Grall