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

[Andrew Jones <drjones@redhat.com>]

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