Re: [PATCH] xen: avoid generation of stub <asm/pci.h> header

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-11-06 at 23:36 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 01/11/2023 10:15, Oleksii Kurochko wrote:
> > Platforms which doesn't have HAS_PCI enabled it is needed to
> > have <asm/pci.h>, which contains only an empty definition of
> > struct arch_pci_dev ( except ARM, it introduces several
> > ARM-specific functions ).
> > 
> > Also, for architectures ( such as PPC or RISC-V ) on initial
> > stages of adding support, it is needed to generate <asm/pci.h>
> > for only define the mentioned above arch_pci_dev structure.
> > 
> > For the Arm-only stubs ( mentioned in <asm/pci.h> for disabled
> > HAS_PCI and ARM-specific) will be needed
> NIT: You seem to have a mix of ARM and Arm within the same commit 
> message. I know that there are debate on which one to use (even
> though 
> the latter is correct). I am ok if you want to either, but please at 
> least be consistent :).
Thanks. I'll be consistent in the future, and I am OK if the commit
message will be changed during the merge ( if it is convenient ).
> 
> > to add <asm/pci.h> directly alongside <xen/pci.h>. Only to
> > <arm/domain.c> <asm/pci.h> was added.
> 
> You are lucky here because there is only one place needed, so
> including 
> both <xen/pci.h> and <asm/pci.h> is ok. However, I am not sure I like
> this approach as a general solution as it may require to include both
> the common and arch specific header in multiple places.
> 
> Anyway, we can discuss this in more details once we have an example.
This is not the best solution, but it works for now.
Let's discuss it again when we will face the necessity of including
both headers.
> 
> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
Thanks.

> 
> > 
> > Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> >   xen/arch/arm/domain_build.c    |  1 +
> >   xen/arch/arm/include/asm/pci.h |  7 -------
> >   xen/arch/ppc/include/asm/pci.h |  7 -------
> >   xen/include/xen/pci.h          | 11 +++++++++++
> >   4 files changed, 12 insertions(+), 14 deletions(-)
> >   delete mode 100644 xen/arch/ppc/include/asm/pci.h
> > 
> > diff --git a/xen/arch/arm/domain_build.c
> > b/xen/arch/arm/domain_build.c
> > index 49792dd590..2dd2926b41 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -23,6 +23,7 @@
> >   #include <asm/kernel.h>
> >   #include <asm/setup.h>
> >   #include <asm/tee/tee.h>
> > +#include <asm/pci.h>
> >   #include <asm/platform.h>
> >   #include <asm/psci.h>
> >   #include <asm/setup.h>
> > diff --git a/xen/arch/arm/include/asm/pci.h
> > b/xen/arch/arm/include/asm/pci.h
> > index 8cb46f6b71..7f77226c9b 100644
> > --- a/xen/arch/arm/include/asm/pci.h
> > +++ b/xen/arch/arm/include/asm/pci.h
> > @@ -130,13 +130,6 @@ bool pci_check_bar(const struct pci_dev *pdev,
> > mfn_t start, mfn_t end);
> >   
> >   #else   /*!CONFIG_HAS_PCI*/
> >   
> > -struct arch_pci_dev { };
> > -
> > -static always_inline bool is_pci_passthrough_enabled(void)
> > -{
> > -    return false;
> > -}
> > -
> >   struct pci_dev;
> >   
> >   static inline void arch_pci_init_pdev(struct pci_dev *pdev) {}
> 
> Some of the definion in !CONFIG_HAS_PCI seems unnecessary. This one
> is 
> an example. I will have a look to clean them up after your patch is 
> committed.
> 
> Cheers,
> 
~ Oleksii