xen: avoid generation of stub <asm/pci.h> header

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

Posted by Oleksii Kurochko 6 months, 2 weeks ago

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
to add <asm/pci.h> directly alongside <xen/pci.h>. Only to
<arm/domain.c> <asm/pci.h> was added.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 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) {}
diff --git a/xen/arch/ppc/include/asm/pci.h b/xen/arch/ppc/include/asm/pci.h
deleted file mode 100644
index e76c8e5475..0000000000
--- a/xen/arch/ppc/include/asm/pci.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#ifndef __ASM_PPC_PCI_H__
-#define __ASM_PPC_PCI_H__
-
-struct arch_pci_dev {
-};
-
-#endif /* __ASM_PPC_PCI_H__ */
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 251b8761a8..168ca320ce 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -68,7 +68,18 @@ typedef union {
     };
 } pci_sbdf_t;
 
+#ifdef CONFIG_HAS_PCI
 #include <asm/pci.h>
+#else
+
+struct arch_pci_dev { };
+
+static always_inline bool is_pci_passthrough_enabled(void)
+{
+    return false;
+}
+
+#endif
 
 struct pci_dev_info {
     /*
-- 
2.41.0

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

Posted by Oleksii 6 months, 1 week ago

Hi all,

On Wed, 2023-11-01 at 12:15 +0200, 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
> to add <asm/pci.h> directly alongside <xen/pci.h>. Only to
> <arm/domain.c> <asm/pci.h> was added.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  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) {}
> diff --git a/xen/arch/ppc/include/asm/pci.h
> b/xen/arch/ppc/include/asm/pci.h
> deleted file mode 100644
> index e76c8e5475..0000000000
> --- a/xen/arch/ppc/include/asm/pci.h
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -#ifndef __ASM_PPC_PCI_H__
> -#define __ASM_PPC_PCI_H__
> -
> -struct arch_pci_dev {
> -};
> -
> -#endif /* __ASM_PPC_PCI_H__ */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 251b8761a8..168ca320ce 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -68,7 +68,18 @@ typedef union {
>      };
>  } pci_sbdf_t;
>  
> +#ifdef CONFIG_HAS_PCI
>  #include <asm/pci.h>
> +#else
> +
> +struct arch_pci_dev { };
> +
> +static always_inline bool is_pci_passthrough_enabled(void)
> +{
> +    return false;
> +}
> +
> +#endif
>  
>  struct pci_dev_info {
>      /*

I forgot to update xen/arch/{arm,ppc}/include/asm/Makefile:
    generic-y += pci.h

Should I send a new patch version or it can be updated durig merge?

~ Oleksii

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

Posted by Jan Beulich 6 months, 1 week ago

On 10.11.2023 10:30, Oleksii wrote:
> I forgot to update xen/arch/{arm,ppc}/include/asm/Makefile:
>     generic-y += pci.h
> 
> Should I send a new patch version or it can be updated durig merge?

See the reply regarding delay.h.

Jan

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

Posted by Julien Grall 6 months, 1 week ago

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 :).

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

Acked-by: Julien Grall <jgrall@amazon.com>

> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>   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,

-- 
Julien Grall

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

Posted by Oleksii 6 months, 1 week ago

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@amazon.com>
Thanks.

> 
> > 
> > Suggested-by: Jan Beulich <jbeulich@suse.com>
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> > ---
> >   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