Hi Julien,
> On 3 Sep 2022, at 8:18 am, Julien Grall <[email protected]> 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 <[email protected]>
>> ---
>> 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
