Re: [PATCH 2/2 gnumach] ACPI: Support XSDT (ACPI >= v2.0)

[Samuel Thibault]

Hello,

Damien Zammit, le mer. 24 janv. 2024 03:52:03 +0000, a ecrit:
> diff --git a/i386/i386at/acpi_parse_apic.c b/i386/i386at/acpi_parse_apic.c
> index 9cd861ed..7b377d33 100644
> --- a/i386/i386at/acpi_parse_apic.c
> +++ b/i386/i386at/acpi_parse_apic.c
> @@ -99,27 +98,33 @@ acpi_check_signature(const uint8_t table_signature[], 
> const char *real_signature
>   *
>   * Preconditions: RSDP pointer must not be NULL.
>   *
> - * Returns 0 if correct.
> + * Returns 1 if ACPI 1.0 and sets sdt_base
> + * Returns 2 if ACPI >= 2.0 and sets sdt_base
>   */
>  static int8_t
> -acpi_check_rsdp(struct acpi_rsdp *rsdp)
> +acpi_check_rsdp(struct acpi_rsdp2 *rsdp, phys_addr_t *sdt_base)
>  {
> -    uint32_t checksum;
>      int is_rsdp;
>  
>      /* Check if rsdp signature match with the ACPI RSDP signature. */
> -    is_rsdp = acpi_check_signature(rsdp->signature, ACPI_RSDP_SIG, 
> 8*sizeof(uint8_t));
> +    is_rsdp = acpi_check_signature(rsdp->v1.signature, ACPI_RSDP_SIG, 
> 8*sizeof(uint8_t));
>  
>      if (is_rsdp != ACPI_SUCCESS)
>          return ACPI_BAD_SIGNATURE;
>  
> -    /* If match, calculates rdsp checksum and check It. */
> -    checksum = acpi_checksum(rsdp, sizeof(struct acpi_rsdp));
> -
> -    if (checksum != 0)
> -        return ACPI_BAD_CHECKSUM;

Don't we want to still check the checksum?
(possibly fix it for the acpi 2.0 case)

> +    if (rsdp->v1.revision == 0) {
> +        // ACPI 1.0
> +        *sdt_base = rsdp->v1.rsdt_addr;
> +        printf("ACPI v1.0\n");
> +        return 1;
> +    } else if (rsdp->v1.revision == 2) {
> +        // ACPI >= 2.0
> +        *sdt_base = rsdp->xsdt_addr;
> +        printf("ACPI >= v2.0\n");
> +        return 2;
> +    }
>  
> -    return ACPI_SUCCESS;
> +    return ACPI_NO_RSDP;
>  }
>  
>  /*
> @@ -147,38 +152,43 @@ acpi_check_rsdp_align(void *addr)
>   *
>   * Preconditions: The start address (addr) must be aligned.
>   *
> - * Returns the reference to rsdp structure if success, NULL if failure.
> + * Returns the physical address of rsdp structure if success, 0 if failure.
>   */
> -static struct acpi_rsdp*
> -acpi_search_rsdp(void *addr, uint32_t length)
> +static phys_addr_t
> +acpi_search_rsdp(void *addr, uint32_t length, int *is_64bit)
>  {
>      void *end;
> +    int version = 0;

There is no need to initialize version.

> +    phys_addr_t sdt_base = 0;
>  
>      /* Search RDSP in memory space between addr and addr+lenght. */
>      for (end = addr+length; addr < end; addr += ACPI_RSDP_ALIGN) {
>  
>          /* Check if the current memory block stores the RDSP. */
> -        if ((addr != NULL) && (acpi_check_rsdp(addr) == ACPI_SUCCESS)) {
> -            /* If yes, return RSDP address */
> -            return (struct acpi_rsdp*) addr;
> +        if ((addr != NULL) && ((version = acpi_check_rsdp(addr, &sdt_base)) 
> > 0)) {
> +            /* If yes, return RSDT/XSDT address */
> +            *is_64bit = 0;
> +            if (version == 2)
> +                *is_64bit = 1;

You can make this *is_64bit = version == 2;

> @@ -449,16 +527,9 @@ acpi_apic_parse_table(struct acpi_apic *apic)
>  static int
>  acpi_apic_setup(struct acpi_apic *apic)
>  {
> -    int apic_checksum;
>      ApicLocalUnit* lapic_unit;
>      uint8_t ncpus, nioapics;
>  
> -    /* Check the checksum of the APIC */
> -    apic_checksum = acpi_checksum(apic, apic->header.length);
> -
> -    if(apic_checksum != 0)
> -        return ACPI_BAD_CHECKSUM;

Here as well we'd rather keep checking the checksum?

Samuel