Re: [PATCH RFC 02/15] xen/arm: move a few guest related #defines to public/arch-arm.h

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien.grall@arm.com>
Cc: artem_mygaiev@epam.com, Stefano Stabellini <stefanos@xilinx.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	andrii_anisov@epam.com, ian.jackson@eu.citrix.com,
	xen-devel@lists.xen.org, wei.liu2@citrix.com, nd@arm.com
Subject: Re: [PATCH RFC 02/15] xen/arm: move a few guest related #defines to public/arch-arm.h
Date: Tue, 3 Jul 2018 14:30:02 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.10.1807031413570.23811@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <051d1ee3-7fbf-ae0b-eb4d-50cd2a59e7fd@arm.com>

On Tue, 3 Jul 2018, Julien Grall wrote:
> Hi,
> 
> On 02/07/18 22:38, Stefano Stabellini wrote:
> > On Mon, 2 Jul 2018, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 02/07/2018 21:37, Stefano Stabellini wrote:
> > > > On Fri, 15 Jun 2018, Julien Grall wrote:
> > > > > Hi Stefano,
> > > > > 
> > > > > On 06/14/2018 10:15 PM, Stefano Stabellini wrote:
> > > > > > On Thu, 14 Jun 2018, Julien Grall wrote:
> > > > > > > On 13/06/18 23:15, Stefano Stabellini wrote:
> > > > > > > > Move a few constants defined by libxl_arm.c to
> > > > > > > > xen/include/public/arch-arm.h, so that they are together with
> > > > > > > > the
> > > > > > > > other
> > > > > > > > guest related #defines such as GUEST_GICD_BASE and
> > > > > > > > GUEST_VPL011_SPI.
> > > > > > > > Also, this way they can be reused by hypervisor code.
> > > > > > > 
> > > > > > > All variables moved to arch-arm.h should be prefixed with GUEST_*
> > > > > > > to
> > > > > > > avoid
> > > > > > > clash with the rest of Xen.
> > > > > > 
> > > > > > I'll do.
> > > > > > 
> > > > > > > > 
> > > > > > > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com>
> > > > > > > > CC: wei.liu2@citrix.com
> > > > > > > > CC: ian.jackson@eu.citrix.com
> > > > > > > > ---
> > > > > > > >      tools/libxl/libxl_arm.c       | 26
> > > > > > > > --------------------------
> > > > > > > >      xen/include/public/arch-arm.h | 26
> > > > > > > > ++++++++++++++++++++++++++
> > > > > > > >      2 files changed, 26 insertions(+), 26 deletions(-)
> > > > > > > > 
> > > > > > > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> > > > > > > > index 8af9f6f..89a417f 100644
> > > > > > > > --- a/tools/libxl/libxl_arm.c
> > > > > > > > +++ b/tools/libxl/libxl_arm.c
> > > > > > > > @@ -8,23 +8,6 @@
> > > > > > > >      #include <libfdt.h>
> > > > > > > >      #include <assert.h>
> > > > > > > >      -/**
> > > > > > > > - * IRQ line type.
> > > > > > > > - * DT_IRQ_TYPE_NONE            - default, unspecified type
> > > > > > > > - * DT_IRQ_TYPE_EDGE_RISING     - rising edge triggered
> > > > > > > > - * DT_IRQ_TYPE_EDGE_FALLING    - falling edge triggered
> > > > > > > > - * DT_IRQ_TYPE_EDGE_BOTH       - rising and falling edge
> > > > > > > > triggered
> > > > > > > > - * DT_IRQ_TYPE_LEVEL_HIGH      - high level triggered
> > > > > > > > - * DT_IRQ_TYPE_LEVEL_LOW       - low level triggered
> > > > > > > > - */
> > > > > > > > -#define DT_IRQ_TYPE_NONE           0x00000000
> > > > > > > > -#define DT_IRQ_TYPE_EDGE_RISING    0x00000001
> > > > > > > > -#define DT_IRQ_TYPE_EDGE_FALLING   0x00000002
> > > > > > > > -#define DT_IRQ_TYPE_EDGE_BOTH                           \
> > > > > > > > -    (DT_IRQ_TYPE_EDGE_FALLING | DT_IRQ_TYPE_EDGE_RISING)
> > > > > > > > -#define DT_IRQ_TYPE_LEVEL_HIGH     0x00000004
> > > > > > > > -#define DT_IRQ_TYPE_LEVEL_LOW      0x00000008
> > > > > > > > -
> > > > > > > 
> > > > > > > Those defines have nothing to do with the guest itself. They are
> > > > > > > currently
> > > > > > > define in Xen without the DT_ prefix.
> > > > > > 
> > > > > > Sounds like we want to get rid of the DT_IRQ_TYPE_* definitions
> > > > > > completely, move the IRQ_TYPE_* definitions from device_tree.h to
> > > > > > here,
> > > > > > and start using them in tools/libxl/libxl_arm.c (which involves a
> > > > > > renaming s/DT_IRQ_TYPE/IRQ_TYPE/g).
> > > > > > 
> > > > > > Is that what you had in mind?
> > > > > 
> > > > > Even if DT is Arm only today, the DT code is in common code and
> > > > > therefore
> > > > > header device_tree.h should contain every thing necessary to use a DT.
> > > > > 
> > > > > If we still want to share constant with libxl then I would prefer to
> > > > > introduce
> > > > > a new header (similar to acpi/acconfig.h) that provide all the common
> > > > > values.
> > > > 
> > > > OK, I can do that. I'll introduce a new header file.
> > > > 
> > > > 
> > > > > Note that the hypervisor one don't have the DT_ prefix because they
> > > > > are
> > > > > use to
> > > > > describe IRQ for both DT and ACPI in Xen. It is not that nice, we
> > > > > might
> > > > > want
> > > > > to introduce aliases in that case. So we keep DT_* in libxl.
> > > > 
> > > > With the new header file we'll be able to reuse the same #defines in
> > > > libxl and xen. I think it would be nicer to avoid the aliases and just
> > > > use the regular definitions in libxl too? Changing libxl to use
> > > > IRQ_TYPE_ directly only requires a small patch.
> > > 
> > > I really don't want that. As I explained, IRQ_TYPE have been conveniently
> > > chosen to avoid converting DT value to IRQ_TYPE. They are not meant to be
> > > use
> > > like that libxl (or any header publicly shared) and will provide more
> > > confusion that anything for other bits than the hypervisor.
> > 
> > I don't follow the explanation. Why would they be confusing in public
> > headers? What is the important difference between DT_IRQ_TYPE_* and
> > IRQ_TYPE_* that we are trying to keep? Why IRQ_TYPE_* shouldn't be used
> > by libxl?
> 
> Because IRQ_TYPE_* does not have the prefix DT_ in them. There are no way from
> the name to say this is for DT only and might be misused outside of the
> hypervisor.
> 
> IRQ_TYPE_* used to have the prefix DT_ but it was dropped as in the hypervisor
> they are used to represent irq type for both ACPI and DT. If we end up to
> expose those values to libxl then we should define DT_IRQ_TYPE_* and provide
> aliases for IRQ_TYPE_.

The irq types defined as IRQ_TYPE_* are not just used in device tree
code but also in ACPI. This is the reason why they were originally
renamed from DT_IRQ_TYPE_* to IRQ_TYPE_.

Libxl is also carrying a copy of the same definition as DT_IRQ_TYPE_*.

I am suggesting to remove the code duplication and use the same
IRQ_TYPE_* #defines everywhere: DT in Xen, ACPI in Xen and DT in libxl.

Your suggestion is to use DT_IRQ_TYPE_* for DT in libxl and introduce
IRQ_TYPE_* aliases for DT and ACPI in Xen. The reason is that they are
meant for device tree usage so it should be clear that they are for
device tree. (I hope I got it right.)

The two goals are not mutually exclusive. I don't care about what is
called IRQ_TYPE_* and what is called DT_IRQ_TYPE_*, I am just trying to
use the same defines in libxl DT and Xen DT code. I think those two
should be the same, whether they are named DT_IRQ_TYPE_* or simply
IRQ_TYPE_*. For that, I could introduce a new header with DT_IRQ_TYPE_*
and use it both in Xen and Libxl. I think that would satisfy your
requirements?

It remains to be seen what to do for the ACPI defines. I would probably
just use the DT_* defines in the ACPI code, given that now the
DT_IRQ_TYPE_* defines become available outside of device_tree.h.
Another option is to introduce ACPI specific aliases, such as
ACPI_IRQ_TYPE_*. I don't particularly care.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel