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

[Ricardo Koller <ricarkol@google.com>]

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
>