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: Mon, 2 Jul 2018 14:38:30 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.10.1807021432040.23811@sstabellini-ThinkPad-X260> (raw)
In-Reply-To: <5de55023-3bd7-0b30-7b36-637a590e5e10@arm.com>

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?

> So in the hypervisor case we want to:
> 	1) Define DT_* in a separate header
> 	2) Alias IRQ_TYPE_* to corresponding one.
> 
> In the libxl case we want to use the new header containing DT_*.

I understand what you are asking now, that is clear. I don't understand
why though.

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