Re: [PATCH v6 9/9] xen/asm-generic: introduce generic device.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

On Thu, 2023-12-21 at 19:38 +0000, Julien Grall wrote:
> Hi,
> 
> On 20/12/2023 14:08, Oleksii Kurochko wrote:
> > Arm, PPC and RISC-V use the same device.h thereby device.h
> > was moved to asm-generic. Arm's device.h was taken as a base with
> > the following changes:
> >   - #ifdef PCI related things.
> >   - #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.
> > 
> > Also Arm and PPC are switched to asm-generic version of device.h
> > 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > 
> >       Jan wrote the following:
> >         Overall I think there are too many changes done all in one
> > go here.
> >         But it's mostly Arm which is affected, so I'll leave
> > judging about that
> >         to the Arm maintainers.
> >       
> >       Arm maintainers, will it be fine for you to not split the
> > patch?
> 
> So in general I agree with Jan, patches should be kept small so they
> are 
> easy to review.
> 
> Given the discussion has been on-going for a while (we are at v6),  I
> will give an attempt to review the patch as-is. But in the future, 
> please try to split. The smaller it is, the easier to review. For
> code 
> movement and refactoring, I tend to first have a few refactoring
> patches 
> and then move the code in a separate patch. So it is easier to spot
> the 
> differences.
Thanks, I'll separate the patch.
> 
> > ---
> > 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/arch/arm/device.c                         |  15 ++-
> >   xen/arch/arm/domain_build.c                   |   2 +-
> >   xen/arch/arm/gic-v2.c                         |   4 +-
> >   xen/arch/arm/gic-v3.c                         |   6 +-
> >   xen/arch/arm/gic.c                            |   4 +-
> >   xen/arch/arm/include/asm/Makefile             |   1 +
> >   xen/arch/ppc/include/asm/Makefile             |   1 +
> >   xen/arch/ppc/include/asm/device.h             |  53 --------
> >   .../asm => include/asm-generic}/device.h      | 125 +++++++++++--
> > -----
> >   9 files changed, 102 insertions(+), 109 deletions(-)
> >   delete mode 100644 xen/arch/ppc/include/asm/device.h
> >   rename xen/{arch/arm/include/asm => include/asm-generic}/device.h
> > (79%)
> > 
> > diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> > index 1f631d3274..affbe79f9a 100644
> > --- a/xen/arch/arm/device.c
> > +++ b/xen/arch/arm/device.c
> > @@ -16,7 +16,10 @@
> >   #include <xen/lib.h>
> >   
> >   extern const struct device_desc _sdevice[], _edevice[];
> > +
> > +#ifdef CONFIG_ACPI
> >   extern const struct acpi_device_desc _asdevice[], _aedevice[];
> > +#endif
> >   
> >   int __init device_init(struct dt_device_node *dev, enum
> > device_class class,
> >                          const void *data)
> > @@ -45,6 +48,7 @@ int __init device_init(struct dt_device_node
> > *dev, enum device_class class,
> >       return -EBADF;
> >   }
> >   
> > +#ifdef CONFIG_ACPI
> >   int __init acpi_device_init(enum device_class class, const void
> > *data, int class_type)
> >   {
> >       const struct acpi_device_desc *desc;
> > @@ -61,6 +65,7 @@ int __init acpi_device_init(enum device_class
> > class, const void *data, int class
> >   
> >       return -EBADF;
> >   }
> > +#endif
> >   
> >   enum device_class device_get_class(const struct dt_device_node
> > *dev)
> >   {
> > @@ -329,9 +334,13 @@ int handle_device(struct domain *d, struct
> > dt_device_node *dev, p2m_type_t p2mt,
> >       struct map_range_data mr_data = {
> >           .d = d,
> >           .p2mt = p2mt,
> > -        .skip_mapping = !own_device ||
> > -                        (is_pci_passthrough_enabled() &&
> > -                        (device_get_class(dev) ==
> > DEVICE_PCI_HOSTBRIDGE)),
> > +        .skip_mapping =
> > +                        !own_device
> > +#ifdef CONFIG_HAS_PCI
> > +                        || (is_pci_passthrough_enabled() &&
> > +                        (device_get_class(dev) ==
> > DEVICE_PCI_HOSTBRIDGE))
> > +#endif
> 
> So the #ifdef is only here because DEVICE_PCI_HOSTBRIDGE is not
> defined. 
> It is not clear what's the actual problem of keeping 
> DEVICE_PCI_HOSTBRIDGE available for every build.
The only reason for that is that it seems to be used only in thecase when 
CONFIG_HAS_PCI is enabled ( probably not all archs will
have PCI support ). Generally,
I think it's okay not to use #ifdef DEVICE_PCI_HOSTBRIDGE to keep
the Arm code cleaner.

Does it make sense?
> 
> In fact, we have tried to get away from #ifdef in order to make
> ensure 
> we can catch any build failure without a randconfig (see use of 
> IS_ENABLED() which would not apply work here).
It would be nice to use IS_ENABLED but in this case still need to
something with definition of DEVICE_PCI_HOSTBRIDGE.

> 
> [...]
> 
> > diff --git a/xen/arch/ppc/include/asm/device.h
> > b/xen/arch/ppc/include/asm/device.h
> > deleted file mode 100644
> > index 8253e61d51..0000000000
> > --- a/xen/arch/ppc/include/asm/device.h
> > +++ /dev/null
> > @@ -1,53 +0,0 @@
> > -/* SPDX-License-Identifier: GPL-2.0-only */
> > -#ifndef __ASM_PPC_DEVICE_H__
> > -#define __ASM_PPC_DEVICE_H__
> > -
> > -enum device_type
> > -{
> > -    DEV_DT,
> > -    DEV_PCI,
> > -};
> > -
> > -struct device {
> > -    enum device_type type;
> > -#ifdef CONFIG_HAS_DEVICE_TREE
> > -    struct dt_device_node *of_node; /* Used by drivers imported
> > from Linux */
> > -#endif
> > -};
> > -
> > -enum device_class
> > -{
> > -    DEVICE_SERIAL,
> > -    DEVICE_IOMMU,
> > -    DEVICE_PCI_HOSTBRIDGE,
> > -    /* Use for error */
> > -    DEVICE_UNKNOWN,
> > -};
> > -
> > -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);
> > -};
> > -
> > -typedef struct device device_t;
> > -
> > -#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                                               \
> > -};
> > -
> > -#endif /* __ASM_PPC_DEVICE_H__ */
> > diff --git a/xen/arch/arm/include/asm/device.h b/xen/include/asm-
> > generic/device.h
> > similarity index 79%
> > rename from xen/arch/arm/include/asm/device.h
> > rename to xen/include/asm-generic/device.h
> > index b5d451e087..063c448c4e 100644
> > --- a/xen/arch/arm/include/asm/device.h
> > +++ b/xen/include/asm-generic/device.h
> > @@ -1,14 +1,37 @@
> > -#ifndef __ASM_ARM_DEVICE_H
> > -#define __ASM_ARM_DEVICE_H
> > +/* 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
> > +
> > +#ifdef CONFIG_HAS_PCI
> >       DEV_PCI,
> > +#endif
> > +    DEV_TYPE_MAX,
> Nobody is using it. AFAICT, this was introduced because enum may be 
> empty if !HAS_DEVICE_TREE and !HAS_PCI. If so, can you add a comment 
> explaining it on top?
Sure, thanks. I'll add the comment.

> 
> The rest LGTM.
> 
> Cheers,
> 
~ Oleksii