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 6 Sep 2022, at 10:53 am, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 06/09/2022 10:39, Rahul Singh wrote:
>> 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.
> 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