diff mbox

[v4,05/16] libxl/arm: Construct ACPI RSDP table

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

Commit Message

Shannon Zhao Aug. 16, 2016, 10:25 a.m. UTC
From: Shannon Zhao <shannon.zhao@linaro.org>

Construct ACPI RSDP table and add a helper to calculate the ACPI table
checksum.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 tools/libxl/libxl_arm_acpi.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

Comments

Wei Liu Aug. 24, 2016, 12:52 p.m. UTC | #1
On Tue, Aug 16, 2016 at 06:25:02PM +0800, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Construct ACPI RSDP table and add a helper to calculate the ACPI table
> checksum.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  tools/libxl/libxl_arm_acpi.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
> index 6be9eb0..9432e44 100644
> --- a/tools/libxl/libxl_arm_acpi.c
> +++ b/tools/libxl/libxl_arm_acpi.c
> @@ -33,6 +33,9 @@ extern const unsigned char dsdt_anycpu_arm[];
>  _hidden
>  extern const int dsdt_anycpu_arm_len;
>  
> +#define ACPI_BUILD_APPNAME6 "XenARM"
> +#define ACPI_BUILD_APPNAME4 "Xen "
> +

Where do these come from? If they are from a spec, could you please add
a comment here?

>  enum {
>      RSDP,
>      XSDT,
> @@ -112,6 +115,37 @@ out:
>      return rc;
>  }
>  
> +static void calculate_checksum(void *table, uint32_t checksum_offset,
> +                               uint32_t length)
> +{
> +    uint8_t *p, sum = 0;
> +
> +    p = table;
> +    p[checksum_offset] = 0;
> +
> +    while ( length-- )

Coding style.

Wei.
Shannon Zhao Aug. 25, 2016, 8:05 a.m. UTC | #2
On 2016/8/24 20:52, Wei Liu wrote:
> On Tue, Aug 16, 2016 at 06:25:02PM +0800, Shannon Zhao wrote:
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > Construct ACPI RSDP table and add a helper to calculate the ACPI table
>> > checksum.
>> > 
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> >  tools/libxl/libxl_arm_acpi.c | 38 ++++++++++++++++++++++++++++++++++++++
>> >  1 file changed, 38 insertions(+)
>> > 
>> > diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
>> > index 6be9eb0..9432e44 100644
>> > --- a/tools/libxl/libxl_arm_acpi.c
>> > +++ b/tools/libxl/libxl_arm_acpi.c
>> > @@ -33,6 +33,9 @@ extern const unsigned char dsdt_anycpu_arm[];
>> >  _hidden
>> >  extern const int dsdt_anycpu_arm_len;
>> >  
>> > +#define ACPI_BUILD_APPNAME6 "XenARM"
>> > +#define ACPI_BUILD_APPNAME4 "Xen "
>> > +
> Where do these come from? If they are from a spec, could you please add
> a comment here?
> 
Not from some spec. Just fake a OEM for these tables like the
ACPI_OEM_ID, ACPI_CREATOR_ID used by x86.

Thanks,
Wei Liu Aug. 25, 2016, 9:05 a.m. UTC | #3
On Thu, Aug 25, 2016 at 04:05:46PM +0800, Shannon Zhao wrote:
> 
> 
> On 2016/8/24 20:52, Wei Liu wrote:
> > On Tue, Aug 16, 2016 at 06:25:02PM +0800, Shannon Zhao wrote:
> >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >> > 
> >> > Construct ACPI RSDP table and add a helper to calculate the ACPI table
> >> > checksum.
> >> > 
> >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> > ---
> >> >  tools/libxl/libxl_arm_acpi.c | 38 ++++++++++++++++++++++++++++++++++++++
> >> >  1 file changed, 38 insertions(+)
> >> > 
> >> > diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
> >> > index 6be9eb0..9432e44 100644
> >> > --- a/tools/libxl/libxl_arm_acpi.c
> >> > +++ b/tools/libxl/libxl_arm_acpi.c
> >> > @@ -33,6 +33,9 @@ extern const unsigned char dsdt_anycpu_arm[];
> >> >  _hidden
> >> >  extern const int dsdt_anycpu_arm_len;
> >> >  
> >> > +#define ACPI_BUILD_APPNAME6 "XenARM"
> >> > +#define ACPI_BUILD_APPNAME4 "Xen "
> >> > +
> > Where do these come from? If they are from a spec, could you please add
> > a comment here?
> > 
> Not from some spec. Just fake a OEM for these tables like the
> ACPI_OEM_ID, ACPI_CREATOR_ID used by x86.
> 

OK.  I'm no expert of ACPI, so if that's how things are done, I am fine
with that.

Wei.

> Thanks,
> -- 
> Shannon
>
Julien Grall Aug. 29, 2016, 6:03 p.m. UTC | #4
Hi Shannon,

On 16/08/2016 06:25, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Construct ACPI RSDP table and add a helper to calculate the ACPI table
> checksum.
>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  tools/libxl/libxl_arm_acpi.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
> index 6be9eb0..9432e44 100644
> --- a/tools/libxl/libxl_arm_acpi.c
> +++ b/tools/libxl/libxl_arm_acpi.c
> @@ -33,6 +33,9 @@ extern const unsigned char dsdt_anycpu_arm[];
>  _hidden
>  extern const int dsdt_anycpu_arm_len;
>
> +#define ACPI_BUILD_APPNAME6 "XenARM"
> +#define ACPI_BUILD_APPNAME4 "Xen "
> +
>  enum {
>      RSDP,
>      XSDT,
> @@ -112,6 +115,37 @@ out:
>      return rc;
>  }
>
> +static void calculate_checksum(void *table, uint32_t checksum_offset,
> +                               uint32_t length)
> +{
> +    uint8_t *p, sum = 0;
> +
> +    p = table;
> +    p[checksum_offset] = 0;
> +
> +    while ( length-- )
> +        sum = sum + *p++;
> +
> +    p = table;
> +    p[checksum_offset] = -sum;
> +}
> +
> +static void make_acpi_rsdp(libxl__gc *gc, struct xc_dom_image *dom,
> +                           struct acpitable acpitables[])
> +{
> +    uint64_t offset = acpitables[RSDP].addr - GUEST_ACPI_BASE;
> +    struct acpi_table_rsdp *rsdp = (void *)dom->acpi_modules[0].data + offset;

Why do you cast to (void *)?

> +
> +    memcpy(rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> +    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
> +    rsdp->length = acpitables[RSDP].size;
> +    rsdp->revision = 0x02;
> +    rsdp->xsdt_physical_address = acpitables[XSDT].addr;
> +    calculate_checksum(rsdp,
> +                       offsetof(struct acpi_table_rsdp, extended_checksum),
> +                       acpitables[RSDP].size);
> +}
> +
>  int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>                          libxl__domain_build_state *state,
>                          struct xc_dom_image *dom)
> @@ -137,6 +171,10 @@ int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
>      dom->acpi_modules[0].guest_addr_out = GUEST_ACPI_BASE;
>
>      rc = libxl__estimate_acpi_size(gc, info, dom, xc_config, acpitables);
> +    if (rc)
> +        goto out;
> +
> +    make_acpi_rsdp(gc, dom, acpitables);
>
>  out:
>      return rc;
>
Julien Grall Aug. 29, 2016, 6:05 p.m. UTC | #5
Hi Shannon,

On 25/08/2016 04:05, Shannon Zhao wrote:
>
>
> On 2016/8/24 20:52, Wei Liu wrote:
>> On Tue, Aug 16, 2016 at 06:25:02PM +0800, Shannon Zhao wrote:
>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>
>>>> Construct ACPI RSDP table and add a helper to calculate the ACPI table
>>>> checksum.
>>>>
>>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>> ---
>>>>  tools/libxl/libxl_arm_acpi.c | 38 ++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 38 insertions(+)
>>>>
>>>> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
>>>> index 6be9eb0..9432e44 100644
>>>> --- a/tools/libxl/libxl_arm_acpi.c
>>>> +++ b/tools/libxl/libxl_arm_acpi.c
>>>> @@ -33,6 +33,9 @@ extern const unsigned char dsdt_anycpu_arm[];
>>>>  _hidden
>>>>  extern const int dsdt_anycpu_arm_len;
>>>>
>>>> +#define ACPI_BUILD_APPNAME6 "XenARM"
>>>> +#define ACPI_BUILD_APPNAME4 "Xen "
>>>> +
>> Where do these come from? If they are from a spec, could you please add
>> a comment here?
>>
> Not from some spec. Just fake a OEM for these tables like the
> ACPI_OEM_ID, ACPI_CREATOR_ID used by x86.

In this case, why don't we re-use the one from x86?

Regards,
Shannon Zhao Aug. 30, 2016, 1:21 a.m. UTC | #6
On 2016/8/30 2:05, Julien Grall wrote:
> Hi Shannon,
> 
> On 25/08/2016 04:05, Shannon Zhao wrote:
>>
>>
>> On 2016/8/24 20:52, Wei Liu wrote:
>>> On Tue, Aug 16, 2016 at 06:25:02PM +0800, Shannon Zhao wrote:
>>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>
>>>>> Construct ACPI RSDP table and add a helper to calculate the ACPI table
>>>>> checksum.
>>>>>
>>>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>>> ---
>>>>>  tools/libxl/libxl_arm_acpi.c | 38
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>  1 file changed, 38 insertions(+)
>>>>>
>>>>> diff --git a/tools/libxl/libxl_arm_acpi.c
>>>>> b/tools/libxl/libxl_arm_acpi.c
>>>>> index 6be9eb0..9432e44 100644
>>>>> --- a/tools/libxl/libxl_arm_acpi.c
>>>>> +++ b/tools/libxl/libxl_arm_acpi.c
>>>>> @@ -33,6 +33,9 @@ extern const unsigned char dsdt_anycpu_arm[];
>>>>>  _hidden
>>>>>  extern const int dsdt_anycpu_arm_len;
>>>>>
>>>>> +#define ACPI_BUILD_APPNAME6 "XenARM"
>>>>> +#define ACPI_BUILD_APPNAME4 "Xen "
>>>>> +
>>> Where do these come from? If they are from a spec, could you please add
>>> a comment here?
>>>
>> Not from some spec. Just fake a OEM for these tables like the
>> ACPI_OEM_ID, ACPI_CREATOR_ID used by x86.
> 
> In this case, why don't we re-use the one from x86?
> 
While the ACPI_OEM_TABLE_ID and ACPI_CREATOR_ID of x86 are HVM specific,
I don't think it's proper for ARM.

Thanks,
Julien Grall Aug. 30, 2016, 5:11 p.m. UTC | #7
Hi Shannon,

On 30/08/16 02:21, Shannon Zhao wrote:
>
>
> On 2016/8/30 2:05, Julien Grall wrote:
>> Hi Shannon,
>>
>> On 25/08/2016 04:05, Shannon Zhao wrote:
>>>
>>>
>>> On 2016/8/24 20:52, Wei Liu wrote:
>>>> On Tue, Aug 16, 2016 at 06:25:02PM +0800, Shannon Zhao wrote:
>>>>>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>>
>>>>>> Construct ACPI RSDP table and add a helper to calculate the ACPI table
>>>>>> checksum.
>>>>>>
>>>>>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>>>>>> ---
>>>>>>  tools/libxl/libxl_arm_acpi.c | 38
>>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 38 insertions(+)
>>>>>>
>>>>>> diff --git a/tools/libxl/libxl_arm_acpi.c
>>>>>> b/tools/libxl/libxl_arm_acpi.c
>>>>>> index 6be9eb0..9432e44 100644
>>>>>> --- a/tools/libxl/libxl_arm_acpi.c
>>>>>> +++ b/tools/libxl/libxl_arm_acpi.c
>>>>>> @@ -33,6 +33,9 @@ extern const unsigned char dsdt_anycpu_arm[];
>>>>>>  _hidden
>>>>>>  extern const int dsdt_anycpu_arm_len;
>>>>>>
>>>>>> +#define ACPI_BUILD_APPNAME6 "XenARM"
>>>>>> +#define ACPI_BUILD_APPNAME4 "Xen "
>>>>>> +
>>>> Where do these come from? If they are from a spec, could you please add
>>>> a comment here?
>>>>
>>> Not from some spec. Just fake a OEM for these tables like the
>>> ACPI_OEM_ID, ACPI_CREATOR_ID used by x86.
>>
>> In this case, why don't we re-use the one from x86?
>>
> While the ACPI_OEM_TABLE_ID and ACPI_CREATOR_ID of x86 are HVM specific,
> I don't think it's proper for ARM.

IIRC we have an OEM ID and Creator ID reserved for Xen. Stefano can you 
confirm?

In any case, it would be better if we re-use the existing one. It does 
not hurt to use a different name. For instance the signature of the MADT 
table is "APIC" with is x86 specific...

Regards,
Stefano Stabellini Aug. 30, 2016, 9:38 p.m. UTC | #8
On Tue, 30 Aug 2016, Julien Grall wrote:
> Hi Shannon,
> 
> On 30/08/16 02:21, Shannon Zhao wrote:
> > 
> > 
> > On 2016/8/30 2:05, Julien Grall wrote:
> > > Hi Shannon,
> > > 
> > > On 25/08/2016 04:05, Shannon Zhao wrote:
> > > > 
> > > > 
> > > > On 2016/8/24 20:52, Wei Liu wrote:
> > > > > On Tue, Aug 16, 2016 at 06:25:02PM +0800, Shannon Zhao wrote:
> > > > > > > From: Shannon Zhao <shannon.zhao@linaro.org>
> > > > > > > 
> > > > > > > Construct ACPI RSDP table and add a helper to calculate the ACPI
> > > > > > > table
> > > > > > > checksum.
> > > > > > > 
> > > > > > > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> > > > > > > ---
> > > > > > >  tools/libxl/libxl_arm_acpi.c | 38
> > > > > > > ++++++++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 38 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/tools/libxl/libxl_arm_acpi.c
> > > > > > > b/tools/libxl/libxl_arm_acpi.c
> > > > > > > index 6be9eb0..9432e44 100644
> > > > > > > --- a/tools/libxl/libxl_arm_acpi.c
> > > > > > > +++ b/tools/libxl/libxl_arm_acpi.c
> > > > > > > @@ -33,6 +33,9 @@ extern const unsigned char dsdt_anycpu_arm[];
> > > > > > >  _hidden
> > > > > > >  extern const int dsdt_anycpu_arm_len;
> > > > > > > 
> > > > > > > +#define ACPI_BUILD_APPNAME6 "XenARM"
> > > > > > > +#define ACPI_BUILD_APPNAME4 "Xen "
> > > > > > > +
> > > > > Where do these come from? If they are from a spec, could you please
> > > > > add
> > > > > a comment here?
> > > > > 
> > > > Not from some spec. Just fake a OEM for these tables like the
> > > > ACPI_OEM_ID, ACPI_CREATOR_ID used by x86.
> > > 
> > > In this case, why don't we re-use the one from x86?
> > > 
> > While the ACPI_OEM_TABLE_ID and ACPI_CREATOR_ID of x86 are HVM specific,
> > I don't think it's proper for ARM.
> 
> IIRC we have an OEM ID and Creator ID reserved for Xen. Stefano can you
> confirm?
>
> In any case, it would be better if we re-use the existing one. It does not
> hurt to use a different name. For instance the signature of the MADT table is
> "APIC" with is x86 specific...

I think it makes sense to use the same OEM id as x86, which seems to be
"Xen", but I am not sure about who reserved it, it predates me. I would
change the creator id, because it is "HVML" on x86, for "HVM Loader".
Maybe we could use "xl". FYI creator id is the id of the utility that
generated the tables.
diff mbox

Patch

diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
index 6be9eb0..9432e44 100644
--- a/tools/libxl/libxl_arm_acpi.c
+++ b/tools/libxl/libxl_arm_acpi.c
@@ -33,6 +33,9 @@  extern const unsigned char dsdt_anycpu_arm[];
 _hidden
 extern const int dsdt_anycpu_arm_len;
 
+#define ACPI_BUILD_APPNAME6 "XenARM"
+#define ACPI_BUILD_APPNAME4 "Xen "
+
 enum {
     RSDP,
     XSDT,
@@ -112,6 +115,37 @@  out:
     return rc;
 }
 
+static void calculate_checksum(void *table, uint32_t checksum_offset,
+                               uint32_t length)
+{
+    uint8_t *p, sum = 0;
+
+    p = table;
+    p[checksum_offset] = 0;
+
+    while ( length-- )
+        sum = sum + *p++;
+
+    p = table;
+    p[checksum_offset] = -sum;
+}
+
+static void make_acpi_rsdp(libxl__gc *gc, struct xc_dom_image *dom,
+                           struct acpitable acpitables[])
+{
+    uint64_t offset = acpitables[RSDP].addr - GUEST_ACPI_BASE;
+    struct acpi_table_rsdp *rsdp = (void *)dom->acpi_modules[0].data + offset;
+
+    memcpy(rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
+    memcpy(rsdp->oem_id, ACPI_BUILD_APPNAME6, sizeof(rsdp->oem_id));
+    rsdp->length = acpitables[RSDP].size;
+    rsdp->revision = 0x02;
+    rsdp->xsdt_physical_address = acpitables[XSDT].addr;
+    calculate_checksum(rsdp,
+                       offsetof(struct acpi_table_rsdp, extended_checksum),
+                       acpitables[RSDP].size);
+}
+
 int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
                         libxl__domain_build_state *state,
                         struct xc_dom_image *dom)
@@ -137,6 +171,10 @@  int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
     dom->acpi_modules[0].guest_addr_out = GUEST_ACPI_BASE;
 
     rc = libxl__estimate_acpi_size(gc, info, dom, xc_config, acpitables);
+    if (rc)
+        goto out;
+
+    make_acpi_rsdp(gc, dom, acpitables);
 
 out:
     return rc;