Hi Julien,
> On 6 Sep 2022, at 10:53 am, Julien Grall <jul...@xen.org> wrote:
>
>
>
> On 06/09/2022 10:39, Rahul Singh wrote:
>> Hi Julien,
>>> On 3 Sep 2022, at 8:18 am, Julien Grall <jul...@xen.org> 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.si...@arm.com>
>>>> ---
>>>> 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.
> The readability is really a matter of taste here. But my point is more on the
> number of time a check is done.
>
> It seems pointless to do the same check N times when you know the values are
> not going to change. Admittedly, the operation is fast (this is a comparison)
> and N should be small (?).
>
> However, I think it raises the question on where do you draw the line?
>
> Personally, I think all invariant should be checked outside of callbacks. So
> the line is very clear.
>
I will move the check for "s <=e” outside the callback and will send it for
review.
Regards,
Rahul
