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

[Julien Grall <julien@xxxxxxx>]

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

Cheers,

--
Julien Grall