diff mbox

[RESEND,05/14] libxl/arm: Construct ACPI GTDT table

Message ID 1464670986-10256-6-git-send-email-zhaoshenglong@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shannon Zhao May 31, 2016, 5:02 a.m. UTC
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.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 74 insertions(+), 1 deletion(-)

Comments

Stefano Stabellini June 6, 2016, 11:40 a.m. UTC | #1
On Tue, 31 May 2016, 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.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 9e99159..0fb4f69 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -3,6 +3,7 @@
>  #include "libxl_libfdt_compat.h"
>  
>  #include <xc_dom.h>
> +#include <acpi_defs.h>
>  #include <stdbool.h>
>  #include <libfdt.h>
>  #include <assert.h>
> @@ -880,13 +881,85 @@ out:
>      return rc;
>  }
>  
> +static void make_acpi_header(struct acpi_table_header *h, const char *sig,
> +                             int len, uint8_t rev)
> +{
> +    memcpy(&h->signature, sig, 4);
> +    h->length = len;
> +    h->revision = rev;
> +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
> +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
> +    memcpy(h->oem_table_id + 4, sig, 4);
> +    h->oem_revision = 1;
> +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
> +    h->asl_compiler_revision = 1;
> +    h->checksum = 0;
> +}
> +
> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
> +{
> +    struct acpi_gtdt_descriptor *gtdt;
> +
> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
> +
> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
> +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
> +    gtdt->non_secure_el1_flags =
> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
> +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
> +    gtdt->virtual_timer_flags =
> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
> +
> +    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
> +
> +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
> +    /* Align to 64bit. */
> +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
> +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
> +}
> +
> +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
> +                        libxl__domain_build_state *state,
> +                        struct xc_dom_image *dom)
> +{
> +    const libxl_version_info *vers;
> +
> +    /* convenience aliases */
> +    xc_domain_configuration_t *xc_config = &state->config;
> +
> +    vers = libxl_get_version_info(CTX);
> +    if (vers == NULL)
> +        return ERROR_FAIL;
> +
> +    LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest",
> +        vers->xen_version_major, vers->xen_version_minor);
> +
> +    /* Alloc memory for ACPI blob placeholders. */
> +    dom->acpitable_blob = libxl__zalloc(gc, sizeof(struct acpitable_blob));
> +    dom->acpitable_size = 0;
> +
> +    make_acpi_gtdt(gc, dom);
> +
> +    return 0;
> +}
> +
>  int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>                                             libxl_domain_build_info *info,
>                                             libxl__domain_build_state *state,
>                                             struct xc_dom_image *dom)
>  {
> +    int rc;
> +
>      assert(info->type == LIBXL_DOMAIN_TYPE_PV);
> -    return prepare_dtb(gc, info, state, dom);
> +    rc = prepare_dtb(gc, info, state, dom);
> +    if (rc)
> +        return rc;
> +
> +    return prepare_acpi(gc, info, state, dom);
>  }

ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
config file enables them, with default off.
Julien Grall June 6, 2016, 11:52 a.m. UTC | #2
Hello,

On 06/06/16 12:40, Stefano Stabellini wrote:
> On Tue, 31 May 2016, 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.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 9e99159..0fb4f69 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -3,6 +3,7 @@
>>   #include "libxl_libfdt_compat.h"
>>
>>   #include <xc_dom.h>
>> +#include <acpi_defs.h>
>>   #include <stdbool.h>
>>   #include <libfdt.h>
>>   #include <assert.h>
>> @@ -880,13 +881,85 @@ out:
>>       return rc;
>>   }
>>
>> +static void make_acpi_header(struct acpi_table_header *h, const char *sig,
>> +                             int len, uint8_t rev)
>> +{
>> +    memcpy(&h->signature, sig, 4);
>> +    h->length = len;
>> +    h->revision = rev;
>> +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
>> +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
>> +    memcpy(h->oem_table_id + 4, sig, 4);
>> +    h->oem_revision = 1;
>> +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
>> +    h->asl_compiler_revision = 1;
>> +    h->checksum = 0;
>> +}
>> +
>> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
>> +{
>> +    struct acpi_gtdt_descriptor *gtdt;
>> +
>> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
>> +
>> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
>> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
>> +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
>> +    gtdt->non_secure_el1_flags =
>> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
>> +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
>> +    gtdt->virtual_timer_flags =
>> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
>> +
>> +    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
>> +
>> +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
>> +    /* Align to 64bit. */
>> +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
>> +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
>> +}
>> +
>> +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>> +                        libxl__domain_build_state *state,
>> +                        struct xc_dom_image *dom)
>> +{
>> +    const libxl_version_info *vers;
>> +
>> +    /* convenience aliases */
>> +    xc_domain_configuration_t *xc_config = &state->config;
>> +
>> +    vers = libxl_get_version_info(CTX);
>> +    if (vers == NULL)
>> +        return ERROR_FAIL;
>> +
>> +    LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest",
>> +        vers->xen_version_major, vers->xen_version_minor);
>> +
>> +    /* Alloc memory for ACPI blob placeholders. */
>> +    dom->acpitable_blob = libxl__zalloc(gc, sizeof(struct acpitable_blob));
>> +    dom->acpitable_size = 0;
>> +
>> +    make_acpi_gtdt(gc, dom);
>> +
>> +    return 0;
>> +}
>> +
>>   int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>>                                              libxl_domain_build_info *info,
>>                                              libxl__domain_build_state *state,
>>                                              struct xc_dom_image *dom)
>>   {
>> +    int rc;
>> +
>>       assert(info->type == LIBXL_DOMAIN_TYPE_PV);
>> -    return prepare_dtb(gc, info, state, dom);
>> +    rc = prepare_dtb(gc, info, state, dom);
>> +    if (rc)
>> +        return rc;
>> +
>> +    return prepare_acpi(gc, info, state, dom);
>>   }
>
> ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> config file enables them, with default off.

The VM system specification for ARM [1] mandates that both ACPI and DT 
should be provided and described the entire VM and its peripheral (see 
the section "Hardware Description").

Furthermore, the user may not know if the guest OS will use ACPI or DT 
For instance Redhat is using ACPI whilst Debian is using DT.

So we have to provide both by default. However, 32-bit domain should 
only have Device-Tree table created.

Anyway, the reason should have been described in the commit message. I 
would split this patch in two: introducing prepare ACPI and then GTDT 
table. So we can provide details in the commit message.

Regards,

[1] 
http://people.linaro.org/~christoffer.dall/VMSystemSpecificationForARM-v2.0-rc1.pdf
Stefano Stabellini June 6, 2016, 12:04 p.m. UTC | #3
On Mon, 6 Jun 2016, Julien Grall wrote:
> Hello,
> 
> On 06/06/16 12:40, Stefano Stabellini wrote:
> > On Tue, 31 May 2016, 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.c | 75
> > > ++++++++++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 74 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> > > index 9e99159..0fb4f69 100644
> > > --- a/tools/libxl/libxl_arm.c
> > > +++ b/tools/libxl/libxl_arm.c
> > > @@ -3,6 +3,7 @@
> > >   #include "libxl_libfdt_compat.h"
> > > 
> > >   #include <xc_dom.h>
> > > +#include <acpi_defs.h>
> > >   #include <stdbool.h>
> > >   #include <libfdt.h>
> > >   #include <assert.h>
> > > @@ -880,13 +881,85 @@ out:
> > >       return rc;
> > >   }
> > > 
> > > +static void make_acpi_header(struct acpi_table_header *h, const char
> > > *sig,
> > > +                             int len, uint8_t rev)
> > > +{
> > > +    memcpy(&h->signature, sig, 4);
> > > +    h->length = len;
> > > +    h->revision = rev;
> > > +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
> > > +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
> > > +    memcpy(h->oem_table_id + 4, sig, 4);
> > > +    h->oem_revision = 1;
> > > +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
> > > +    h->asl_compiler_revision = 1;
> > > +    h->checksum = 0;
> > > +}
> > > +
> > > +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
> > > +{
> > > +    struct acpi_gtdt_descriptor *gtdt;
> > > +
> > > +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
> > > +
> > > +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
> > > +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE <<
> > > ACPI_GTDT_INTERRUPT_MODE)
> > > +                             |(ACPI_ACTIVE_LOW <<
> > > ACPI_GTDT_INTERRUPT_POLARITY);
> > > +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
> > > +    gtdt->non_secure_el1_flags =
> > > +                             (ACPI_LEVEL_SENSITIVE <<
> > > ACPI_GTDT_INTERRUPT_MODE)
> > > +                             |(ACPI_ACTIVE_LOW <<
> > > ACPI_GTDT_INTERRUPT_POLARITY);
> > > +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
> > > +    gtdt->virtual_timer_flags =
> > > +                             (ACPI_LEVEL_SENSITIVE <<
> > > ACPI_GTDT_INTERRUPT_MODE)
> > > +                             |(ACPI_ACTIVE_LOW <<
> > > ACPI_GTDT_INTERRUPT_POLARITY);
> > > +
> > > +    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
> > > +
> > > +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
> > > +    /* Align to 64bit. */
> > > +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
> > > +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
> > > +}
> > > +
> > > +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
> > > +                        libxl__domain_build_state *state,
> > > +                        struct xc_dom_image *dom)
> > > +{
> > > +    const libxl_version_info *vers;
> > > +
> > > +    /* convenience aliases */
> > > +    xc_domain_configuration_t *xc_config = &state->config;
> > > +
> > > +    vers = libxl_get_version_info(CTX);
> > > +    if (vers == NULL)
> > > +        return ERROR_FAIL;
> > > +
> > > +    LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest",
> > > +        vers->xen_version_major, vers->xen_version_minor);
> > > +
> > > +    /* Alloc memory for ACPI blob placeholders. */
> > > +    dom->acpitable_blob = libxl__zalloc(gc, sizeof(struct
> > > acpitable_blob));
> > > +    dom->acpitable_size = 0;
> > > +
> > > +    make_acpi_gtdt(gc, dom);
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >   int libxl__arch_domain_init_hw_description(libxl__gc *gc,
> > >                                              libxl_domain_build_info
> > > *info,
> > >                                              libxl__domain_build_state
> > > *state,
> > >                                              struct xc_dom_image *dom)
> > >   {
> > > +    int rc;
> > > +
> > >       assert(info->type == LIBXL_DOMAIN_TYPE_PV);
> > > -    return prepare_dtb(gc, info, state, dom);
> > > +    rc = prepare_dtb(gc, info, state, dom);
> > > +    if (rc)
> > > +        return rc;
> > > +
> > > +    return prepare_acpi(gc, info, state, dom);
> > >   }
> > 
> > ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> > config file enables them, with default off.
> 
> The VM system specification for ARM [1] mandates that both ACPI and DT should
> be provided and described the entire VM and its peripheral (see the section
> "Hardware Description").
> 
> Furthermore, the user may not know if the guest OS will use ACPI or DT For
> instance Redhat is using ACPI whilst Debian is using DT.
> 
> So we have to provide both by default. However, 32-bit domain should only have
> Device-Tree table created.
> 
> Anyway, the reason should have been described in the commit message. I would
> split this patch in two: introducing prepare ACPI and then GTDT table. So we
> can provide details in the commit message.

All right, let me rephrase then: we should have a VMSPEC=on or off to
enable or disable compliance with the VM system specification for ARM.
(The good thing about specifications is that there are so many to choose
from.) With compliance disabled, we can avoid introducing ACPI tables
for the guest.

Given that "VMSPEC" is cumbersome, I suggest to introduce a simpler and
more meaningful alias: "ACPI" :-)

I am open to discussion on what the default should be, but there needs
to be a way to disable ACPI.
Wei Liu June 6, 2016, 12:18 p.m. UTC | #4
On Tue, May 31, 2016 at 01:02:57PM +0800, 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.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 74 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 9e99159..0fb4f69 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -3,6 +3,7 @@
>  #include "libxl_libfdt_compat.h"
>  
>  #include <xc_dom.h>
> +#include <acpi_defs.h>
>  #include <stdbool.h>
>  #include <libfdt.h>
>  #include <assert.h>
> @@ -880,13 +881,85 @@ out:
>      return rc;
>  }
>  
> +static void make_acpi_header(struct acpi_table_header *h, const char *sig,
> +                             int len, uint8_t rev)
> +{
> +    memcpy(&h->signature, sig, 4);
> +    h->length = len;
> +    h->revision = rev;
> +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
> +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
> +    memcpy(h->oem_table_id + 4, sig, 4);
> +    h->oem_revision = 1;
> +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
> +    h->asl_compiler_revision = 1;
> +    h->checksum = 0;
> +}
> +
> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
> +{
> +    struct acpi_gtdt_descriptor *gtdt;
> +
> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
> +
> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
> +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
> +    gtdt->non_secure_el1_flags =
> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
> +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
> +    gtdt->virtual_timer_flags =
> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
> +
> +    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
> +
> +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
> +    /* Align to 64bit. */
> +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
> +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
> +}
> +
> +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
> +                        libxl__domain_build_state *state,
> +                        struct xc_dom_image *dom)

libxl__prepare_acpi

I would also suggest moving ACPI related functions to a dedicated file
(libxl_arm_acpi.c) so that the possible future merger with x86 code can
be made easier.

Wei.
Shannon Zhao June 6, 2016, 2:31 p.m. UTC | #5
On 2016年06月06日 20:04, Stefano Stabellini wrote:
> On Mon, 6 Jun 2016, Julien Grall wrote:
>> > Hello,
>> > 
>> > On 06/06/16 12:40, Stefano Stabellini wrote:
>>> > > On Tue, 31 May 2016, 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.c | 75
>>>> > > > ++++++++++++++++++++++++++++++++++++++++++++++++-
>>>> > > >   1 file changed, 74 insertions(+), 1 deletion(-)
>>>> > > > 
>>>> > > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>>>> > > > index 9e99159..0fb4f69 100644
>>>> > > > --- a/tools/libxl/libxl_arm.c
>>>> > > > +++ b/tools/libxl/libxl_arm.c
>>>> > > > @@ -3,6 +3,7 @@
>>>> > > >   #include "libxl_libfdt_compat.h"
>>>> > > > 
>>>> > > >   #include <xc_dom.h>
>>>> > > > +#include <acpi_defs.h>
>>>> > > >   #include <stdbool.h>
>>>> > > >   #include <libfdt.h>
>>>> > > >   #include <assert.h>
>>>> > > > @@ -880,13 +881,85 @@ out:
>>>> > > >       return rc;
>>>> > > >   }
>>>> > > > 
>>>> > > > +static void make_acpi_header(struct acpi_table_header *h, const char
>>>> > > > *sig,
>>>> > > > +                             int len, uint8_t rev)
>>>> > > > +{
>>>> > > > +    memcpy(&h->signature, sig, 4);
>>>> > > > +    h->length = len;
>>>> > > > +    h->revision = rev;
>>>> > > > +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
>>>> > > > +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
>>>> > > > +    memcpy(h->oem_table_id + 4, sig, 4);
>>>> > > > +    h->oem_revision = 1;
>>>> > > > +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
>>>> > > > +    h->asl_compiler_revision = 1;
>>>> > > > +    h->checksum = 0;
>>>> > > > +}
>>>> > > > +
>>>> > > > +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
>>>> > > > +{
>>>> > > > +    struct acpi_gtdt_descriptor *gtdt;
>>>> > > > +
>>>> > > > +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
>>>> > > > +
>>>> > > > +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
>>>> > > > +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE <<
>>>> > > > ACPI_GTDT_INTERRUPT_MODE)
>>>> > > > +                             |(ACPI_ACTIVE_LOW <<
>>>> > > > ACPI_GTDT_INTERRUPT_POLARITY);
>>>> > > > +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
>>>> > > > +    gtdt->non_secure_el1_flags =
>>>> > > > +                             (ACPI_LEVEL_SENSITIVE <<
>>>> > > > ACPI_GTDT_INTERRUPT_MODE)
>>>> > > > +                             |(ACPI_ACTIVE_LOW <<
>>>> > > > ACPI_GTDT_INTERRUPT_POLARITY);
>>>> > > > +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
>>>> > > > +    gtdt->virtual_timer_flags =
>>>> > > > +                             (ACPI_LEVEL_SENSITIVE <<
>>>> > > > ACPI_GTDT_INTERRUPT_MODE)
>>>> > > > +                             |(ACPI_ACTIVE_LOW <<
>>>> > > > ACPI_GTDT_INTERRUPT_POLARITY);
>>>> > > > +
>>>> > > > +    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
>>>> > > > +
>>>> > > > +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
>>>> > > > +    /* Align to 64bit. */
>>>> > > > +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
>>>> > > > +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
>>>> > > > +}
>>>> > > > +
>>>> > > > +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>>>> > > > +                        libxl__domain_build_state *state,
>>>> > > > +                        struct xc_dom_image *dom)
>>>> > > > +{
>>>> > > > +    const libxl_version_info *vers;
>>>> > > > +
>>>> > > > +    /* convenience aliases */
>>>> > > > +    xc_domain_configuration_t *xc_config = &state->config;
>>>> > > > +
>>>> > > > +    vers = libxl_get_version_info(CTX);
>>>> > > > +    if (vers == NULL)
>>>> > > > +        return ERROR_FAIL;
>>>> > > > +
>>>> > > > +    LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest",
>>>> > > > +        vers->xen_version_major, vers->xen_version_minor);
>>>> > > > +
>>>> > > > +    /* Alloc memory for ACPI blob placeholders. */
>>>> > > > +    dom->acpitable_blob = libxl__zalloc(gc, sizeof(struct
>>>> > > > acpitable_blob));
>>>> > > > +    dom->acpitable_size = 0;
>>>> > > > +
>>>> > > > +    make_acpi_gtdt(gc, dom);
>>>> > > > +
>>>> > > > +    return 0;
>>>> > > > +}
>>>> > > > +
>>>> > > >   int libxl__arch_domain_init_hw_description(libxl__gc *gc,
>>>> > > >                                              libxl_domain_build_info
>>>> > > > *info,
>>>> > > >                                              libxl__domain_build_state
>>>> > > > *state,
>>>> > > >                                              struct xc_dom_image *dom)
>>>> > > >   {
>>>> > > > +    int rc;
>>>> > > > +
>>>> > > >       assert(info->type == LIBXL_DOMAIN_TYPE_PV);
>>>> > > > -    return prepare_dtb(gc, info, state, dom);
>>>> > > > +    rc = prepare_dtb(gc, info, state, dom);
>>>> > > > +    if (rc)
>>>> > > > +        return rc;
>>>> > > > +
>>>> > > > +    return prepare_acpi(gc, info, state, dom);
>>>> > > >   }
>>> > > 
>>> > > ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
>>> > > config file enables them, with default off.
>> > 
>> > The VM system specification for ARM [1] mandates that both ACPI and DT should
>> > be provided and described the entire VM and its peripheral (see the section
>> > "Hardware Description").
>> > 
>> > Furthermore, the user may not know if the guest OS will use ACPI or DT For
>> > instance Redhat is using ACPI whilst Debian is using DT.
>> > 
>> > So we have to provide both by default. However, 32-bit domain should only have
>> > Device-Tree table created.
>> > 
>> > Anyway, the reason should have been described in the commit message. I would
>> > split this patch in two: introducing prepare ACPI and then GTDT table. So we
>> > can provide details in the commit message.
> All right, let me rephrase then: we should have a VMSPEC=on or off to
> enable or disable compliance with the VM system specification for ARM.
> (The good thing about specifications is that there are so many to choose
> from.) With compliance disabled, we can avoid introducing ACPI tables
> for the guest.
> 
> Given that "VMSPEC" is cumbersome, I suggest to introduce a simpler and
> more meaningful alias: "ACPI" :-)
> 
> I am open to discussion on what the default should be, but there needs
> to be a way to disable ACPI.
I don't know why we need to disable ACPI because we can provide ACPI
tables but guest could choose to not use it. And for ARM32 domain, since
the linux guest kernel doesn't support ACPI, even we provide ACPI
tables, it can't use it, anyway.

Thanks,
Stefano Stabellini June 6, 2016, 2:50 p.m. UTC | #6
On Mon, 6 Jun 2016, Shannon Zhao wrote:
> On 2016年06月06日 20:04, Stefano Stabellini wrote:
> > On Mon, 6 Jun 2016, Julien Grall wrote:
> >> > Hello,
> >> > 
> >> > On 06/06/16 12:40, Stefano Stabellini wrote:
> >>> > > On Tue, 31 May 2016, 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.c | 75
> >>>> > > > ++++++++++++++++++++++++++++++++++++++++++++++++-
> >>>> > > >   1 file changed, 74 insertions(+), 1 deletion(-)
> >>>> > > > 
> >>>> > > > diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> >>>> > > > index 9e99159..0fb4f69 100644
> >>>> > > > --- a/tools/libxl/libxl_arm.c
> >>>> > > > +++ b/tools/libxl/libxl_arm.c
> >>>> > > > @@ -3,6 +3,7 @@
> >>>> > > >   #include "libxl_libfdt_compat.h"
> >>>> > > > 
> >>>> > > >   #include <xc_dom.h>
> >>>> > > > +#include <acpi_defs.h>
> >>>> > > >   #include <stdbool.h>
> >>>> > > >   #include <libfdt.h>
> >>>> > > >   #include <assert.h>
> >>>> > > > @@ -880,13 +881,85 @@ out:
> >>>> > > >       return rc;
> >>>> > > >   }
> >>>> > > > 
> >>>> > > > +static void make_acpi_header(struct acpi_table_header *h, const char
> >>>> > > > *sig,
> >>>> > > > +                             int len, uint8_t rev)
> >>>> > > > +{
> >>>> > > > +    memcpy(&h->signature, sig, 4);
> >>>> > > > +    h->length = len;
> >>>> > > > +    h->revision = rev;
> >>>> > > > +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
> >>>> > > > +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
> >>>> > > > +    memcpy(h->oem_table_id + 4, sig, 4);
> >>>> > > > +    h->oem_revision = 1;
> >>>> > > > +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
> >>>> > > > +    h->asl_compiler_revision = 1;
> >>>> > > > +    h->checksum = 0;
> >>>> > > > +}
> >>>> > > > +
> >>>> > > > +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
> >>>> > > > +{
> >>>> > > > +    struct acpi_gtdt_descriptor *gtdt;
> >>>> > > > +
> >>>> > > > +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
> >>>> > > > +
> >>>> > > > +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
> >>>> > > > +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE <<
> >>>> > > > ACPI_GTDT_INTERRUPT_MODE)
> >>>> > > > +                             |(ACPI_ACTIVE_LOW <<
> >>>> > > > ACPI_GTDT_INTERRUPT_POLARITY);
> >>>> > > > +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
> >>>> > > > +    gtdt->non_secure_el1_flags =
> >>>> > > > +                             (ACPI_LEVEL_SENSITIVE <<
> >>>> > > > ACPI_GTDT_INTERRUPT_MODE)
> >>>> > > > +                             |(ACPI_ACTIVE_LOW <<
> >>>> > > > ACPI_GTDT_INTERRUPT_POLARITY);
> >>>> > > > +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
> >>>> > > > +    gtdt->virtual_timer_flags =
> >>>> > > > +                             (ACPI_LEVEL_SENSITIVE <<
> >>>> > > > ACPI_GTDT_INTERRUPT_MODE)
> >>>> > > > +                             |(ACPI_ACTIVE_LOW <<
> >>>> > > > ACPI_GTDT_INTERRUPT_POLARITY);
> >>>> > > > +
> >>>> > > > +    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
> >>>> > > > +
> >>>> > > > +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
> >>>> > > > +    /* Align to 64bit. */
> >>>> > > > +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
> >>>> > > > +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
> >>>> > > > +}
> >>>> > > > +
> >>>> > > > +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
> >>>> > > > +                        libxl__domain_build_state *state,
> >>>> > > > +                        struct xc_dom_image *dom)
> >>>> > > > +{
> >>>> > > > +    const libxl_version_info *vers;
> >>>> > > > +
> >>>> > > > +    /* convenience aliases */
> >>>> > > > +    xc_domain_configuration_t *xc_config = &state->config;
> >>>> > > > +
> >>>> > > > +    vers = libxl_get_version_info(CTX);
> >>>> > > > +    if (vers == NULL)
> >>>> > > > +        return ERROR_FAIL;
> >>>> > > > +
> >>>> > > > +    LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest",
> >>>> > > > +        vers->xen_version_major, vers->xen_version_minor);
> >>>> > > > +
> >>>> > > > +    /* Alloc memory for ACPI blob placeholders. */
> >>>> > > > +    dom->acpitable_blob = libxl__zalloc(gc, sizeof(struct
> >>>> > > > acpitable_blob));
> >>>> > > > +    dom->acpitable_size = 0;
> >>>> > > > +
> >>>> > > > +    make_acpi_gtdt(gc, dom);
> >>>> > > > +
> >>>> > > > +    return 0;
> >>>> > > > +}
> >>>> > > > +
> >>>> > > >   int libxl__arch_domain_init_hw_description(libxl__gc *gc,
> >>>> > > >                                              libxl_domain_build_info
> >>>> > > > *info,
> >>>> > > >                                              libxl__domain_build_state
> >>>> > > > *state,
> >>>> > > >                                              struct xc_dom_image *dom)
> >>>> > > >   {
> >>>> > > > +    int rc;
> >>>> > > > +
> >>>> > > >       assert(info->type == LIBXL_DOMAIN_TYPE_PV);
> >>>> > > > -    return prepare_dtb(gc, info, state, dom);
> >>>> > > > +    rc = prepare_dtb(gc, info, state, dom);
> >>>> > > > +    if (rc)
> >>>> > > > +        return rc;
> >>>> > > > +
> >>>> > > > +    return prepare_acpi(gc, info, state, dom);
> >>>> > > >   }
> >>> > > 
> >>> > > ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> >>> > > config file enables them, with default off.
> >> > 
> >> > The VM system specification for ARM [1] mandates that both ACPI and DT should
> >> > be provided and described the entire VM and its peripheral (see the section
> >> > "Hardware Description").
> >> > 
> >> > Furthermore, the user may not know if the guest OS will use ACPI or DT For
> >> > instance Redhat is using ACPI whilst Debian is using DT.
> >> > 
> >> > So we have to provide both by default. However, 32-bit domain should only have
> >> > Device-Tree table created.
> >> > 
> >> > Anyway, the reason should have been described in the commit message. I would
> >> > split this patch in two: introducing prepare ACPI and then GTDT table. So we
> >> > can provide details in the commit message.
> > All right, let me rephrase then: we should have a VMSPEC=on or off to
> > enable or disable compliance with the VM system specification for ARM.
> > (The good thing about specifications is that there are so many to choose
> > from.) With compliance disabled, we can avoid introducing ACPI tables
> > for the guest.
> > 
> > Given that "VMSPEC" is cumbersome, I suggest to introduce a simpler and
> > more meaningful alias: "ACPI" :-)
> > 
> > I am open to discussion on what the default should be, but there needs
> > to be a way to disable ACPI.
> I don't know why we need to disable ACPI because we can provide ACPI
> tables but guest could choose to not use it. And for ARM32 domain, since
> the linux guest kernel doesn't support ACPI, even we provide ACPI
> tables, it can't use it, anyway.

Memory usage. Simplicity: if you know you are not going to use ACPI, you
might as well disable it to have one less moving piece (every line of
code is potential for a bug). Guest configuration: if your guest
operating system supports both ACPI and Device Tree and you want to be
sure that Device Tree is the one that gets used, then you can do it by
disabling ACPI at the VM level. Linux offers a command line option to do
that, but other OSes might not and could choose ACPI by default.
Debugging: it might be helpful to be able to enable/disable ACPI at the
VM level just to test different code paths in the guest. These are just
the first few reasons that come to mind.
Julien Grall June 6, 2016, 3:35 p.m. UTC | #7
Hello,

On 06/06/16 13:04, Stefano Stabellini wrote:
> On Mon, 6 Jun 2016, Julien Grall wrote:
>> On 06/06/16 12:40, Stefano Stabellini wrote:
>>> On Tue, 31 May 2016, Shannon Zhao wrote:
>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>
>>> ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
>>> config file enables them, with default off.
>>
>> The VM system specification for ARM [1] mandates that both ACPI and DT should
>> be provided and described the entire VM and its peripheral (see the section
>> "Hardware Description").
>>
>> Furthermore, the user may not know if the guest OS will use ACPI or DT For
>> instance Redhat is using ACPI whilst Debian is using DT.
>>
>> So we have to provide both by default. However, 32-bit domain should only have
>> Device-Tree table created.
>>
>> Anyway, the reason should have been described in the commit message. I would
>> split this patch in two: introducing prepare ACPI and then GTDT table. So we
>> can provide details in the commit message.
>
> All right, let me rephrase then: we should have a VMSPEC=on or off to
> enable or disable compliance with the VM system specification for ARM.
> (The good thing about specifications is that there are so many to choose
> from.) With compliance disabled, we can avoid introducing ACPI tables
> for the guest.
>
> Given that "VMSPEC" is cumbersome, I suggest to introduce a simpler and
> more meaningful alias: "ACPI" :-)

The VM specification introduces other components such as a SBSA UART 
emulation (which is not yet implemented by Xen).

Do we want an option for each components?

>
> I am open to discussion on what the default should be, but there needs
> to be a way to disable ACPI.

Agree with that.
Julien Grall June 6, 2016, 3:42 p.m. UTC | #8
Hi Stefano,

On 06/06/16 15:50, Stefano Stabellini wrote:
> On Mon, 6 Jun 2016, Shannon Zhao wrote:
>> I don't know why we need to disable ACPI because we can provide ACPI
>> tables but guest could choose to not use it. And for ARM32 domain, since
>> the linux guest kernel doesn't support ACPI, even we provide ACPI
>> tables, it can't use it, anyway.
>
> Memory usage. Simplicity: if you know you are not going to use ACPI, you
> might as well disable it to have one less moving piece (every line of
> code is potential for a bug). Guest configuration: if your guest
> operating system supports both ACPI and Device Tree and you want to be
> sure that Device Tree is the one that gets used, then you can do it by
> disabling ACPI at the VM level. Linux offers a command line option to do
> that, but other OSes might not and could choose ACPI by default.

Other OSes would have the same problem on baremetal. A VM should 
reproduce the baremetal behavior. If you have enabled both ACPI and DT 
in your kernel, then you greed to let the kernel choose for you.

However, I agree that an having option to enable/disable ACPI is useful. 
The main use case would be embedded environment where ACPI will less used.

Regards,
Stefano Stabellini June 7, 2016, 9:41 a.m. UTC | #9
On Mon, 6 Jun 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 06/06/16 15:50, Stefano Stabellini wrote:
> > On Mon, 6 Jun 2016, Shannon Zhao wrote:
> > > I don't know why we need to disable ACPI because we can provide ACPI
> > > tables but guest could choose to not use it. And for ARM32 domain, since
> > > the linux guest kernel doesn't support ACPI, even we provide ACPI
> > > tables, it can't use it, anyway.
> > 
> > Memory usage. Simplicity: if you know you are not going to use ACPI, you
> > might as well disable it to have one less moving piece (every line of
> > code is potential for a bug). Guest configuration: if your guest
> > operating system supports both ACPI and Device Tree and you want to be
> > sure that Device Tree is the one that gets used, then you can do it by
> > disabling ACPI at the VM level. Linux offers a command line option to do
> > that, but other OSes might not and could choose ACPI by default.
> 
> Other OSes would have the same problem on baremetal. A VM should reproduce the
> baremetal behavior. If you have enabled both ACPI and DT in your kernel, then
> you greed to let the kernel choose for you.
> 
> However, I agree that an having option to enable/disable ACPI is useful. The
> main use case would be embedded environment where ACPI will less used.

That's right.
Stefano Stabellini June 7, 2016, 9:48 a.m. UTC | #10
On Mon, 6 Jun 2016, Julien Grall wrote:
> Hello,
> 
> On 06/06/16 13:04, Stefano Stabellini wrote:
> > On Mon, 6 Jun 2016, Julien Grall wrote:
> > > On 06/06/16 12:40, Stefano Stabellini wrote:
> > > > On Tue, 31 May 2016, Shannon Zhao wrote:
> > > > > From: Shannon Zhao <shannon.zhao@linaro.org>
> > > > 
> > > > ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> > > > config file enables them, with default off.
> > > 
> > > The VM system specification for ARM [1] mandates that both ACPI and DT
> > > should
> > > be provided and described the entire VM and its peripheral (see the
> > > section
> > > "Hardware Description").
> > > 
> > > Furthermore, the user may not know if the guest OS will use ACPI or DT For
> > > instance Redhat is using ACPI whilst Debian is using DT.
> > > 
> > > So we have to provide both by default. However, 32-bit domain should only
> > > have
> > > Device-Tree table created.
> > > 
> > > Anyway, the reason should have been described in the commit message. I
> > > would
> > > split this patch in two: introducing prepare ACPI and then GTDT table. So
> > > we
> > > can provide details in the commit message.
> > 
> > All right, let me rephrase then: we should have a VMSPEC=on or off to
> > enable or disable compliance with the VM system specification for ARM.
> > (The good thing about specifications is that there are so many to choose
> > from.) With compliance disabled, we can avoid introducing ACPI tables
> > for the guest.
> > 
> > Given that "VMSPEC" is cumbersome, I suggest to introduce a simpler and
> > more meaningful alias: "ACPI" :-)
> 
> The VM specification introduces other components such as a SBSA UART emulation
> (which is not yet implemented by Xen).
> 
> Do we want an option for each components?

This is a good point. If one wants to avoid ACPI then she probably would
want to avoid SBSA UART emulation too. So maybe after all it might be
better to have a single

vm_system_spec=1/0

option? I am OK with both having multiple options or just one.
Julien Grall June 7, 2016, 11:02 a.m. UTC | #11
Hello,

On 06/06/16 13:18, Wei Liu wrote:
> On Tue, May 31, 2016 at 01:02:57PM +0800, Shannon Zhao wrote:
>> +static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>> +                        libxl__domain_build_state *state,
>> +                        struct xc_dom_image *dom)
>
> libxl__prepare_acpi
>
> I would also suggest moving ACPI related functions to a dedicated file
> (libxl_arm_acpi.c) so that the possible future merger with x86 code can
> be made easier.

+1 and it makes sense to have the code to build the Device Tree and the 
ACPI tables in a separate files.

Regards,
Julien Grall June 7, 2016, 11:19 a.m. UTC | #12
Hello Shannon,

On 31/05/16 06:02, Shannon Zhao wrote:
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 9e99159..0fb4f69 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -3,6 +3,7 @@
>   #include "libxl_libfdt_compat.h"
>
>   #include <xc_dom.h>
> +#include <acpi_defs.h>
>   #include <stdbool.h>
>   #include <libfdt.h>
>   #include <assert.h>
> @@ -880,13 +881,85 @@ out:
>       return rc;
>   }
>
> +static void make_acpi_header(struct acpi_table_header *h, const char *sig,
> +                             int len, uint8_t rev)
> +{
> +    memcpy(&h->signature, sig, 4);
> +    h->length = len;
> +    h->revision = rev;
> +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
> +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
> +    memcpy(h->oem_table_id + 4, sig, 4);
> +    h->oem_revision = 1;
> +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
> +    h->asl_compiler_revision = 1;
> +    h->checksum = 0;

Should not we compute the checksum here?

> +}
> +
> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
> +{
> +    struct acpi_gtdt_descriptor *gtdt;
> +
> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
> +
> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);

There is no secure EL1 for guest, so this should be 0.

> +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
> +    gtdt->non_secure_el1_flags =
> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
> +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
> +    gtdt->virtual_timer_flags =
> +                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
> +                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
> +
> +    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
> +
> +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
> +    /* Align to 64bit. */

I am not sure what the comment is for.

> +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
> +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
> +}

Regards,
Shannon Zhao June 7, 2016, 11:30 a.m. UTC | #13
On 2016/6/7 19:19, Julien Grall wrote:
> Hello Shannon,
> 
> On 31/05/16 06:02, Shannon Zhao wrote:
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 9e99159..0fb4f69 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -3,6 +3,7 @@
>>   #include "libxl_libfdt_compat.h"
>>
>>   #include <xc_dom.h>
>> +#include <acpi_defs.h>
>>   #include <stdbool.h>
>>   #include <libfdt.h>
>>   #include <assert.h>
>> @@ -880,13 +881,85 @@ out:
>>       return rc;
>>   }
>>
>> +static void make_acpi_header(struct acpi_table_header *h, const char
>> *sig,
>> +                             int len, uint8_t rev)
>> +{
>> +    memcpy(&h->signature, sig, 4);
>> +    h->length = len;
>> +    h->revision = rev;
>> +    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
>> +    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
>> +    memcpy(h->oem_table_id + 4, sig, 4);
>> +    h->oem_revision = 1;
>> +    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
>> +    h->asl_compiler_revision = 1;
>> +    h->checksum = 0;
> 
> Should not we compute the checksum here?
> 
Sure.
>> +}
>> +
>> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
>> +{
>> +    struct acpi_gtdt_descriptor *gtdt;
>> +
>> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
>> +
>> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
>> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE <<
>> ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW <<
>> ACPI_GTDT_INTERRUPT_POLARITY);
> 
> There is no secure EL1 for guest, so this should be 0.
> 
So why does DT add secure EL1 timer in make_timer_node()?

>> +    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
>> +    gtdt->non_secure_el1_flags =
>> +                             (ACPI_LEVEL_SENSITIVE <<
>> ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW <<
>> ACPI_GTDT_INTERRUPT_POLARITY);
>> +    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
>> +    gtdt->virtual_timer_flags =
>> +                             (ACPI_LEVEL_SENSITIVE <<
>> ACPI_GTDT_INTERRUPT_MODE)
>> +                             |(ACPI_ACTIVE_LOW <<
>> ACPI_GTDT_INTERRUPT_POLARITY);
>> +
>> +    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
>> +
>> +    dom->acpitable_blob->gtdt.table = (void *)gtdt;
>> +    /* Align to 64bit. */
> 
> I am not sure what the comment is for.
> 
Oops, I planed to make the length of these tables aligned to 64bit, but
it turns out it's unnecessary. And I forgot to delete this.

>> +    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
>> +    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
>> +}
> 
> Regards,
>
Julien Grall June 7, 2016, 11:36 a.m. UTC | #14
On 07/06/16 12:30, Shannon Zhao wrote:
> On 2016/6/7 19:19, Julien Grall wrote:
>> On 31/05/16 06:02, Shannon Zhao wrote:
>>> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
>>> +{
>>> +    struct acpi_gtdt_descriptor *gtdt;
>>> +
>>> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
>>> +
>>> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
>>> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE <<
>>> ACPI_GTDT_INTERRUPT_MODE)
>>> +                             |(ACPI_ACTIVE_LOW <<
>>> ACPI_GTDT_INTERRUPT_POLARITY);
>>
>> There is no secure EL1 for guest, so this should be 0.
>>
> So why does DT add secure EL1 timer in make_timer_node()?

Because the DT binding mandates the secure EL1 IRQ. However this 
interrupt will never be asserted by the virtual timer.

Regards,
Shannon Zhao June 7, 2016, 11:39 a.m. UTC | #15
On 2016/6/7 19:36, Julien Grall wrote:
> 
> 
> On 07/06/16 12:30, Shannon Zhao wrote:
>> On 2016/6/7 19:19, Julien Grall wrote:
>>> On 31/05/16 06:02, Shannon Zhao wrote:
>>>> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
>>>> +{
>>>> +    struct acpi_gtdt_descriptor *gtdt;
>>>> +
>>>> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
>>>> +
>>>> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
>>>> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE <<
>>>> ACPI_GTDT_INTERRUPT_MODE)
>>>> +                             |(ACPI_ACTIVE_LOW <<
>>>> ACPI_GTDT_INTERRUPT_POLARITY);
>>>
>>> There is no secure EL1 for guest, so this should be 0.
>>>
>> So why does DT add secure EL1 timer in make_timer_node()?
> 
> Because the DT binding mandates the secure EL1 IRQ. However this
> interrupt will never be asserted by the virtual timer.
Oh, right. But it's harmless. Anyway, I'll remove this.
Julien Grall June 7, 2016, 11:43 a.m. UTC | #16
On 07/06/16 12:39, Shannon Zhao wrote:
>
>
> On 2016/6/7 19:36, Julien Grall wrote:
>>
>>
>> On 07/06/16 12:30, Shannon Zhao wrote:
>>> On 2016/6/7 19:19, Julien Grall wrote:
>>>> On 31/05/16 06:02, Shannon Zhao wrote:
>>>>> +static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
>>>>> +{
>>>>> +    struct acpi_gtdt_descriptor *gtdt;
>>>>> +
>>>>> +    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
>>>>> +
>>>>> +    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
>>>>> +    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE <<
>>>>> ACPI_GTDT_INTERRUPT_MODE)
>>>>> +                             |(ACPI_ACTIVE_LOW <<
>>>>> ACPI_GTDT_INTERRUPT_POLARITY);
>>>>
>>>> There is no secure EL1 for guest, so this should be 0.
>>>>
>>> So why does DT add secure EL1 timer in make_timer_node()?
>>
>> Because the DT binding mandates the secure EL1 IRQ. However this
>> interrupt will never be asserted by the virtual timer.
> Oh, right. But it's harmless. Anyway, I'll remove this.

Correct, but let's not expose things a guest could not use :).

Regards,
Wei Liu June 7, 2016, 11:55 a.m. UTC | #17
On Tue, Jun 07, 2016 at 10:48:36AM +0100, Stefano Stabellini wrote:
> On Mon, 6 Jun 2016, Julien Grall wrote:
> > Hello,
> > 
> > On 06/06/16 13:04, Stefano Stabellini wrote:
> > > On Mon, 6 Jun 2016, Julien Grall wrote:
> > > > On 06/06/16 12:40, Stefano Stabellini wrote:
> > > > > On Tue, 31 May 2016, Shannon Zhao wrote:
> > > > > > From: Shannon Zhao <shannon.zhao@linaro.org>
> > > > > 
> > > > > ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> > > > > config file enables them, with default off.
> > > > 
> > > > The VM system specification for ARM [1] mandates that both ACPI and DT
> > > > should
> > > > be provided and described the entire VM and its peripheral (see the
> > > > section
> > > > "Hardware Description").
> > > > 
> > > > Furthermore, the user may not know if the guest OS will use ACPI or DT For
> > > > instance Redhat is using ACPI whilst Debian is using DT.
> > > > 
> > > > So we have to provide both by default. However, 32-bit domain should only
> > > > have
> > > > Device-Tree table created.
> > > > 
> > > > Anyway, the reason should have been described in the commit message. I
> > > > would
> > > > split this patch in two: introducing prepare ACPI and then GTDT table. So
> > > > we
> > > > can provide details in the commit message.
> > > 
> > > All right, let me rephrase then: we should have a VMSPEC=on or off to
> > > enable or disable compliance with the VM system specification for ARM.
> > > (The good thing about specifications is that there are so many to choose
> > > from.) With compliance disabled, we can avoid introducing ACPI tables
> > > for the guest.
> > > 
> > > Given that "VMSPEC" is cumbersome, I suggest to introduce a simpler and
> > > more meaningful alias: "ACPI" :-)
> > 
> > The VM specification introduces other components such as a SBSA UART emulation
> > (which is not yet implemented by Xen).
> > 
> > Do we want an option for each components?
> 
> This is a good point. If one wants to avoid ACPI then she probably would
> want to avoid SBSA UART emulation too. So maybe after all it might be
> better to have a single
> 
> vm_system_spec=1/0
> 
> option? I am OK with both having multiple options or just one.

If the user is not supposed to pick and choose then using one is fine.
We probably don't want to expose such capability anyway.

Is the spec going to be updated at some point? Maybe make it some sort
of version number?

Wei.
Shannon Zhao June 17, 2016, 3:29 a.m. UTC | #18
On 2016/6/6 19:40, Stefano Stabellini wrote:
> ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> config file enables them, with default off.
While the configuration "acpi" already exists for HVM guest
configuration, do we plan to reuse it or use a new name, e.g. arm_acpi?

Thanks,
Julien Grall June 17, 2016, 8:17 a.m. UTC | #19
Hello Shannon,

On 17/06/16 04:29, Shannon Zhao wrote:
> On 2016/6/6 19:40, Stefano Stabellini wrote:
>> ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
>> config file enables them, with default off.
> While the configuration "acpi" already exists for HVM guest
> configuration, do we plan to reuse it or use a new name, e.g. arm_acpi?

We always try to re-use options unless their meaning are completely 
different.

Regards,
Stefano Stabellini June 17, 2016, 9:15 a.m. UTC | #20
On Fri, 17 Jun 2016, Julien Grall wrote:
> Hello Shannon,
> 
> On 17/06/16 04:29, Shannon Zhao wrote:
> > On 2016/6/6 19:40, Stefano Stabellini wrote:
> > > ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
> > > config file enables them, with default off.
> > While the configuration "acpi" already exists for HVM guest
> > configuration, do we plan to reuse it or use a new name, e.g. arm_acpi?
> 
> We always try to re-use options unless their meaning are completely different.
 
I agree. "acpi" is a good name for it. I would avoid introducing
something like "arm_acpi".
Julien Grall June 17, 2016, 1:27 p.m. UTC | #21
On 17/06/16 10:15, Stefano Stabellini wrote:
> On Fri, 17 Jun 2016, Julien Grall wrote:
>> Hello Shannon,
>>
>> On 17/06/16 04:29, Shannon Zhao wrote:
>>> On 2016/6/6 19:40, Stefano Stabellini wrote:
>>>> ACPI tables for ARM guests should be user configurable: acpi=1 in the VM
>>>> config file enables them, with default off.
>>> While the configuration "acpi" already exists for HVM guest
>>> configuration, do we plan to reuse it or use a new name, e.g. arm_acpi?
>>
>> We always try to re-use options unless their meaning are completely different.
>
> I agree. "acpi" is a good name for it. I would avoid introducing
> something like "arm_acpi".

BTW, this value should be default false for at least 32-bit domain.

I am not sure if we agreed what should be the default for 64-bit domain.

Cheers,
diff mbox

Patch

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 9e99159..0fb4f69 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -3,6 +3,7 @@ 
 #include "libxl_libfdt_compat.h"
 
 #include <xc_dom.h>
+#include <acpi_defs.h>
 #include <stdbool.h>
 #include <libfdt.h>
 #include <assert.h>
@@ -880,13 +881,85 @@  out:
     return rc;
 }
 
+static void make_acpi_header(struct acpi_table_header *h, const char *sig,
+                             int len, uint8_t rev)
+{
+    memcpy(&h->signature, sig, 4);
+    h->length = len;
+    h->revision = rev;
+    memcpy(h->oem_id, ACPI_BUILD_APPNAME6, 6);
+    memcpy(h->oem_table_id, ACPI_BUILD_APPNAME4, 4);
+    memcpy(h->oem_table_id + 4, sig, 4);
+    h->oem_revision = 1;
+    memcpy(h->asl_compiler_id, ACPI_BUILD_APPNAME4, 4);
+    h->asl_compiler_revision = 1;
+    h->checksum = 0;
+}
+
+static void make_acpi_gtdt(libxl__gc *gc, struct xc_dom_image *dom)
+{
+    struct acpi_gtdt_descriptor *gtdt;
+
+    gtdt = libxl__zalloc(gc, sizeof(*gtdt));
+
+    gtdt->secure_el1_interrupt = GUEST_TIMER_PHYS_S_PPI;
+    gtdt->secure_el1_flags = (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
+                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
+    gtdt->non_secure_el1_interrupt = GUEST_TIMER_PHYS_NS_PPI;
+    gtdt->non_secure_el1_flags =
+                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
+                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
+    gtdt->virtual_timer_interrupt = GUEST_TIMER_VIRT_PPI;
+    gtdt->virtual_timer_flags =
+                             (ACPI_LEVEL_SENSITIVE << ACPI_GTDT_INTERRUPT_MODE)
+                             |(ACPI_ACTIVE_LOW << ACPI_GTDT_INTERRUPT_POLARITY);
+
+    make_acpi_header(&gtdt->header, "GTDT", sizeof(*gtdt), 2);
+
+    dom->acpitable_blob->gtdt.table = (void *)gtdt;
+    /* Align to 64bit. */
+    dom->acpitable_blob->gtdt.size = sizeof(*gtdt);
+    dom->acpitable_size += dom->acpitable_blob->gtdt.size;
+}
+
+static int prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
+                        libxl__domain_build_state *state,
+                        struct xc_dom_image *dom)
+{
+    const libxl_version_info *vers;
+
+    /* convenience aliases */
+    xc_domain_configuration_t *xc_config = &state->config;
+
+    vers = libxl_get_version_info(CTX);
+    if (vers == NULL)
+        return ERROR_FAIL;
+
+    LOG(DEBUG, "constructing ACPI tables for Xen version %d.%d guest",
+        vers->xen_version_major, vers->xen_version_minor);
+
+    /* Alloc memory for ACPI blob placeholders. */
+    dom->acpitable_blob = libxl__zalloc(gc, sizeof(struct acpitable_blob));
+    dom->acpitable_size = 0;
+
+    make_acpi_gtdt(gc, dom);
+
+    return 0;
+}
+
 int libxl__arch_domain_init_hw_description(libxl__gc *gc,
                                            libxl_domain_build_info *info,
                                            libxl__domain_build_state *state,
                                            struct xc_dom_image *dom)
 {
+    int rc;
+
     assert(info->type == LIBXL_DOMAIN_TYPE_PV);
-    return prepare_dtb(gc, info, state, dom);
+    rc = prepare_dtb(gc, info, state, dom);
+    if (rc)
+        return rc;
+
+    return prepare_acpi(gc, info, state, dom);
 }
 
 static void finalise_one_memory_node(libxl__gc *gc, void *fdt,