[v8,5/7] xen/asm-generic: introduce generic device.h

Commit Message

Oleksii Kurochko Feb. 9, 2024, 6 p.m. UTC

Arm, PPC and RISC-V introduce the same things in asm/device.h, so
generic device.h was introduced.
Arm's device.h was taken as a base with the following changes:
 - #ifdef ACPI related things.
 - Rename #ifdef guards.
 - Add SPDX tag.
 - #ifdef CONFIG_HAS_DEVICE_TREE related things.
 - #ifdef-ing iommu related things with CONFIG_HAS_PASSTHROUGH.

Signed-off-by: Oleksii Kurochko <[e-mail]>
---
Changes in V8:
 - drop #ifdef HAS_PCI around DEV_PCI in enum device_type to be consistent with DEVICE_PCI_HOSTBRIDGE
 - drop the comment above enum device_type, after DEV_PCI is alaways present in enum device_type, so
   there is no need anymore in comment and DEV_TYPE_MAX.
 - drop #ifdef HAS_PCI around dev_is_pci() as DEV_PCI is alwayds define now.
 - rename args _name, _namestr, _class to name, namestr, class.
 - drop unnessary backslash after .class = class in defines ACPI_DEVICE_START() and DT_DEVICE_START()
 - update the commit message 
---
Changes in V7:
 - keeping DEVICE_PCI_HOSTBRIDGE available for every build based on the reply:
    https://lore.kernel.org/xen-devel/[e-mail]/
 - add comment above enum device_type.h with explanation about DEV_TYPE_MAX.        
 - separate patch  "[PATCH v6 9/9] xen/asm-generic: introduce generic device.h" into 3 patches.
---
Changes in V6:
 - Rebase only.
---
Changes in V5:
  - Removed generated file: xen/include/headers++.chk.new
  - Removed pointless #ifdef CONFIG_HAS_DEVICE_TREE ... #endif for PPC as
    CONFIG_HAS_DEVICE_TREE will be always used for PPC.
---
Changes in V4:
 - Updated the commit message
 - Switched Arm and PPC to asm-generic version of device.h
 - Replaced HAS_PCI with CONFIG_HAS_PCI
 - ifdef-ing iommu filed of dev_archdata struct with CONFIG_HAS_PASSTHROUGH
 - ifdef-ing iommu_fwspec of device struct with CONFIG_HAS_PASSTHROUGH
 - ifdef-ing DT related things with CONFIG_HAS_DEVICE_TREE
 - Updated the commit message ( remove a note with question about
   if device.h should be in asm-generic or not )
 - Replaced DEVICE_IC with DEVICE_INTERRUPT_CONTROLLER
 - Rationalized usage of CONFIG_HAS_* in device.h
 - Fixed indents for ACPI_DEVICE_START and ACPI_DEVICE_END
---
Changes in V3:
 - ifdef device tree related things.
 - update the commit message
---
Changes in V2:
	- take ( as common ) device.h from Arm as PPC and RISC-V use it as a base.
	- #ifdef PCI related things.
	- #ifdef ACPI related things.
	- rename DEVICE_GIC to DEVIC_IC.
	- rename #ifdef guards.
	- switch Arm and PPC to generic device.h
	- add SPDX tag
	- update the commit message
---
 xen/include/asm-generic/device.h | 149 +++++++++++++++++++++++++++++++
 1 file changed, 149 insertions(+)
 create mode 100644 xen/include/asm-generic/device.h

Comments

Jan Beulich Feb. 12, 2024, 2:19 p.m. UTC | #1

On 09.02.2024 19:00, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/include/asm-generic/device.h
> @@ -0,0 +1,149 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_GENERIC_DEVICE_H__
> +#define __ASM_GENERIC_DEVICE_H__
> +
> +#include <xen/stdbool.h>
> +
> +enum device_type
> +{
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    DEV_DT,
> +#endif
> +    DEV_PCI
> +};
> +
> +enum device_class
> +{
> +    DEVICE_SERIAL,
> +    DEVICE_IOMMU,
> +    DEVICE_INTERRUPT_CONTROLLER,
> +    DEVICE_PCI_HOSTBRIDGE,
> +    /* Use for error */
> +    DEVICE_UNKNOWN,
> +};
> +
> +struct dev_archdata {
> +#ifdef CONFIG_HAS_PASSTHROUGH
> +    void *iommu;    /* IOMMU private data */
> +#endif
> +};
> +
> +/* struct device - The basic device structure */
> +struct device
> +{
> +    enum device_type type;
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
> +#endif
> +    struct dev_archdata archdata;
> +#ifdef CONFIG_HAS_PASSTHROUGH
> +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
> +#endif
> +};
> +
> +typedef struct device device_t;
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +
> +#include <xen/device_tree.h>
> +
> +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> +
> +/**
> + *  device_init - Initialize a device
> + *  @dev: device to initialize
> + *  @class: class of the device (serial, network...)
> + *  @data: specific data for initializing the device
> + *
> + *  Return 0 on success.
> + */
> +int device_init(struct dt_device_node *dev, enum device_class class,
> +                const void *data);
> +
> +/**
> + * device_get_type - Get the type of the device
> + * @dev: device to match
> + *
> + * Return the device type on success or DEVICE_ANY on failure
> + */
> +enum device_class device_get_class(const struct dt_device_node *dev);
> +
> +#define DT_DEVICE_START(name_, namestr_, class_)            \

I don't think the trailing underscores are needed or helpful here
(or in the ACPI counterpart), ...

> +static const struct device_desc __dev_desc_##name_ __used   \
> +__section(".dev.info") = {                                  \
> +    .name = namestr_,                                       \
> +    .class = class_,

... seeing this all it would have taken was to avoid the two words
"name" and "class" (by e.g. using "ident" and "cls").

Nevertheless:
Acked-by: Jan Beulich <[e-mail]>

Jan

Julien Grall Feb. 13, 2024, 6:09 p.m. UTC | #2

Hi Oleksii,

On 09/02/2024 18:00, Oleksii Kurochko wrote:
> diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h
> new file mode 100644
> index 0000000000..6e56658271
> --- /dev/null
> +++ b/xen/include/asm-generic/device.h
> @@ -0,0 +1,149 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef __ASM_GENERIC_DEVICE_H__
> +#define __ASM_GENERIC_DEVICE_H__
> +
> +#include <xen/stdbool.h>
> +
> +enum device_type
> +{
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    DEV_DT,
> +#endif
> +    DEV_PCI
> +};
> +
> +enum device_class
> +{
> +    DEVICE_SERIAL,
> +    DEVICE_IOMMU,
> +    DEVICE_INTERRUPT_CONTROLLER,
> +    DEVICE_PCI_HOSTBRIDGE,
> +    /* Use for error */
> +    DEVICE_UNKNOWN,
> +};
> +
> +struct dev_archdata {
> +#ifdef CONFIG_HAS_PASSTHROUGH
> +    void *iommu;    /* IOMMU private data */
> +#endif > +};

It is a bit too late to change, but I thought I would point it if 
someone wants to send a follow-up. It is a bit odd to have a structure 
dev_archdata that, if I am not mistaken, is only used ...

> +
> +/* struct device - The basic device structure */
> +struct device
> +{
> +    enum device_type type;
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
> +#endif
> +    struct dev_archdata archdata;

... in struct device. Looking at the use, I believe this was only 
introduced to try to keep that SMMU code close to Linux. I would 
consider to fold the other structure and update dev_archdata() in 
drivers/passthrough/arm/smmu.c.

> +#ifdef CONFIG_HAS_PASSTHROUGH
> +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
> +#endif
> +};
> +
> +typedef struct device device_t;
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +
> +#include <xen/device_tree.h>
> +
> +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> +
> +/**
> + *  device_init - Initialize a device
> + *  @dev: device to initialize
> + *  @class: class of the device (serial, network...)
> + *  @data: specific data for initializing the device
> + *
> + *  Return 0 on success.
> + */
> +int device_init(struct dt_device_node *dev, enum device_class class,
> +                const void *data);
> +
> +/**
> + * device_get_type - Get the type of the device
> + * @dev: device to match
> + *
> + * Return the device type on success or DEVICE_ANY on failure
> + */
> +enum device_class device_get_class(const struct dt_device_node *dev);
> +
> +#define DT_DEVICE_START(name_, namestr_, class_)            \
> +static const struct device_desc __dev_desc_##name_ __used   \
> +__section(".dev.info") = {                                  \
> +    .name = namestr_,                                       \
> +    .class = class_,
> +
> +#define DT_DEVICE_END                                       \
> +};
> +
> +#else /* !CONFIG_HAS_DEVICE_TREE */
> +#define dev_is_dt(dev) ((void)(dev), false)
> +#endif /* CONFIG_HAS_DEVICE_TREE */
> +
> +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> +
> +struct device_desc {
> +    /* Device name */
> +    const char *name;
> +    /* Device class */
> +    enum device_class class;
> +
> +#ifdef CONFIG_HAS_DEVICE_TREE
> +
> +    /* List of devices supported by this driver */
> +    const struct dt_device_match *dt_match;
> +    /*
> +     * Device initialization.
> +     *
> +     * -EAGAIN is used to indicate that device probing is deferred.
> +     */
> +    int (*init)(struct dt_device_node *dev, const void *data);
> +
> +#endif
> +};
I am not sure I fully understand why "device_desc" is not protected by 
CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the config 
is disabled. Can you clarify?

Cheers,

Oleksii Kurochko Feb. 14, 2024, 9:12 a.m. UTC | #3

On Mon, 2024-02-12 at 15:19 +0100, Jan Beulich wrote:
> On 09.02.2024 19:00, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/xen/include/asm-generic/device.h
> > @@ -0,0 +1,149 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +#include <xen/stdbool.h>
> > +
> > +enum device_type
> > +{
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    DEV_DT,
> > +#endif
> > +    DEV_PCI
> > +};
> > +
> > +enum device_class
> > +{
> > +    DEVICE_SERIAL,
> > +    DEVICE_IOMMU,
> > +    DEVICE_INTERRUPT_CONTROLLER,
> > +    DEVICE_PCI_HOSTBRIDGE,
> > +    /* Use for error */
> > +    DEVICE_UNKNOWN,
> > +};
> > +
> > +struct dev_archdata {
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > +    void *iommu;    /* IOMMU private data */
> > +#endif
> > +};
> > +
> > +/* struct device - The basic device structure */
> > +struct device
> > +{
> > +    enum device_type type;
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > +#endif
> > +    struct dev_archdata archdata;
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
> > instance data */
> > +#endif
> > +};
> > +
> > +typedef struct device device_t;
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +
> > +#include <xen/device_tree.h>
> > +
> > +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> > +
> > +/**
> > + *  device_init - Initialize a device
> > + *  @dev: device to initialize
> > + *  @class: class of the device (serial, network...)
> > + *  @data: specific data for initializing the device
> > + *
> > + *  Return 0 on success.
> > + */
> > +int device_init(struct dt_device_node *dev, enum device_class
> > class,
> > +                const void *data);
> > +
> > +/**
> > + * device_get_type - Get the type of the device
> > + * @dev: device to match
> > + *
> > + * Return the device type on success or DEVICE_ANY on failure
> > + */
> > +enum device_class device_get_class(const struct dt_device_node
> > *dev);
> > +
> > +#define DT_DEVICE_START(name_, namestr_, class_)            \
> 
> I don't think the trailing underscores are needed or helpful here
> (or in the ACPI counterpart), ...
> 
> > +static const struct device_desc __dev_desc_##name_ __used   \
> > +__section(".dev.info") = {                                  \
> > +    .name = namestr_,                                       \
> > +    .class = class_,
> 
> ... seeing this all it would have taken was to avoid the two words
> "name" and "class" (by e.g. using "ident" and "cls").
> 
> Nevertheless:
> Acked-by: Jan Beulich <[e-mail]>
Thanks.

I'll apply your comments in the next patch version.

~ Oleksii

Oleksii Kurochko Feb. 14, 2024, 9:32 a.m. UTC | #4

On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote:
> Hi Oleksii,
> 
> On 09/02/2024 18:00, Oleksii Kurochko wrote:
> > diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-
> > generic/device.h
> > new file mode 100644
> > index 0000000000..6e56658271
> > --- /dev/null
> > +++ b/xen/include/asm-generic/device.h
> > @@ -0,0 +1,149 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef __ASM_GENERIC_DEVICE_H__
> > +#define __ASM_GENERIC_DEVICE_H__
> > +
> > +#include <xen/stdbool.h>
> > +
> > +enum device_type
> > +{
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    DEV_DT,
> > +#endif
> > +    DEV_PCI
> > +};
> > +
> > +enum device_class
> > +{
> > +    DEVICE_SERIAL,
> > +    DEVICE_IOMMU,
> > +    DEVICE_INTERRUPT_CONTROLLER,
> > +    DEVICE_PCI_HOSTBRIDGE,
> > +    /* Use for error */
> > +    DEVICE_UNKNOWN,
> > +};
> > +
> > +struct dev_archdata {
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > +    void *iommu;    /* IOMMU private data */
> > +#endif > +};
> 
> It is a bit too late to change, but I thought I would point it if 
> someone wants to send a follow-up. It is a bit odd to have a
> structure 
> dev_archdata that, if I am not mistaken, is only used ...
> 
> > +
> > +/* struct device - The basic device structure */
> > +struct device
> > +{
> > +    enum device_type type;
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > +#endif
> > +    struct dev_archdata archdata;
> 
> ... in struct device. Looking at the use, I believe this was only 
> introduced to try to keep that SMMU code close to Linux. I would 
> consider to fold the other structure and update dev_archdata() in 
> drivers/passthrough/arm/smmu.c.
I can do that in separate patch. struct dev_archdata was left because
of drivers/passthrough/arm/smmu.c.

> 
> > +#ifdef CONFIG_HAS_PASSTHROUGH
> > +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
> > instance data */
> > +#endif
> > +};
> > +
> > +typedef struct device device_t;
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +
> > +#include <xen/device_tree.h>
> > +
> > +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> > +
> > +/**
> > + *  device_init - Initialize a device
> > + *  @dev: device to initialize
> > + *  @class: class of the device (serial, network...)
> > + *  @data: specific data for initializing the device
> > + *
> > + *  Return 0 on success.
> > + */
> > +int device_init(struct dt_device_node *dev, enum device_class
> > class,
> > +                const void *data);
> > +
> > +/**
> > + * device_get_type - Get the type of the device
> > + * @dev: device to match
> > + *
> > + * Return the device type on success or DEVICE_ANY on failure
> > + */
> > +enum device_class device_get_class(const struct dt_device_node
> > *dev);
> > +
> > +#define DT_DEVICE_START(name_, namestr_, class_)            \
> > +static const struct device_desc __dev_desc_##name_ __used   \
> > +__section(".dev.info") = {                                  \
> > +    .name = namestr_,                                       \
> > +    .class = class_,
> > +
> > +#define DT_DEVICE_END                                       \
> > +};
> > +
> > +#else /* !CONFIG_HAS_DEVICE_TREE */
> > +#define dev_is_dt(dev) ((void)(dev), false)
> > +#endif /* CONFIG_HAS_DEVICE_TREE */
> > +
> > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> > +
> > +struct device_desc {
> > +    /* Device name */
> > +    const char *name;
> > +    /* Device class */
> > +    enum device_class class;
> > +
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> > +
> > +    /* List of devices supported by this driver */
> > +    const struct dt_device_match *dt_match;
> > +    /*
> > +     * Device initialization.
> > +     *
> > +     * -EAGAIN is used to indicate that device probing is
> > deferred.
> > +     */
> > +    int (*init)(struct dt_device_node *dev, const void *data);
> > +
> > +#endif
> > +};
> I am not sure I fully understand why "device_desc" is not protected
> by 
> CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the
> config 
> is disabled. Can you clarify?
I thought that one day struct device_desc and acpi_device_desc will be
"merged", and so decided just to #ifdef only DEVICE_TREE specific
fields.
Another one reason it is if to protect fully struct device_desc then it
would be needed more #ifdef in arm/device.c ( for example,
device_init() should be all protected then ) what will require to ifdef
all calls of device_init(). As an option device_init can can be defined
in case when !CONFIG_HAS_DEVICE_TREE as:
   int __init device_init(struct dt_device_node *dev, enum device_class
   class,
                          const void *data)
   {
    return -EBADF;
   }
   
The similar thing will be needed for device_get_class() in Arm's device.c.

Would it be better to ifdef full struct device_desc ?

~ Oleksii

Julien Grall Feb. 14, 2024, 12:09 p.m. UTC | #5

Hi Oleksii,

On 14/02/2024 09:32, Oleksii wrote:
> On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote:
>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>> +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
>>> instance data */
>>> +#endif
>>> +};
>>> +
>>> +typedef struct device device_t;
>>> +
>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>> +
>>> +#include <xen/device_tree.h>
>>> +
>>> +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
>>> +
>>> +/**
>>> + *  device_init - Initialize a device
>>> + *  @dev: device to initialize
>>> + *  @class: class of the device (serial, network...)
>>> + *  @data: specific data for initializing the device
>>> + *
>>> + *  Return 0 on success.
>>> + */
>>> +int device_init(struct dt_device_node *dev, enum device_class
>>> class,
>>> +                const void *data);
>>> +
>>> +/**
>>> + * device_get_type - Get the type of the device
>>> + * @dev: device to match
>>> + *
>>> + * Return the device type on success or DEVICE_ANY on failure
>>> + */
>>> +enum device_class device_get_class(const struct dt_device_node
>>> *dev);
>>> +
>>> +#define DT_DEVICE_START(name_, namestr_, class_)            \
>>> +static const struct device_desc __dev_desc_##name_ __used   \
>>> +__section(".dev.info") = {                                  \
>>> +    .name = namestr_,                                       \
>>> +    .class = class_,
>>> +
>>> +#define DT_DEVICE_END                                       \
>>> +};
>>> +
>>> +#else /* !CONFIG_HAS_DEVICE_TREE */
>>> +#define dev_is_dt(dev) ((void)(dev), false)
>>> +#endif /* CONFIG_HAS_DEVICE_TREE */
>>> +
>>> +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
>>> +
>>> +struct device_desc {
>>> +    /* Device name */
>>> +    const char *name;
>>> +    /* Device class */
>>> +    enum device_class class;
>>> +
>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>> +
>>> +    /* List of devices supported by this driver */
>>> +    const struct dt_device_match *dt_match;
>>> +    /*
>>> +     * Device initialization.
>>> +     *
>>> +     * -EAGAIN is used to indicate that device probing is
>>> deferred.
>>> +     */
>>> +    int (*init)(struct dt_device_node *dev, const void *data);
>>> +
>>> +#endif
>>> +};
>> I am not sure I fully understand why "device_desc" is not protected
>> by
>> CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the
>> config
>> is disabled. Can you clarify?
> I thought that one day struct device_desc and acpi_device_desc will be
> "merged", and so decided just to #ifdef only DEVICE_TREE specific
> fields.

It might be possible to merge the two if we were using an union for the 
ACPI/DT specific part. However the majority of the parsing code needs to 
differ. So I am not convinced there would be any value to merge the two 
structures.

> Another one reason it is if to protect fully struct device_desc then it
> would be needed more #ifdef in arm/device.c ( for example,
> device_init() should be all protected then ) what will require to ifdef
> all calls of device_init(). As an option device_init can can be defined
> in case when !CONFIG_HAS_DEVICE_TREE as:
>     int __init device_init(struct dt_device_node *dev, enum device_class
>     class,
>                            const void *data)
>     {
>      return -EBADF;
>     }
>     
> The similar thing will be needed for device_get_class() in Arm's device.c.

I agree that in theory device_init() & co should be protected with 
CONFIG_HAS_DEVICE_TREE. However, it is not possible to compile Xen on 
Arm without the Device-Tree part today. So I don't view adding the 
#ifdef or any extra stub as necessary today.

This may be useful in the future though. Note this is not a request to 
modify the patch more than...

> 
> Would it be better to ifdef full struct device_desc ?
.. moving structure within the #ifdef.

Cheers,

Oleksii Kurochko Feb. 15, 2024, 4:54 p.m. UTC | #6

> 
> On 14/02/2024 09:32, Oleksii wrote:
> > On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote:
> > > > +#ifdef CONFIG_HAS_PASSTHROUGH
> > > > +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
> > > > instance data */
> > > > +#endif
> > > > +};
> > > > +
> > > > +typedef struct device device_t;
> > > > +
> > > > +#ifdef CONFIG_HAS_DEVICE_TREE
> > > > +
> > > > +#include <xen/device_tree.h>
> > > > +
> > > > +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
> > > > +
> > > > +/**
> > > > + *  device_init - Initialize a device
> > > > + *  @dev: device to initialize
> > > > + *  @class: class of the device (serial, network...)
> > > > + *  @data: specific data for initializing the device
> > > > + *
> > > > + *  Return 0 on success.
> > > > + */
> > > > +int device_init(struct dt_device_node *dev, enum device_class
> > > > class,
> > > > +                const void *data);
> > > > +
> > > > +/**
> > > > + * device_get_type - Get the type of the device
> > > > + * @dev: device to match
> > > > + *
> > > > + * Return the device type on success or DEVICE_ANY on failure
> > > > + */
> > > > +enum device_class device_get_class(const struct dt_device_node
> > > > *dev);
> > > > +
> > > > +#define DT_DEVICE_START(name_, namestr_, class_)            \
> > > > +static const struct device_desc __dev_desc_##name_ __used   \
> > > > +__section(".dev.info") = {                                  \
> > > > +    .name = namestr_,                                       \
> > > > +    .class = class_,
> > > > +
> > > > +#define DT_DEVICE_END                                       \
> > > > +};
> > > > +
> > > > +#else /* !CONFIG_HAS_DEVICE_TREE */
> > > > +#define dev_is_dt(dev) ((void)(dev), false)
> > > > +#endif /* CONFIG_HAS_DEVICE_TREE */
> > > > +
> > > > +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
> > > > +
> > > > +struct device_desc {
> > > > +    /* Device name */
> > > > +    const char *name;
> > > > +    /* Device class */
> > > > +    enum device_class class;
> > > > +
> > > > +#ifdef CONFIG_HAS_DEVICE_TREE
> > > > +
> > > > +    /* List of devices supported by this driver */
> > > > +    const struct dt_device_match *dt_match;
> > > > +    /*
> > > > +     * Device initialization.
> > > > +     *
> > > > +     * -EAGAIN is used to indicate that device probing is
> > > > deferred.
> > > > +     */
> > > > +    int (*init)(struct dt_device_node *dev, const void *data);
> > > > +
> > > > +#endif
> > > > +};
> > > I am not sure I fully understand why "device_desc" is not
> > > protected
> > > by
> > > CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the
> > > config
> > > is disabled. Can you clarify?
> > I thought that one day struct device_desc and acpi_device_desc will
> > be
> > "merged", and so decided just to #ifdef only DEVICE_TREE specific
> > fields.
> 
> It might be possible to merge the two if we were using an union for
> the 
> ACPI/DT specific part. However the majority of the parsing code needs
> to 
> differ. So I am not convinced there would be any value to merge the
> two 
> structures.
In this case, let's have two separate structures.

This is not the current situation, and I don't have a specific example.
It appears that all architectures will use Device Tree or ACPI.
However, does it make sense to keep 'struct device_desc' more generic
to accommodate non-DT or non-ACPI cases?

I am okay with making the following change, but I am just curious if
what I mentioned above makes sense at all:

#ifdef CONFIG_HAS_DEVICE_TREE
struct device_desc {
    /* Device name */
    const char *name;
    /* Device class */
    enum device_class class;

    /* List of devices supported by this driver */
    const struct dt_device_match *dt_match;
    /*
     * Device initialization.
     *
     * -EAGAIN is used to indicate that device probing is deferred.
     */
    int (*init)(struct dt_device_node *dev, const void *data);
};
#endif /* CONFIG_HAS_DEVICE_TREE */ 

> 
> > Another one reason it is if to protect fully struct device_desc
> > then it
> > would be needed more #ifdef in arm/device.c ( for example,
> > device_init() should be all protected then ) what will require to
> > ifdef
> > all calls of device_init(). As an option device_init can can be
> > defined
> > in case when !CONFIG_HAS_DEVICE_TREE as:
> >     int __init device_init(struct dt_device_node *dev, enum
> > device_class
> >     class,
> >                            const void *data)
> >     {
> >      return -EBADF;
> >     }
> >     
> > The similar thing will be needed for device_get_class() in Arm's
> > device.c.
> 
> I agree that in theory device_init() & co should be protected with 
> CONFIG_HAS_DEVICE_TREE. However, it is not possible to compile Xen on
> Arm without the Device-Tree part today. So I don't view adding the 
> #ifdef or any extra stub as necessary today.
> 
> This may be useful in the future though. Note this is not a request
> to 
> modify the patch more than...
> 
> > 
> > Would it be better to ifdef full struct device_desc ?
> .. moving structure within the #ifdef.
Well, I'll update the commit message of the next patch that it is not
possible to compile Xen without CONFIG_HAS_DEVICE_TREE, so
device_init() and Co won't be protected by CONFIG_HAS_DEVICE_TREE.

~ Oleksii

Julien Grall Feb. 19, 2024, 7:04 p.m. UTC | #7

Hi Oleksii,

On 15/02/2024 16:54, Oleksii wrote:
>>
>> On 14/02/2024 09:32, Oleksii wrote:
>>> On Tue, 2024-02-13 at 18:09 +0000, Julien Grall wrote:
>>>>> +#ifdef CONFIG_HAS_PASSTHROUGH
>>>>> +    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU
>>>>> instance data */
>>>>> +#endif
>>>>> +};
>>>>> +
>>>>> +typedef struct device device_t;
>>>>> +
>>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>>> +
>>>>> +#include <xen/device_tree.h>
>>>>> +
>>>>> +#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
>>>>> +
>>>>> +/**
>>>>> + *  device_init - Initialize a device
>>>>> + *  @dev: device to initialize
>>>>> + *  @class: class of the device (serial, network...)
>>>>> + *  @data: specific data for initializing the device
>>>>> + *
>>>>> + *  Return 0 on success.
>>>>> + */
>>>>> +int device_init(struct dt_device_node *dev, enum device_class
>>>>> class,
>>>>> +                const void *data);
>>>>> +
>>>>> +/**
>>>>> + * device_get_type - Get the type of the device
>>>>> + * @dev: device to match
>>>>> + *
>>>>> + * Return the device type on success or DEVICE_ANY on failure
>>>>> + */
>>>>> +enum device_class device_get_class(const struct dt_device_node
>>>>> *dev);
>>>>> +
>>>>> +#define DT_DEVICE_START(name_, namestr_, class_)            \
>>>>> +static const struct device_desc __dev_desc_##name_ __used   \
>>>>> +__section(".dev.info") = {                                  \
>>>>> +    .name = namestr_,                                       \
>>>>> +    .class = class_,
>>>>> +
>>>>> +#define DT_DEVICE_END                                       \
>>>>> +};
>>>>> +
>>>>> +#else /* !CONFIG_HAS_DEVICE_TREE */
>>>>> +#define dev_is_dt(dev) ((void)(dev), false)
>>>>> +#endif /* CONFIG_HAS_DEVICE_TREE */
>>>>> +
>>>>> +#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
>>>>> +
>>>>> +struct device_desc {
>>>>> +    /* Device name */
>>>>> +    const char *name;
>>>>> +    /* Device class */
>>>>> +    enum device_class class;
>>>>> +
>>>>> +#ifdef CONFIG_HAS_DEVICE_TREE
>>>>> +
>>>>> +    /* List of devices supported by this driver */
>>>>> +    const struct dt_device_match *dt_match;
>>>>> +    /*
>>>>> +     * Device initialization.
>>>>> +     *
>>>>> +     * -EAGAIN is used to indicate that device probing is
>>>>> deferred.
>>>>> +     */
>>>>> +    int (*init)(struct dt_device_node *dev, const void *data);
>>>>> +
>>>>> +#endif
>>>>> +};
>>>> I am not sure I fully understand why "device_desc" is not
>>>> protected
>>>> by
>>>> CONFIG_HAS_DEVICE_TREE. The structure doesn't mean much when the
>>>> config
>>>> is disabled. Can you clarify?
>>> I thought that one day struct device_desc and acpi_device_desc will
>>> be
>>> "merged", and so decided just to #ifdef only DEVICE_TREE specific
>>> fields.
>>
>> It might be possible to merge the two if we were using an union for
>> the
>> ACPI/DT specific part. However the majority of the parsing code needs
>> to
>> differ. So I am not convinced there would be any value to merge the
>> two
>> structures.
> In this case, let's have two separate structures.
> 
> This is not the current situation, and I don't have a specific example.
> It appears that all architectures will use Device Tree or ACPI.
> However, does it make sense to keep 'struct device_desc' more generic
> to accommodate non-DT or non-ACPI cases?

I am not entirely sure what else to say. As I wrote before yes it could 
be made generic. But right now I don't see any values.

If you have any idea how to share the structure. Then feel free to make 
a proposal and I will review it.

Cheers,

Patch

diff --git a/xen/include/asm-generic/device.h b/xen/include/asm-generic/device.h
new file mode 100644
index 0000000000..6e56658271
--- /dev/null
+++ b/xen/include/asm-generic/device.h
@@ -0,0 +1,149 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef __ASM_GENERIC_DEVICE_H__
+#define __ASM_GENERIC_DEVICE_H__
+
+#include <xen/stdbool.h>
+
+enum device_type
+{
+#ifdef CONFIG_HAS_DEVICE_TREE
+    DEV_DT,
+#endif
+    DEV_PCI
+};
+
+enum device_class
+{
+    DEVICE_SERIAL,
+    DEVICE_IOMMU,
+    DEVICE_INTERRUPT_CONTROLLER,
+    DEVICE_PCI_HOSTBRIDGE,
+    /* Use for error */
+    DEVICE_UNKNOWN,
+};
+
+struct dev_archdata {
+#ifdef CONFIG_HAS_PASSTHROUGH
+    void *iommu;    /* IOMMU private data */
+#endif
+};
+
+/* struct device - The basic device structure */
+struct device
+{
+    enum device_type type;
+#ifdef CONFIG_HAS_DEVICE_TREE
+    struct dt_device_node *of_node; /* Used by drivers imported from Linux */
+#endif
+    struct dev_archdata archdata;
+#ifdef CONFIG_HAS_PASSTHROUGH
+    struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
+#endif
+};
+
+typedef struct device device_t;
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+
+#include <xen/device_tree.h>
+
+#define dev_is_dt(dev)  ((dev)->type == DEV_DT)
+
+/**
+ *  device_init - Initialize a device
+ *  @dev: device to initialize
+ *  @class: class of the device (serial, network...)
+ *  @data: specific data for initializing the device
+ *
+ *  Return 0 on success.
+ */
+int device_init(struct dt_device_node *dev, enum device_class class,
+                const void *data);
+
+/**
+ * device_get_type - Get the type of the device
+ * @dev: device to match
+ *
+ * Return the device type on success or DEVICE_ANY on failure
+ */
+enum device_class device_get_class(const struct dt_device_node *dev);
+
+#define DT_DEVICE_START(name_, namestr_, class_)            \
+static const struct device_desc __dev_desc_##name_ __used   \
+__section(".dev.info") = {                                  \
+    .name = namestr_,                                       \
+    .class = class_,
+
+#define DT_DEVICE_END                                       \
+};
+
+#else /* !CONFIG_HAS_DEVICE_TREE */
+#define dev_is_dt(dev) ((void)(dev), false)
+#endif /* CONFIG_HAS_DEVICE_TREE */
+
+#define dev_is_pci(dev) ((dev)->type == DEV_PCI)
+
+struct device_desc {
+    /* Device name */
+    const char *name;
+    /* Device class */
+    enum device_class class;
+
+#ifdef CONFIG_HAS_DEVICE_TREE
+
+    /* List of devices supported by this driver */
+    const struct dt_device_match *dt_match;
+    /*
+     * Device initialization.
+     *
+     * -EAGAIN is used to indicate that device probing is deferred.
+     */
+    int (*init)(struct dt_device_node *dev, const void *data);
+
+#endif
+};
+
+#ifdef CONFIG_ACPI
+
+struct acpi_device_desc {
+    /* Device name */
+    const char *name;
+    /* Device class */
+    enum device_class class;
+    /* type of device supported by the driver */
+    const int class_type;
+    /* Device initialization */
+    int (*init)(const void *data);
+};
+
+/**
+ *  acpi_device_init - Initialize a device
+ *  @class: class of the device (serial, network...)
+ *  @data: specific data for initializing the device
+ *
+ *  Return 0 on success.
+ */
+int acpi_device_init(enum device_class class,
+                     const void *data, int class_type);
+
+#define ACPI_DEVICE_START(name_, namestr_, class_)              \
+static const struct acpi_device_desc __dev_desc_##name_ __used  \
+__section(".adev.info") = {                                     \
+    .name = namestr_,                                           \
+    .class = class_,
+
+#define ACPI_DEVICE_END                                         \
+};
+
+#endif /* CONFIG_ACPI */
+
+#endif /* __ASM_GENERIC_DEVICE_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */