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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 1 Sep 2022, Rahul Singh wrote:
> 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.
> + */
> +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)) )
> +        bar_data->is_valid =  true;


This patch looks good and you addressed all Jan's comment well. Before I
ack it, one question.

I know that you made this change to address Jan's comment but using
PFN_DOWN for the (s >= PFN_DOWN(addr)) check and PFN_UP for the (e <=
PFN_UP(addr + len - 1)) check means that we are relaxing the
requirements, aren't we?

I know that this discussion is a bit pointless because addr and len should
always be page aligned, and if they weren't it would be a mistake. But
assuming that they are not page aligned, wouldn't we want this check to
be a strict as possible?

Wouldn't we want to ensure that the [s,e] range is a strict subset of
[addr,addr+len-1] ? If so we would need to do the following instead:

    if ( (s <= e) && (s >= PFN_UP(addr)) && (e <= PFN_DOWN(addr + len - 1)) )
        bar_data->is_valid =  true;