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

Commit Message

Oleksii Kurochko Nov. 1, 2023, 10:15 a.m. UTC

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 <[e-mail]>
Signed-off-by: Oleksii Kurochko <[e-mail]>
---
 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

Comments

Jan Beulich Nov. 2, 2023, 8:39 a.m. UTC | #1

On 01.11.2023 11: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
> to add <asm/pci.h> directly alongside <xen/pci.h>. Only to
> <arm/domain.c> <asm/pci.h> was added.
> 
> Suggested-by: Jan Beulich <[e-mail]>
> Signed-off-by: Oleksii Kurochko <[e-mail]>

Reviewed-by: Jan Beulich <[e-mail]>
albeit with two remarks:

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

This minimal scope of the #ifdef will do for now, but will likely want
extending down the road. Even what's visible in context is already an
entity which should be entirely unused in the code base when !HAS_PCI.

> +struct arch_pci_dev { };
> +
> +static always_inline bool is_pci_passthrough_enabled(void)

Perhaps s/always_inline/inline/ as this is moved here. We really shouldn't
use always_inline unless actually have a clear purpose.

Jan

Oleksii Kurochko Nov. 2, 2023, 9:24 a.m. UTC | #2

On Thu, 2023-11-02 at 09:39 +0100, Jan Beulich wrote:
> On 01.11.2023 11: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
> > to add <asm/pci.h> directly alongside <xen/pci.h>. Only to
> > <arm/domain.c> <asm/pci.h> was added.
> > 
> > Suggested-by: Jan Beulich <[e-mail]>
> > Signed-off-by: Oleksii Kurochko <[e-mail]>
> 
> Reviewed-by: Jan Beulich <[e-mail]>
> albeit with two remarks:

Thanks for the remarks.

> 
> > --- 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
> 
> This minimal scope of the #ifdef will do for now, but will likely
> want
> extending down the road. Even what's visible in context is already an
> entity which should be entirely unused in the code base when
> !HAS_PCI.
> 
> > +struct arch_pci_dev { };
> > +
> > +static always_inline bool is_pci_passthrough_enabled(void)
> 
> Perhaps s/always_inline/inline/ as this is moved here. We really
> shouldn't
> use always_inline unless actually have a clear purpose.
Could it be fixed during the commit ( in case there won't be any other
critical comments about this patch )?

~ Oleksii

Jan Beulich Nov. 2, 2023, 9:35 a.m. UTC | #3

On 02.11.2023 10:24, Oleksii wrote:
> On Thu, 2023-11-02 at 09:39 +0100, Jan Beulich wrote:
>> On 01.11.2023 11:15, Oleksii Kurochko wrote:
>>> --- 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
>>
>> This minimal scope of the #ifdef will do for now, but will likely
>> want
>> extending down the road. Even what's visible in context is already an
>> entity which should be entirely unused in the code base when
>> !HAS_PCI.
>>
>>> +struct arch_pci_dev { };
>>> +
>>> +static always_inline bool is_pci_passthrough_enabled(void)
>>
>> Perhaps s/always_inline/inline/ as this is moved here. We really
>> shouldn't
>> use always_inline unless actually have a clear purpose.
> Could it be fixed during the commit ( in case there won't be any other
> critical comments about this patch )?

Oh, yes, sure. I should have said so.

Jan

Julien Grall Nov. 6, 2023, 11:36 p.m. UTC | #4

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 <[e-mail]>

> 
> Suggested-by: Jan Beulich <[e-mail]>
> Signed-off-by: Oleksii Kurochko <[e-mail]>
> ---
>   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 Kurochko Nov. 9, 2023, 9:01 a.m. UTC | #5

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 <[e-mail]>
Thanks.

> 
> > 
> > Suggested-by: Jan Beulich <[e-mail]>
> > Signed-off-by: Oleksii Kurochko <[e-mail]>
> > ---
> >   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

Oleksii Kurochko Nov. 10, 2023, 9:30 a.m. UTC | #6

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 <[e-mail]>
> Signed-off-by: Oleksii Kurochko <[e-mail]>
> ---
>  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

Jan Beulich Nov. 10, 2023, 9:34 a.m. UTC | #7

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

Patch

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 {
     /*