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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 3 Sep 2022, at 8:18 am, Julien Grall <julien@xxxxxxx> wrote:
> 
> 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.

Ack.
> 
>> +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?

Yes, We can move the check outside the callback but I feel that if we check 
here then it is more
readable that we are checking for all possible values in one statement. Let me 
know your view on this.

> 
>> +        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).

I don’t see any use of this function in more places. As this function will be 
called during dom0 boot when the PCI devices are
added I don’t see any performance issues. We may need to revisit this function 
when we add ACPI PCI passthrough support.
I will add TODO that we need to revisit this function for ACPI PCI passthrough 
support.
 

Regards,
Rahul