Re: [PATCH v2 07/17] libxl/arm: Construct ACPI GTDT table

[Stefano Stabellini <sstabellini@kernel.org>]

On Wed, 6 Jul 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 05/07/16 17:42, Stefano Stabellini wrote:
> > On Mon, 27 Jun 2016, Julien Grall wrote:
> > > On 27/06/16 02:44, Shannon Zhao wrote:
> > > > On 2016/6/24 0:26, Julien Grall wrote:
> > > > > On 23/06/16 04:16, Shannon Zhao wrote:
> > > > > > From: Shannon Zhao <shannon.zhao@linaro.org>
> > > > > > 
> > > > > > Construct GTDT table with the interrupt information of timers.
> > > > > > 
> > > > > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > > > > > ---
> > > > > >     tools/libxl/libxl_arm_acpi.c | 28 ++++++++++++++++++++++++++++
> > > > > >     1 file changed, 28 insertions(+)
> > > > > > 
> > > > > > diff --git a/tools/libxl/libxl_arm_acpi.c
> > > > > > b/tools/libxl/libxl_arm_acpi.c
> > > > > > index d5ffedf..de863f4 100644
> > > > > > --- a/tools/libxl/libxl_arm_acpi.c
> > > > > > +++ b/tools/libxl/libxl_arm_acpi.c
> > > > > > @@ -39,6 +39,9 @@ typedef uint64_t u64;
> > > > > >     #define ACPI_BUILD_APPNAME6 "XenARM"
> > > > > >     #define ACPI_BUILD_APPNAME4 "Xen "
> > > > > > 
> > > > > > +#define ACPI_LEVEL_SENSITIVE            (u8) 0x00
> > > > > > +#define ACPI_ACTIVE_LOW                 (u8) 0x01
> > > > > > +
> > > > > 
> > > > > Why did not you include actypes.h rather than define these two
> > > > > defines?
> > > > If we include actypes.h, there will be some compiling errors.
> > > > 
> > > > ../../xen/include/acpi/actypes.h:55:2: error: #error ACPI_MACHINE_WIDTH
> > > > not defined
> > > >    #error ACPI_MACHINE_WIDTH not defined
> > > >     ^
> > > > ../../xen/include/acpi/actypes.h:130:9: error: unknown type name
> > > > 'COMPILER_DEPENDENT_UINT64'
> > > >    typedef COMPILER_DEPENDENT_UINT64 UINT64;
> > > >            ^
> > > > ../../xen/include/acpi/actypes.h:131:9: error: unknown type name
> > > > 'COMPILER_DEPENDENT_INT64'
> > > >    typedef COMPILER_DEPENDENT_INT64 INT64;
> > > >            ^
> > > > ../../xen/include/acpi/actypes.h:202:2: error: #error unknown
> > > > ACPI_MACHINE_WIDTH
> > > >    #error unknown ACPI_MACHINE_WIDTH
> > > >     ^
> > > > ../../xen/include/acpi/actypes.h:207:9: error: unknown type name
> > > > 'acpi_native_uint'
> > > >    typedef acpi_native_uint acpi_size;
> > > >            ^
> > > > ../../xen/include/acpi/actypes.h:617:3: error: unknown type name
> > > > 'acpi_io_address'
> > > >      acpi_io_address pblk_address;
> > > > 
> > > > Yeah, it maybe can be solved by defining ACPI_MACHINE_WIDTH and
> > > > COMPILER_DEPENDENT_INT64 here, but since we only needs
> > > > ACPI_LEVEL_SENSITIVE and ACPI_ACTIVE_LOW, I think it's ok to define them
> > > > here.
> > > 
> > > We should avoid to redefine value as much as possible. The 2 missing
> > > values
> > > are easy to define (see below) so there is no point to redefine in a less
> > > obvious way: no comment to explain what the values are for, and only a
> > > part of
> > > the set defined.
> > > 
> > > #define ACPI_MACHINE_WIDTH BITS_PER_LONG
> > > #define COMPILER_DEPENDENT_INT64 int64_t
> > > 
> > > Wei, Ian, Stefano, do you have any opinions?
> > 
> > I think you are right that we should avoid redefining
> > ACPI_LEVEL_SENSITIVE and ACPI_ACTIVE_LOW. But I think if possible we
> > should also avoid redefining ACPI_MACHINE_WIDTH and
> > COMPILER_DEPENDENT_INT64. If possible, we should include the header file
> > with those definitions too (acpi/platform/acenv.h ?).
> 
> acenv.h is including aclinux.h with contains a lot of hypervisor specific
> include. So we would need to rework the include to use it in the toolstack.
> 
> Or maybe this can be found in /usr/include/?

I was thinking of the one in the Xen tree. If it is not possible to
include it, then OK.

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