diff mbox series

[kvm-unit-tests,v2,03/23] lib: Add support for the XSDT ACPI table

Message ID 20220506205605.359830-4-nikos.nikoleris@arm.com (mailing list archive)
State New, archived
Headers show
Series EFI and ACPI support for arm64 | expand

Commit Message

Nikos Nikoleris May 6, 2022, 8:55 p.m. UTC 
XSDT provides pointers to other ACPI tables much like RSDT. However,
contrary to RSDT that provides 32-bit addresses, XSDT provides 64-bit
pointers. ACPI requires that if XSDT is valid then it takes precedence
over RSDT.

Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
---
 lib/acpi.h |   6 ++++
 lib/acpi.c | 103 ++++++++++++++++++++++++++++++++---------------------
 2 files changed, 68 insertions(+), 41 deletions(-)

Comments

Andrew Jones May 19, 2022, 1:30 p.m. UTC | #1 
On Fri, May 06, 2022 at 09:55:45PM +0100, Nikos Nikoleris wrote:
> XSDT provides pointers to other ACPI tables much like RSDT. However,
> contrary to RSDT that provides 32-bit addresses, XSDT provides 64-bit
> pointers. ACPI requires that if XSDT is valid then it takes precedence
> over RSDT.
> 
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
>  lib/acpi.h |   6 ++++
>  lib/acpi.c | 103 ++++++++++++++++++++++++++++++++---------------------
>  2 files changed, 68 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/acpi.h b/lib/acpi.h
> index 42a2c16..d80b983 100644
> --- a/lib/acpi.h
> +++ b/lib/acpi.h
> @@ -13,6 +13,7 @@
>  
>  #define RSDP_SIGNATURE ACPI_SIGNATURE('R','S','D','P')
>  #define RSDT_SIGNATURE ACPI_SIGNATURE('R','S','D','T')
> +#define XSDT_SIGNATURE ACPI_SIGNATURE('X','S','D','T')
>  #define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
>  #define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
>  
> @@ -56,6 +57,11 @@ struct rsdt_descriptor_rev1 {
>      u32 table_offset_entry[0];
>  } __attribute__ ((packed));
>  
> +struct acpi_table_xsdt {
> +    ACPI_TABLE_HEADER_DEF
> +    u64 table_offset_entry[1];
> +} __attribute__ ((packed));
> +
>  struct fadt_descriptor_rev1
>  {
>      ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
> diff --git a/lib/acpi.c b/lib/acpi.c
> index de275ca..9b8700c 100644
> --- a/lib/acpi.c
> +++ b/lib/acpi.c
> @@ -38,45 +38,66 @@ static struct rsdp_descriptor *get_rsdp(void)
>  
>  void* find_acpi_table_addr(u32 sig)
>  {
> -    struct rsdp_descriptor *rsdp;
> -    struct rsdt_descriptor_rev1 *rsdt;
> -    void *end;
> -    int i;
> -
> -    /* FACS is special... */
> -    if (sig == FACS_SIGNATURE) {
> -        struct fadt_descriptor_rev1 *fadt;
> -        fadt = find_acpi_table_addr(FACP_SIGNATURE);
> -        if (!fadt) {
> -            return NULL;
> -        }
> -        return (void*)(ulong)fadt->firmware_ctrl;
> -    }
> -
> -    rsdp = get_rsdp();
> -    if (rsdp == NULL) {
> -        printf("Can't find RSDP\n");
> -        return 0;
> -    }
> -
> -    if (sig == RSDP_SIGNATURE) {
> -        return rsdp;
> -    }
> -
> -    rsdt = (void*)(ulong)rsdp->rsdt_physical_address;
> -    if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
> -        return 0;
> -
> -    if (sig == RSDT_SIGNATURE) {
> -        return rsdt;
> -    }
> -
> -    end = (void*)rsdt + rsdt->length;
> -    for (i=0; (void*)&rsdt->table_offset_entry[i] < end; i++) {
> -        struct acpi_table *t = (void*)(ulong)rsdt->table_offset_entry[i];
> -        if (t && t->signature == sig) {
> -            return t;
> -        }
> -    }
> -   return NULL;

Let's definitely fix the coding style earlier in the series. Either while
moving the file or as another patch right after moving the file. That, or
use the old style for this file when updating it, since we don't want to
mix styles in the same file.

> +	struct rsdp_descriptor *rsdp;
> +	struct rsdt_descriptor_rev1 *rsdt;
> +	struct acpi_table_xsdt *xsdt = NULL;
> +	void *end;
> +	int i;
> +
> +	/* FACS is special... */
> +	if (sig == FACS_SIGNATURE) {
> +		struct fadt_descriptor_rev1 *fadt;
> +
> +		fadt = find_acpi_table_addr(FACP_SIGNATURE);
> +		if (!fadt)
> +			return NULL;
> +
> +		return (void*)(ulong)fadt->firmware_ctrl;
> +	}
> +
> +	rsdp = get_rsdp();
> +	if (rsdp == NULL) {
> +		printf("Can't find RSDP\n");
> +		return 0;
> +	}
> +
> +	if (sig == RSDP_SIGNATURE)
> +		return rsdp;
> +
> +	rsdt = (void *)(ulong)rsdp->rsdt_physical_address;
> +	if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
> +		rsdt = NULL;
> +
> +	if (sig == RSDT_SIGNATURE)
> +		return rsdt;
> +
> +	if (rsdp->revision > 1)
> +		xsdt = (void *)(ulong)rsdp->xsdt_physical_address;
> +	if (!xsdt || xsdt->signature != XSDT_SIGNATURE)
> +		xsdt = NULL;
> +
> +	if (sig == XSDT_SIGNATURE)
> +		return xsdt;
> +
> +	// APCI requires that we first try to use XSDT if it's valid,
> +	//  we use to find other tables, otherwise we use RSDT.

/* ... */ style comments please. And the comment looks like it's missing
something like "When it's valid..."

> +	if (xsdt) {
> +		end = (void *)(ulong)xsdt + xsdt->length;
> +		for (i = 0; (void *)&xsdt->table_offset_entry[i] < end; i++) {
> +			struct acpi_table *t =
> +				(void *)xsdt->table_offset_entry[i];

nit: The kernel's checkpatch allows 100 char line length. Let's use all of them :-)

> +			if (t && t->signature == sig)
> +				return t;
> +		}
> +	} else if (rsdt) {
> +		end = (void *)rsdt + rsdt->length;
> +		for (i = 0; (void *)&rsdt->table_offset_entry[i] < end; i++) {
> +			struct acpi_table *t =
> +				(void *)(ulong)rsdt->table_offset_entry[i];

Same nit as above.

> +			if (t && t->signature == sig)
> +				return t;
> +		}
> +	}
> +
> +	return NULL;
>  }
> -- 
> 2.25.1
>

Thanks,
drew
Ricardo Koller June 18, 2022, 12:38 a.m. UTC | #2 
Hi Nikos,

On Fri, May 06, 2022 at 09:55:45PM +0100, Nikos Nikoleris wrote:
> XSDT provides pointers to other ACPI tables much like RSDT. However,
> contrary to RSDT that provides 32-bit addresses, XSDT provides 64-bit
> pointers. ACPI requires that if XSDT is valid then it takes precedence
> over RSDT.
> 
> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> ---
>  lib/acpi.h |   6 ++++
>  lib/acpi.c | 103 ++++++++++++++++++++++++++++++++---------------------
>  2 files changed, 68 insertions(+), 41 deletions(-)
> 
> diff --git a/lib/acpi.h b/lib/acpi.h
> index 42a2c16..d80b983 100644
> --- a/lib/acpi.h
> +++ b/lib/acpi.h
> @@ -13,6 +13,7 @@
>  
>  #define RSDP_SIGNATURE ACPI_SIGNATURE('R','S','D','P')
>  #define RSDT_SIGNATURE ACPI_SIGNATURE('R','S','D','T')
> +#define XSDT_SIGNATURE ACPI_SIGNATURE('X','S','D','T')
>  #define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
>  #define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
>  
> @@ -56,6 +57,11 @@ struct rsdt_descriptor_rev1 {
>      u32 table_offset_entry[0];
>  } __attribute__ ((packed));
>  
> +struct acpi_table_xsdt {
> +    ACPI_TABLE_HEADER_DEF
> +    u64 table_offset_entry[1];

nit: This should be "[0]" to match the usage above (in rsdt).

I was about to suggest using an unspecified size "[]", but after reading
what the C standard says about it (below), now I'm not sure. was the
"[1]" needed for something that I'm missing?

	106) The length is unspecified to allow for the fact that
	implementations may give array members different
	alignments according to their lengths.


> +} __attribute__ ((packed));
> +
>  struct fadt_descriptor_rev1
>  {
>      ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
> diff --git a/lib/acpi.c b/lib/acpi.c
> index de275ca..9b8700c 100644
> --- a/lib/acpi.c
> +++ b/lib/acpi.c
> @@ -38,45 +38,66 @@ static struct rsdp_descriptor *get_rsdp(void)
>  
>  void* find_acpi_table_addr(u32 sig)

nit: This one could also be fixed as well: "void *".

>  {
> -    struct rsdp_descriptor *rsdp;
> -    struct rsdt_descriptor_rev1 *rsdt;
> -    void *end;
> -    int i;
> -
> -    /* FACS is special... */
> -    if (sig == FACS_SIGNATURE) {
> -        struct fadt_descriptor_rev1 *fadt;
> -        fadt = find_acpi_table_addr(FACP_SIGNATURE);
> -        if (!fadt) {
> -            return NULL;
> -        }
> -        return (void*)(ulong)fadt->firmware_ctrl;
> -    }
> -
> -    rsdp = get_rsdp();
> -    if (rsdp == NULL) {
> -        printf("Can't find RSDP\n");
> -        return 0;
> -    }
> -
> -    if (sig == RSDP_SIGNATURE) {
> -        return rsdp;
> -    }
> -
> -    rsdt = (void*)(ulong)rsdp->rsdt_physical_address;
> -    if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
> -        return 0;
> -
> -    if (sig == RSDT_SIGNATURE) {
> -        return rsdt;
> -    }
> -
> -    end = (void*)rsdt + rsdt->length;
> -    for (i=0; (void*)&rsdt->table_offset_entry[i] < end; i++) {
> -        struct acpi_table *t = (void*)(ulong)rsdt->table_offset_entry[i];
> -        if (t && t->signature == sig) {
> -            return t;
> -        }
> -    }
> -   return NULL;
> +	struct rsdp_descriptor *rsdp;
> +	struct rsdt_descriptor_rev1 *rsdt;
> +	struct acpi_table_xsdt *xsdt = NULL;
> +	void *end;
> +	int i;
> +
> +	/* FACS is special... */
> +	if (sig == FACS_SIGNATURE) {
> +		struct fadt_descriptor_rev1 *fadt;
> +
> +		fadt = find_acpi_table_addr(FACP_SIGNATURE);
> +		if (!fadt)
> +			return NULL;
> +
> +		return (void*)(ulong)fadt->firmware_ctrl;
> +	}
> +
> +	rsdp = get_rsdp();
> +	if (rsdp == NULL) {
> +		printf("Can't find RSDP\n");
> +		return 0;
> +	}
> +
> +	if (sig == RSDP_SIGNATURE)
> +		return rsdp;
> +
> +	rsdt = (void *)(ulong)rsdp->rsdt_physical_address;
> +	if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
> +		rsdt = NULL;
> +
> +	if (sig == RSDT_SIGNATURE)
> +		return rsdt;
> +
> +	if (rsdp->revision > 1)
> +		xsdt = (void *)(ulong)rsdp->xsdt_physical_address;
> +	if (!xsdt || xsdt->signature != XSDT_SIGNATURE)
> +		xsdt = NULL;
> +

To simplify this function a bit, finding the xsdt could be moved to some
kind of init function.

> +	if (sig == XSDT_SIGNATURE)
> +		return xsdt;
> +
> +	// APCI requires that we first try to use XSDT if it's valid,
> +	//  we use to find other tables, otherwise we use RSDT.
> +	if (xsdt) {
> +		end = (void *)(ulong)xsdt + xsdt->length;
> +		for (i = 0; (void *)&xsdt->table_offset_entry[i] < end; i++) {
> +			struct acpi_table *t =
> +				(void *)xsdt->table_offset_entry[i];
> +			if (t && t->signature == sig)
> +				return t;
> +		}
> +	} else if (rsdt) {
> +		end = (void *)rsdt + rsdt->length;
> +		for (i = 0; (void *)&rsdt->table_offset_entry[i] < end; i++) {
> +			struct acpi_table *t =
> +				(void *)(ulong)rsdt->table_offset_entry[i];
> +			if (t && t->signature == sig)
> +				return t;
> +		}
> +	}

The two for-loops could be moved into some common function, or maybe a
macro to deal with the fact that it deals with two different structures
(the rsdt and the xsdt). This is my attempt at making it a function:

void *scan_system_descriptor_table(void *dt)
{
	int i;
	void *end;
	/* XXX: Not sure if this is the nicest thing to do, but the rsdt
	 * and the xsdt have "length" and "table_offset_entry" at the
	 * same offsets. */
	struct acpi_table_xsdt *xsdt = dt;

	end = (void *)(ulong)xsdt + xsdt->length;
	for (i = 0; &xsdt->table_offset_entry[i] < end; i++) {
		struct acpi_table *t = (void *)xsdt->table_offset_entry[i];
		if (t && t->signature == sig)
			return t;
}

Thanks,
Ricardo

> +
> +	return NULL;
>  }
> -- 
> 2.25.1
>
Alexandru Elisei June 20, 2022, 8:53 a.m. UTC | #3 
Hi,

On Fri, Jun 17, 2022 at 05:38:01PM -0700, Ricardo Koller wrote:
> Hi Nikos,
> 
> On Fri, May 06, 2022 at 09:55:45PM +0100, Nikos Nikoleris wrote:
> > XSDT provides pointers to other ACPI tables much like RSDT. However,
> > contrary to RSDT that provides 32-bit addresses, XSDT provides 64-bit
> > pointers. ACPI requires that if XSDT is valid then it takes precedence
> > over RSDT.
> > 
> > Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> > ---
> >  lib/acpi.h |   6 ++++
> >  lib/acpi.c | 103 ++++++++++++++++++++++++++++++++---------------------
> >  2 files changed, 68 insertions(+), 41 deletions(-)
> > 
> > diff --git a/lib/acpi.h b/lib/acpi.h
> > index 42a2c16..d80b983 100644
> > --- a/lib/acpi.h
> > +++ b/lib/acpi.h
> > @@ -13,6 +13,7 @@
> >  
> >  #define RSDP_SIGNATURE ACPI_SIGNATURE('R','S','D','P')
> >  #define RSDT_SIGNATURE ACPI_SIGNATURE('R','S','D','T')
> > +#define XSDT_SIGNATURE ACPI_SIGNATURE('X','S','D','T')
> >  #define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
> >  #define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
> >  
> > @@ -56,6 +57,11 @@ struct rsdt_descriptor_rev1 {
> >      u32 table_offset_entry[0];
> >  } __attribute__ ((packed));
> >  
> > +struct acpi_table_xsdt {
> > +    ACPI_TABLE_HEADER_DEF
> > +    u64 table_offset_entry[1];
> 
> nit: This should be "[0]" to match the usage above (in rsdt).
> 
> I was about to suggest using an unspecified size "[]", but after reading
> what the C standard says about it (below), now I'm not sure. was the
> "[1]" needed for something that I'm missing?
> 
> 	106) The length is unspecified to allow for the fact that
> 	implementations may give array members different
> 	alignments according to their lengths.

GCC prefers "flexible array members" (array[]) [1]. Linux has deprecated
the use of zero-length arrays [2]. The kernel docs do make a pretty good
case for flexible array members.

[1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
[2] https://elixir.bootlin.com/linux/v5.18/source/Documentation/process/deprecated.rst#L234

Thanks,
Alex
Nikos Nikoleris June 20, 2022, 11:06 a.m. UTC | #4 
Hi Alex, Ricardo,

Thank you both for the reviews!

On 20/06/2022 09:53, Alexandru Elisei wrote:
> Hi,
> 
> On Fri, Jun 17, 2022 at 05:38:01PM -0700, Ricardo Koller wrote:
>> Hi Nikos,
>>
>> On Fri, May 06, 2022 at 09:55:45PM +0100, Nikos Nikoleris wrote:
>>> XSDT provides pointers to other ACPI tables much like RSDT. However,
>>> contrary to RSDT that provides 32-bit addresses, XSDT provides 64-bit
>>> pointers. ACPI requires that if XSDT is valid then it takes precedence
>>> over RSDT.
>>>
>>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>>> ---
>>>   lib/acpi.h |   6 ++++
>>>   lib/acpi.c | 103 ++++++++++++++++++++++++++++++++---------------------
>>>   2 files changed, 68 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/lib/acpi.h b/lib/acpi.h
>>> index 42a2c16..d80b983 100644
>>> --- a/lib/acpi.h
>>> +++ b/lib/acpi.h
>>> @@ -13,6 +13,7 @@
>>>   
>>>   #define RSDP_SIGNATURE ACPI_SIGNATURE('R','S','D','P')
>>>   #define RSDT_SIGNATURE ACPI_SIGNATURE('R','S','D','T')
>>> +#define XSDT_SIGNATURE ACPI_SIGNATURE('X','S','D','T')
>>>   #define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
>>>   #define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
>>>   
>>> @@ -56,6 +57,11 @@ struct rsdt_descriptor_rev1 {
>>>       u32 table_offset_entry[0];
>>>   } __attribute__ ((packed));
>>>   
>>> +struct acpi_table_xsdt {
>>> +    ACPI_TABLE_HEADER_DEF
>>> +    u64 table_offset_entry[1];
>>
>> nit: This should be "[0]" to match the usage above (in rsdt).
>>
>> I was about to suggest using an unspecified size "[]", but after reading
>> what the C standard says about it (below), now I'm not sure. was the
>> "[1]" needed for something that I'm missing?
>>
>> 	106) The length is unspecified to allow for the fact that
>> 	implementations may give array members different
>> 	alignments according to their lengths.
> 
> GCC prefers "flexible array members" (array[]) [1]. Linux has deprecated
> the use of zero-length arrays [2]. The kernel docs do make a pretty good
> case for flexible array members.
> 
> [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> [2] https://elixir.bootlin.com/linux/v5.18/source/Documentation/process/deprecated.rst#L234
> 

Happy to change this, I don't have a strong preference. To be consistent 
with RSDT we would have to declare:

u64 table_offset_entry[0];

But it might be better to change RSDT as well. Linux kernel declares:

u64 table_offset_entry[1];

but it seems, we would rather have:

u64 table_offset_entry[];

For alignment, we shouldn't be relying on the length specifier, all 
structs in <acpi.h> should be packed.

Thanks,

Nikos
Nikos Nikoleris June 21, 2022, 11:26 a.m. UTC | #5 
Hi Ricardo,

Thanks for the review!

On 18/06/2022 01:38, Ricardo Koller wrote:
> Hi Nikos,
> 
> On Fri, May 06, 2022 at 09:55:45PM +0100, Nikos Nikoleris wrote:
>> XSDT provides pointers to other ACPI tables much like RSDT. However,
>> contrary to RSDT that provides 32-bit addresses, XSDT provides 64-bit
>> pointers. ACPI requires that if XSDT is valid then it takes precedence
>> over RSDT.
>>
>> Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
>> ---
>>   lib/acpi.h |   6 ++++
>>   lib/acpi.c | 103 ++++++++++++++++++++++++++++++++---------------------
>>   2 files changed, 68 insertions(+), 41 deletions(-)
>>
>> diff --git a/lib/acpi.h b/lib/acpi.h
>> index 42a2c16..d80b983 100644
>> --- a/lib/acpi.h
>> +++ b/lib/acpi.h
>> @@ -13,6 +13,7 @@
>>   
>>   #define RSDP_SIGNATURE ACPI_SIGNATURE('R','S','D','P')
>>   #define RSDT_SIGNATURE ACPI_SIGNATURE('R','S','D','T')
>> +#define XSDT_SIGNATURE ACPI_SIGNATURE('X','S','D','T')
>>   #define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
>>   #define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
>>   
>> @@ -56,6 +57,11 @@ struct rsdt_descriptor_rev1 {
>>       u32 table_offset_entry[0];
>>   } __attribute__ ((packed));
>>   
>> +struct acpi_table_xsdt {
>> +    ACPI_TABLE_HEADER_DEF
>> +    u64 table_offset_entry[1];
> 
> nit: This should be "[0]" to match the usage above (in rsdt).
> 
> I was about to suggest using an unspecified size "[]", but after reading
> what the C standard says about it (below), now I'm not sure. was the
> "[1]" needed for something that I'm missing?
> 
> 	106) The length is unspecified to allow for the fact that
> 	implementations may give array members different
> 	alignments according to their lengths.
> 
> 
>> +} __attribute__ ((packed));
>> +
>>   struct fadt_descriptor_rev1
>>   {
>>       ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
>> diff --git a/lib/acpi.c b/lib/acpi.c
>> index de275ca..9b8700c 100644
>> --- a/lib/acpi.c
>> +++ b/lib/acpi.c
>> @@ -38,45 +38,66 @@ static struct rsdp_descriptor *get_rsdp(void)
>>   
>>   void* find_acpi_table_addr(u32 sig)
> 
> nit: This one could also be fixed as well: "void *".
> 
>>   {
>> -    struct rsdp_descriptor *rsdp;
>> -    struct rsdt_descriptor_rev1 *rsdt;
>> -    void *end;
>> -    int i;
>> -
>> -    /* FACS is special... */
>> -    if (sig == FACS_SIGNATURE) {
>> -        struct fadt_descriptor_rev1 *fadt;
>> -        fadt = find_acpi_table_addr(FACP_SIGNATURE);
>> -        if (!fadt) {
>> -            return NULL;
>> -        }
>> -        return (void*)(ulong)fadt->firmware_ctrl;
>> -    }
>> -
>> -    rsdp = get_rsdp();
>> -    if (rsdp == NULL) {
>> -        printf("Can't find RSDP\n");
>> -        return 0;
>> -    }
>> -
>> -    if (sig == RSDP_SIGNATURE) {
>> -        return rsdp;
>> -    }
>> -
>> -    rsdt = (void*)(ulong)rsdp->rsdt_physical_address;
>> -    if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
>> -        return 0;
>> -
>> -    if (sig == RSDT_SIGNATURE) {
>> -        return rsdt;
>> -    }
>> -
>> -    end = (void*)rsdt + rsdt->length;
>> -    for (i=0; (void*)&rsdt->table_offset_entry[i] < end; i++) {
>> -        struct acpi_table *t = (void*)(ulong)rsdt->table_offset_entry[i];
>> -        if (t && t->signature == sig) {
>> -            return t;
>> -        }
>> -    }
>> -   return NULL;
>> +	struct rsdp_descriptor *rsdp;
>> +	struct rsdt_descriptor_rev1 *rsdt;
>> +	struct acpi_table_xsdt *xsdt = NULL;
>> +	void *end;
>> +	int i;
>> +
>> +	/* FACS is special... */
>> +	if (sig == FACS_SIGNATURE) {
>> +		struct fadt_descriptor_rev1 *fadt;
>> +
>> +		fadt = find_acpi_table_addr(FACP_SIGNATURE);
>> +		if (!fadt)
>> +			return NULL;
>> +
>> +		return (void*)(ulong)fadt->firmware_ctrl;
>> +	}
>> +
>> +	rsdp = get_rsdp();
>> +	if (rsdp == NULL) {
>> +		printf("Can't find RSDP\n");
>> +		return 0;
>> +	}
>> +
>> +	if (sig == RSDP_SIGNATURE)
>> +		return rsdp;
>> +
>> +	rsdt = (void *)(ulong)rsdp->rsdt_physical_address;
>> +	if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
>> +		rsdt = NULL;
>> +
>> +	if (sig == RSDT_SIGNATURE)
>> +		return rsdt;
>> +
>> +	if (rsdp->revision > 1)
>> +		xsdt = (void *)(ulong)rsdp->xsdt_physical_address;
>> +	if (!xsdt || xsdt->signature != XSDT_SIGNATURE)
>> +		xsdt = NULL;
>> +
> 
> To simplify this function a bit, finding the xsdt could be moved to some
> kind of init function.

That's a good point, I can add xsdt and rsdt as globals (like we do for 
efi_rsdp) and then initialise them in set_efi_rsdp().

On the other hand, if we would like to avoid the global variables there 
  is only a handful of time we will be calling  find_acpi_table_addr()

> 
>> +	if (sig == XSDT_SIGNATURE)
>> +		return xsdt;
>> +
>> +	// APCI requires that we first try to use XSDT if it's valid,
>> +	//  we use to find other tables, otherwise we use RSDT.
>> +	if (xsdt) {
>> +		end = (void *)(ulong)xsdt + xsdt->length;
>> +		for (i = 0; (void *)&xsdt->table_offset_entry[i] < end; i++) {
>> +			struct acpi_table *t =
>> +				(void *)xsdt->table_offset_entry[i];
>> +			if (t && t->signature == sig)
>> +				return t;
>> +		}
>> +	} else if (rsdt) {
>> +		end = (void *)rsdt + rsdt->length;
>> +		for (i = 0; (void *)&rsdt->table_offset_entry[i] < end; i++) {
>> +			struct acpi_table *t =
>> +				(void *)(ulong)rsdt->table_offset_entry[i];
>> +			if (t && t->signature == sig)
>> +				return t;
>> +		}
>> +	}
> 
> The two for-loops could be moved into some common function, or maybe a
> macro to deal with the fact that it deals with two different structures
> (the rsdt and the xsdt). This is my attempt at making it a function:
> 
> void *scan_system_descriptor_table(void *dt)
> {
> 	int i;
> 	void *end;
> 	/* XXX: Not sure if this is the nicest thing to do, but the rsdt
> 	 * and the xsdt have "length" and "table_offset_entry" at the
> 	 * same offsets. */
> 	struct acpi_table_xsdt *xsdt = dt;
> 
> 	end = (void *)(ulong)xsdt + xsdt->length;
> 	for (i = 0; &xsdt->table_offset_entry[i] < end; i++) {
> 		struct acpi_table *t = (void *)xsdt->table_offset_entry[i];
> 		if (t && t->signature == sig)
> 			return t;
> }
> 

I remember trying something similar but it got a bit messy because 
table_offset_entry is u32 in rsdt and u64 in xsdt. I'll check again if 
there is a way we can improve this in v3.

Thanks,

Nikos

> Thanks,
> Ricardo
> 
>> +
>> +	return NULL;
>>   }
>> -- 
>> 2.25.1
>>
Alexandru Elisei June 21, 2022, 12:25 p.m. UTC | #6 
Hi,

On Mon, Jun 20, 2022 at 12:06:24PM +0100, Nikos Nikoleris wrote:
> Hi Alex, Ricardo,
> 
> Thank you both for the reviews!
> 
> On 20/06/2022 09:53, Alexandru Elisei wrote:
> > Hi,
> > 
> > On Fri, Jun 17, 2022 at 05:38:01PM -0700, Ricardo Koller wrote:
> > > Hi Nikos,
> > > 
> > > On Fri, May 06, 2022 at 09:55:45PM +0100, Nikos Nikoleris wrote:
> > > > XSDT provides pointers to other ACPI tables much like RSDT. However,
> > > > contrary to RSDT that provides 32-bit addresses, XSDT provides 64-bit
> > > > pointers. ACPI requires that if XSDT is valid then it takes precedence
> > > > over RSDT.
> > > > 
> > > > Signed-off-by: Nikos Nikoleris <nikos.nikoleris@arm.com>
> > > > ---
> > > >   lib/acpi.h |   6 ++++
> > > >   lib/acpi.c | 103 ++++++++++++++++++++++++++++++++---------------------
> > > >   2 files changed, 68 insertions(+), 41 deletions(-)
> > > > 
> > > > diff --git a/lib/acpi.h b/lib/acpi.h
> > > > index 42a2c16..d80b983 100644
> > > > --- a/lib/acpi.h
> > > > +++ b/lib/acpi.h
> > > > @@ -13,6 +13,7 @@
> > > >   #define RSDP_SIGNATURE ACPI_SIGNATURE('R','S','D','P')
> > > >   #define RSDT_SIGNATURE ACPI_SIGNATURE('R','S','D','T')
> > > > +#define XSDT_SIGNATURE ACPI_SIGNATURE('X','S','D','T')
> > > >   #define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
> > > >   #define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
> > > > @@ -56,6 +57,11 @@ struct rsdt_descriptor_rev1 {
> > > >       u32 table_offset_entry[0];
> > > >   } __attribute__ ((packed));
> > > > +struct acpi_table_xsdt {
> > > > +    ACPI_TABLE_HEADER_DEF
> > > > +    u64 table_offset_entry[1];
> > > 
> > > nit: This should be "[0]" to match the usage above (in rsdt).
> > > 
> > > I was about to suggest using an unspecified size "[]", but after reading
> > > what the C standard says about it (below), now I'm not sure. was the
> > > "[1]" needed for something that I'm missing?
> > > 
> > > 	106) The length is unspecified to allow for the fact that
> > > 	implementations may give array members different
> > > 	alignments according to their lengths.
> > 
> > GCC prefers "flexible array members" (array[]) [1]. Linux has deprecated
> > the use of zero-length arrays [2]. The kernel docs do make a pretty good
> > case for flexible array members.
> > 
> > [1] https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html
> > [2] https://elixir.bootlin.com/linux/v5.18/source/Documentation/process/deprecated.rst#L234
> > 
> 
> Happy to change this, I don't have a strong preference. To be consistent
> with RSDT we would have to declare:
> 
> u64 table_offset_entry[0];
> 
> But it might be better to change RSDT as well. Linux kernel declares:
> 
> u64 table_offset_entry[1];
> 
> but it seems, we would rather have:
> 
> u64 table_offset_entry[];

Sorry, I didn't think about Linux when I wrote my reply. In my opinion, we
should keep our definitions aligned with Linux for two reasons:

- Makes making changes to the header file and borrowing code from Linux less
  error prone.

- Linux' implementation, even though it might not be ideal, is proven to be
  working.

It might also be that Linux uses table_offset_entry[1] for compatibility
with older compilers, and supporting the same compilers that Linux does
looks like a safe approach for me; I am not aware of an official policy for
kvm-unit-tests regarding compiler versions (might be one, it just that I
don't know about it!).

Thanks,
Alex
diff mbox series

Patch

diff --git a/lib/acpi.h b/lib/acpi.h
index 42a2c16..d80b983 100644
--- a/lib/acpi.h
+++ b/lib/acpi.h
@@ -13,6 +13,7 @@ 
 
 #define RSDP_SIGNATURE ACPI_SIGNATURE('R','S','D','P')
 #define RSDT_SIGNATURE ACPI_SIGNATURE('R','S','D','T')
+#define XSDT_SIGNATURE ACPI_SIGNATURE('X','S','D','T')
 #define FACP_SIGNATURE ACPI_SIGNATURE('F','A','C','P')
 #define FACS_SIGNATURE ACPI_SIGNATURE('F','A','C','S')
 
@@ -56,6 +57,11 @@  struct rsdt_descriptor_rev1 {
     u32 table_offset_entry[0];
 } __attribute__ ((packed));
 
+struct acpi_table_xsdt {
+    ACPI_TABLE_HEADER_DEF
+    u64 table_offset_entry[1];
+} __attribute__ ((packed));
+
 struct fadt_descriptor_rev1
 {
     ACPI_TABLE_HEADER_DEF     /* ACPI common table header */
diff --git a/lib/acpi.c b/lib/acpi.c
index de275ca..9b8700c 100644
--- a/lib/acpi.c
+++ b/lib/acpi.c
@@ -38,45 +38,66 @@  static struct rsdp_descriptor *get_rsdp(void)
 
 void* find_acpi_table_addr(u32 sig)
 {
-    struct rsdp_descriptor *rsdp;
-    struct rsdt_descriptor_rev1 *rsdt;
-    void *end;
-    int i;
-
-    /* FACS is special... */
-    if (sig == FACS_SIGNATURE) {
-        struct fadt_descriptor_rev1 *fadt;
-        fadt = find_acpi_table_addr(FACP_SIGNATURE);
-        if (!fadt) {
-            return NULL;
-        }
-        return (void*)(ulong)fadt->firmware_ctrl;
-    }
-
-    rsdp = get_rsdp();
-    if (rsdp == NULL) {
-        printf("Can't find RSDP\n");
-        return 0;
-    }
-
-    if (sig == RSDP_SIGNATURE) {
-        return rsdp;
-    }
-
-    rsdt = (void*)(ulong)rsdp->rsdt_physical_address;
-    if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
-        return 0;
-
-    if (sig == RSDT_SIGNATURE) {
-        return rsdt;
-    }
-
-    end = (void*)rsdt + rsdt->length;
-    for (i=0; (void*)&rsdt->table_offset_entry[i] < end; i++) {
-        struct acpi_table *t = (void*)(ulong)rsdt->table_offset_entry[i];
-        if (t && t->signature == sig) {
-            return t;
-        }
-    }
-   return NULL;
+	struct rsdp_descriptor *rsdp;
+	struct rsdt_descriptor_rev1 *rsdt;
+	struct acpi_table_xsdt *xsdt = NULL;
+	void *end;
+	int i;
+
+	/* FACS is special... */
+	if (sig == FACS_SIGNATURE) {
+		struct fadt_descriptor_rev1 *fadt;
+
+		fadt = find_acpi_table_addr(FACP_SIGNATURE);
+		if (!fadt)
+			return NULL;
+
+		return (void*)(ulong)fadt->firmware_ctrl;
+	}
+
+	rsdp = get_rsdp();
+	if (rsdp == NULL) {
+		printf("Can't find RSDP\n");
+		return 0;
+	}
+
+	if (sig == RSDP_SIGNATURE)
+		return rsdp;
+
+	rsdt = (void *)(ulong)rsdp->rsdt_physical_address;
+	if (!rsdt || rsdt->signature != RSDT_SIGNATURE)
+		rsdt = NULL;
+
+	if (sig == RSDT_SIGNATURE)
+		return rsdt;
+
+	if (rsdp->revision > 1)
+		xsdt = (void *)(ulong)rsdp->xsdt_physical_address;
+	if (!xsdt || xsdt->signature != XSDT_SIGNATURE)
+		xsdt = NULL;
+
+	if (sig == XSDT_SIGNATURE)
+		return xsdt;
+
+	// APCI requires that we first try to use XSDT if it's valid,
+	//  we use to find other tables, otherwise we use RSDT.
+	if (xsdt) {
+		end = (void *)(ulong)xsdt + xsdt->length;
+		for (i = 0; (void *)&xsdt->table_offset_entry[i] < end; i++) {
+			struct acpi_table *t =
+				(void *)xsdt->table_offset_entry[i];
+			if (t && t->signature == sig)
+				return t;
+		}
+	} else if (rsdt) {
+		end = (void *)rsdt + rsdt->length;
+		for (i = 0; (void *)&rsdt->table_offset_entry[i] < end; i++) {
+			struct acpi_table *t =
+				(void *)(ulong)rsdt->table_offset_entry[i];
+			if (t && t->signature == sig)
+				return t;
+		}
+	}
+
+	return NULL;
 }