Re: [PATCH v4 02/14] xen/asm-generic: introduce generic device.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-11-27 at 15:31 +0100, Jan Beulich wrote:
> On 27.11.2023 15:13, 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>
> > ---
> > 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 --------
> >  xen/arch/ppc/include/asm/irq.h                |   4 +
> >  .../asm => include/asm-generic}/device.h      | 125 +++++++++++---
> > ----
> >  xen/include/headers++.chk.new                 |   0
> >  11 files changed, 106 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%)
> >  create mode 100644 xen/include/headers++.chk.new
> 
> Stray new file, presumably because of a missing entry in .gitignore?
Yeah, I don't have such entry in .gitignore.
I will remove this file in next version of the patch.

> 
> 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.
> 
> > --- a/xen/arch/ppc/include/asm/irq.h
> > +++ b/xen/arch/ppc/include/asm/irq.h
> > @@ -3,7 +3,9 @@
> >  #define __ASM_PPC_IRQ_H__
> >  
> >  #include <xen/lib.h>
> > +#ifdef CONFIG_HAS_DEVICE_TREE
> >  #include <xen/device_tree.h>
> > +#endif
> >  #include <public/device_tree_defs.h>
> 
> Why would this #ifdef not cover the public header as well? (Otherwise
> I'd
> be inclined to ask that the conditional be moved inside that header.)
In that header is defined only consts without additional header
inclusion. At that moment it looked to me pretty save to ifdef only
xen/device_tree.h but you are right we can move incluion of the public
header inside #ifdef OR just remove as xen/device_tree.h already
includes it.

~ Oleksii