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

[Julien Grall <julien@xxxxxxx>]

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,

--
Julien Grall