[PATCH] watchdog: wdat: handle ACPI RSDP revision < 2

[Cedric Hombourger]

ACPI uses 32-bit addresses for RSDP revision < 2: use RSDT table
in this case instead of XSDT. Tested on SIMATIC IPC847e.

Signed-off-by: Cedric Hombourger <Cedric_H...@mentor.com>
---
drivers/watchdog/wdat.c | 53 ++++++++++++++++++++++++++++++++++-------
1 file changed, 45 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
index df708cf..5157955 100644
--- a/drivers/watchdog/wdat.c
+++ b/drivers/watchdog/wdat.c
@@ -29,6 +29,7 @@
#define EFI_ACPI_ROOT_SDP_REVISION 0x02

#define ACPI_SIG_RSDP (CHAR8 *)"RSD PTR "
+#define ACPI_SIG_RSDT (CHAR8 *)"RSDT"
#define ACPI_SIG_XSDT (CHAR8 *)"XSDT"
#define ACPI_SIG_WDAT (CHAR8 *)"WDAT"

@@ -140,18 +141,32 @@ typedef struct {
*/

static EFI_STATUS
-parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {
- EFI_ACPI_SDT_HEADER *xsdt;
- UINT64 *entry_ptr;
- UINT64 n, count;
+parse_rsdt(EFI_ACPI_SDT_HEADER *rsdt, ACPI_TABLE_WDAT **wdat_table_ptr) {
+ UINT32 *entry_ptr;
+ UINT32 n, count;

- *wdat_table_ptr = NULL;
+ if (strncmpa(ACPI_SIG_RSDT, (CHAR8 *)(VOID *)(rsdt->signature), 4)) {
+ return EFI_INCOMPATIBLE_VERSION;
+ }

- if (rsdp->revision < EFI_ACPI_ROOT_SDP_REVISION) {
- return EFI_NOT_FOUND;
+ entry_ptr = (UINT32 *)(rsdt + 1);
+ count = (rsdt->length - sizeof (EFI_ACPI_SDT_HEADER)) / sizeof(UINT32);
+ for (n = 0; n < count; n++, entry_ptr++) {
+ EFI_ACPI_SDT_HEADER *entry =
+ (EFI_ACPI_SDT_HEADER *)((UINTN)(*entry_ptr));
+ if (!strncmpa(ACPI_SIG_WDAT, entry->signature, 4)) {
+ *wdat_table_ptr = (ACPI_TABLE_WDAT *)entry;
+ return EFI_SUCCESS;
+ }
}
+ return EFI_NOT_FOUND;
+}
+
+static EFI_STATUS
+parse_xsdt(EFI_ACPI_SDT_HEADER *xsdt, ACPI_TABLE_WDAT **wdat_table_ptr) {
+ UINT64 *entry_ptr;
+ UINT64 n, count;

- xsdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->xsdt_address);
if (strncmpa(ACPI_SIG_XSDT, (CHAR8 *)(VOID *)(xsdt->signature), 4)) {
return EFI_INCOMPATIBLE_VERSION;
}
@@ -169,6 +184,28 @@ parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {
return EFI_NOT_FOUND;
}

+static EFI_STATUS
+parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {
+ EFI_ACPI_SDT_HEADER *sdt;
+
+ *wdat_table_ptr = NULL;
+
+ if (rsdp->revision > EFI_ACPI_ROOT_SDP_REVISION) {
+ ERROR(L"SDP revision not supported (%d)\n", rsdp->revision);
+ return EFI_INCOMPATIBLE_VERSION;
+ }
+
+ if (rsdp->revision == EFI_ACPI_ROOT_SDP_REVISION) {
+ sdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->xsdt_address);
+ return parse_xsdt(sdt, wdat_table_ptr);
+ }
+ else {
+ /* Promote 32-bit address to 64-bit... */
+ sdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->rsdt_address);
+ return parse_rsdt(sdt, wdat_table_ptr);
+ }
+}
+
static EFI_STATUS
locate_and_parse_rsdp(ACPI_TABLE_WDAT **wdat_table_ptr) {
EFI_CONFIGURATION_TABLE *ect = ST->ConfigurationTable;
--
2.30.2

[Jan Kiszka]

On 14.10.21 14:24, Cedric Hombourger wrote:
> ACPI uses 32-bit addresses for RSDP revision < 2: use RSDT table
> in this case instead of XSDT. Tested on SIMATIC IPC847e.

Also tested on 127E again?

>
> Signed-off-by: Cedric Hombourger <Cedric_H...@mentor.com>
> ---
> drivers/watchdog/wdat.c | 53 ++++++++++++++++++++++++++++++++++-------
> 1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
> index df708cf..5157955 100644
> --- a/drivers/watchdog/wdat.c
> +++ b/drivers/watchdog/wdat.c
> @@ -29,6 +29,7 @@
> #define EFI_ACPI_ROOT_SDP_REVISION 0x02
>
> #define ACPI_SIG_RSDP (CHAR8 *)"RSD PTR "
> +#define ACPI_SIG_RSDT (CHAR8 *)"RSDT"
> #define ACPI_SIG_XSDT (CHAR8 *)"XSDT"
> #define ACPI_SIG_WDAT (CHAR8 *)"WDAT"
>
> @@ -140,18 +141,32 @@ typedef struct {
> */
>
> static EFI_STATUS
> -parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {
> - EFI_ACPI_SDT_HEADER *xsdt;
> - UINT64 *entry_ptr;
> - UINT64 n, count;
> +parse_rsdt(EFI_ACPI_SDT_HEADER *rsdt, ACPI_TABLE_WDAT **wdat_table_ptr) {

parse_rsdt()
{

normally, but I see the style of the file is generally off.

This rejects now revisions that were previously accepted - intentionally?

> + ERROR(L"SDP revision not supported (%d)\n", rsdp->revision);
> + return EFI_INCOMPATIBLE_VERSION;
> + }
> +
> + if (rsdp->revision == EFI_ACPI_ROOT_SDP_REVISION) {
> + sdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->xsdt_address);
> + return parse_xsdt(sdt, wdat_table_ptr);
> + }
> + else {
> + /* Promote 32-bit address to 64-bit... */

...only on 64-bit targets. Not sure if this adds information, also
because the pattern is identical to xsdt.

> + sdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->rsdt_address);
> + return parse_rsdt(sdt, wdat_table_ptr);
> + }
> +}
> +
> static EFI_STATUS
> locate_and_parse_rsdp(ACPI_TABLE_WDAT **wdat_table_ptr) {
> EFI_CONFIGURATION_TABLE *ect = ST->ConfigurationTable;
>

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Hombourger, Cedric]

> -----Original Message-----
> From: Jan Kiszka <jan.k...@siemens.com>
> Sent: Thursday, October 14, 2021 2:52 PM
> To: Hombourger, Cedric <Cedric_H...@mentor.com>; efibootguard-
> d...@googlegroups.com
> Subject: Re: [PATCH] watchdog: wdat: handle ACPI RSDP revision < 2
>
> On 14.10.21 14:24, Cedric Hombourger wrote:
> > ACPI uses 32-bit addresses for RSDP revision < 2: use RSDT table in
> > this case instead of XSDT. Tested on SIMATIC IPC847e.
>
> Also tested on 127E again?

Yes since that's all I have on my desk. Testing on the 847E was done in Bangalore and test results provided back to me.

>
> >
> > Signed-off-by: Cedric Hombourger <Cedric_H...@mentor.com>
> > ---
> > drivers/watchdog/wdat.c | 53
> > ++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 45 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c index
> > df708cf..5157955 100644
> > --- a/drivers/watchdog/wdat.c
> > +++ b/drivers/watchdog/wdat.c
> > @@ -29,6 +29,7 @@
> > #define EFI_ACPI_ROOT_SDP_REVISION 0x02
> >
> > #define ACPI_SIG_RSDP (CHAR8 *)"RSD PTR "
> > +#define ACPI_SIG_RSDT (CHAR8 *)"RSDT"
> > #define ACPI_SIG_XSDT (CHAR8 *)"XSDT"
> > #define ACPI_SIG_WDAT (CHAR8 *)"WDAT"
> >
> > @@ -140,18 +141,32 @@ typedef struct {
> > */
> >
> > static EFI_STATUS
> > -parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT
> **wdat_table_ptr) {
> > - EFI_ACPI_SDT_HEADER *xsdt;
> > - UINT64 *entry_ptr;
> > - UINT64 n, count;
> > +parse_rsdt(EFI_ACPI_SDT_HEADER *rsdt, ACPI_TABLE_WDAT

> > +**wdat_table_ptr) {

[Jan Kiszka]

On 14.10.21 14:54, Hombourger, Cedric wrote:
>
>
>> -----Original Message-----
>> From: Jan Kiszka <jan.k...@siemens.com>
>> Sent: Thursday, October 14, 2021 2:52 PM
>> To: Hombourger, Cedric <Cedric_H...@mentor.com>; efibootguard-
>> d...@googlegroups.com
>> Subject: Re: [PATCH] watchdog: wdat: handle ACPI RSDP revision < 2
>>
>> On 14.10.21 14:24, Cedric Hombourger wrote:
>>> ACPI uses 32-bit addresses for RSDP revision < 2: use RSDT table in
>>> this case instead of XSDT. Tested on SIMATIC IPC847e.
>>
>> Also tested on 127E again?
>
> Yes since that's all I have on my desk. Testing on the 847E was done in Bangalore and test results provided back to me.
>

Could we clarify the other remarks as well? Then this could still make
it into 0.9.

Jan

[Hombourger, Cedric]

Hi Jan,

I failed to see all queries you had. Responding with all queries
hopefully covered this time.

Cedric

On 10/14/2021 2:51 PM, Jan Kiszka wrote:
> On 14.10.21 14:24, Cedric Hombourger wrote:
>> ACPI uses 32-bit addresses for RSDP revision < 2: use RSDT table
>> in this case instead of XSDT. Tested on SIMATIC IPC847e.
> Also tested on 127E again?

Checked on both 127E (ACPI v2) and 847E (ACPI v1)

>
>> Signed-off-by: Cedric Hombourger <Cedric_H...@mentor.com>
>> ---
>> drivers/watchdog/wdat.c | 53 ++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
>> index df708cf..5157955 100644
>> --- a/drivers/watchdog/wdat.c
>> +++ b/drivers/watchdog/wdat.c
>> @@ -29,6 +29,7 @@
>> #define EFI_ACPI_ROOT_SDP_REVISION 0x02
>>
>> #define ACPI_SIG_RSDP (CHAR8 *)"RSD PTR "
>> +#define ACPI_SIG_RSDT (CHAR8 *)"RSDT"
>> #define ACPI_SIG_XSDT (CHAR8 *)"XSDT"
>> #define ACPI_SIG_WDAT (CHAR8 *)"WDAT"
>>
>> @@ -140,18 +141,32 @@ typedef struct {
>> */
>>
>> static EFI_STATUS
>> -parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {
>> - EFI_ACPI_SDT_HEADER *xsdt;
>> - UINT64 *entry_ptr;
>> - UINT64 n, count;
>> +parse_rsdt(EFI_ACPI_SDT_HEADER *rsdt, ACPI_TABLE_WDAT **wdat_table_ptr) {
> parse_rsdt()
> {
>
> normally, but I see the style of the file is generally off.

Fixing in v2

Yes it is. I felt it may be unsafe to claim support for e.g. ACPI v3

>
>> + ERROR(L"SDP revision not supported (%d)\n", rsdp->revision);
>> + return EFI_INCOMPATIBLE_VERSION;
>> + }
>> +
>> + if (rsdp->revision == EFI_ACPI_ROOT_SDP_REVISION) {
>> + sdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->xsdt_address);
>> + return parse_xsdt(sdt, wdat_table_ptr);
>> + }
>> + else {
>> + /* Promote 32-bit address to 64-bit... */
> ...only on 64-bit targets. Not sure if this adds information, also
> because the pattern is identical to xsdt.

Killing this comment in v2

[Cedric Hombourger]

ACPI uses 32-bit addresses for RSDP revision < 2: use RSDT table

in this case instead of XSDT. Tested on SIMATIC IPC847e for ACPI
revision 1 and used SIMATIC IPC127e for non-regression tests
for revision 2.

Signed-off-by: Cedric Hombourger <Cedric_H...@mentor.com>
---

drivers/watchdog/wdat.c | 58 ++++++++++++++++++++++++++++++++++-------
1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/wdat.c b/drivers/watchdog/wdat.c
index df708cf..11f2adc 100644

--- a/drivers/watchdog/wdat.c
+++ b/drivers/watchdog/wdat.c
@@ -29,6 +29,7 @@
#define EFI_ACPI_ROOT_SDP_REVISION 0x02

#define ACPI_SIG_RSDP (CHAR8 *)"RSD PTR "
+#define ACPI_SIG_RSDT (CHAR8 *)"RSDT"
#define ACPI_SIG_XSDT (CHAR8 *)"XSDT"
#define ACPI_SIG_WDAT (CHAR8 *)"WDAT"

@@ -140,18 +141,34 @@ typedef struct {

*/

static EFI_STATUS
-parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {
- EFI_ACPI_SDT_HEADER *xsdt;
- UINT64 *entry_ptr;
- UINT64 n, count;
+parse_rsdt(EFI_ACPI_SDT_HEADER *rsdt, ACPI_TABLE_WDAT **wdat_table_ptr)

+{

+ UINT32 *entry_ptr;
+ UINT32 n, count;

- *wdat_table_ptr = NULL;
+ if (strncmpa(ACPI_SIG_RSDT, (CHAR8 *)(VOID *)(rsdt->signature), 4)) {
+ return EFI_INCOMPATIBLE_VERSION;
+ }

- if (rsdp->revision < EFI_ACPI_ROOT_SDP_REVISION) {
- return EFI_NOT_FOUND;
+ entry_ptr = (UINT32 *)(rsdt + 1);
+ count = (rsdt->length - sizeof (EFI_ACPI_SDT_HEADER)) / sizeof(UINT32);
+ for (n = 0; n < count; n++, entry_ptr++) {
+ EFI_ACPI_SDT_HEADER *entry =
+ (EFI_ACPI_SDT_HEADER *)((UINTN)(*entry_ptr));
+ if (!strncmpa(ACPI_SIG_WDAT, entry->signature, 4)) {
+ *wdat_table_ptr = (ACPI_TABLE_WDAT *)entry;
+ return EFI_SUCCESS;
+ }
}
+ return EFI_NOT_FOUND;
+}
+
+static EFI_STATUS
+parse_xsdt(EFI_ACPI_SDT_HEADER *xsdt, ACPI_TABLE_WDAT **wdat_table_ptr)

+{

+ UINT64 *entry_ptr;
+ UINT64 n, count;

- xsdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->xsdt_address);
if (strncmpa(ACPI_SIG_XSDT, (CHAR8 *)(VOID *)(xsdt->signature), 4)) {
return EFI_INCOMPATIBLE_VERSION;
}

@@ -170,7 +187,30 @@ parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr) {
}

static EFI_STATUS
-locate_and_parse_rsdp(ACPI_TABLE_WDAT **wdat_table_ptr) {
+parse_rsdp(EFI_ACPI_ROOT_SDP_HEADER *rsdp, ACPI_TABLE_WDAT **wdat_table_ptr)
+{

+ EFI_ACPI_SDT_HEADER *sdt;
+
+ *wdat_table_ptr = NULL;
+
+ if (rsdp->revision > EFI_ACPI_ROOT_SDP_REVISION) {

+ ERROR(L"SDP revision not supported (%d)\n", rsdp->revision);
+ return EFI_INCOMPATIBLE_VERSION;
+ }
+
+ if (rsdp->revision == EFI_ACPI_ROOT_SDP_REVISION) {
+ sdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->xsdt_address);
+ return parse_xsdt(sdt, wdat_table_ptr);
+ }
+ else {

+ sdt = (EFI_ACPI_SDT_HEADER *)(UINTN)(rsdp->rsdt_address);
+ return parse_rsdt(sdt, wdat_table_ptr);
+ }
+}
+

+static EFI_STATUS
+locate_and_parse_rsdp(ACPI_TABLE_WDAT **wdat_table_ptr)
+{

EFI_CONFIGURATION_TABLE *ect = ST->ConfigurationTable;

EFI_ACPI_ROOT_SDP_HEADER *rsdp;
EFI_GUID acpi_table_guid = ACPI_TABLE_GUID;
--
2.30.2

[Jan Kiszka]

Thanks, applied.