Re: [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables

["Rebecca Cran" <rebecca@bsdio.com>]

On 4/17/23 5:46 AM, Corvin Köhne wrote:
> +
> +/**
> +  Get the address of bhyve's ACPI Root System Description Pointer (RSDP).
> +
> +  @param  RsdpPtr             Return pointer to RSDP.
> +
> +  @return EFI_SUCCESS         Bhyve's RSDP successfully found.
> +  @return EFI_NOT_FOUND       Couldn't find bhyve's RSDP.
> +  @return EFI_UNSUPPORTED     Revision is lower than 2.
> +  @return EFI_PROTOCOL_ERROR  Invalid RSDP found.

I think these should be @retval instead of @return ?

> +    if (Rsdp->Revision < 2) {
> +      DEBUG ((DEBUG_INFO, "%a: unsupported RSDP found\n", __FUNCTION__));

You missed converting this __FUNCTION__ to __func__.

> +    //
> +    // For ACPI 1.0/2.0/3.0 the checksum of first 20 bytes should be 0.
> +    // For ACPI 2.0/3.0 the checksum of the entire table should be 0.
> +    //
> +    UINT8  Sum = CalculateCheckSum8 (
> +                   (CONST UINT8 *)Rsdp,
> +                   sizeof (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
> +                   );

Variables should be declared at the top of the function, and initialized 
separately.

> +    if (Sum != 0) {
> +      DEBUG ((
> +        DEBUG_INFO,
> +        "%a: RSDP header checksum not valid: 0x%02x\n",
> +        __func__,
> +        Sum
> +        ));
> +      return EFI_PROTOCOL_ERROR;
> +    }
> +
> +    Sum = CalculateCheckSum8 (
> +            (CONST UINT8 *)Rsdp,
> +            sizeof (EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
> +            );
> +    if (Sum != 0) {
> +      DEBUG ((
> +        DEBUG_INFO,
> +        "%a: RSDP table checksum not valid: 0x%02x\n",
> +        __func__,
> +        Sum
> +        ));
> +      return EFI_PROTOCOL_ERROR;
> +    }
> +
> +    //
> +    // RSDP was found and is valid
> +    //
> +    *RsdpPtr = Rsdp;
> +
> +    return EFI_SUCCESS;
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "%a: RSDP not found\n", __func__));
Should these DEBUG_INFO messages which appear to be warnings/errors use 
DEBUG_WARN or DEBUG_ERROR, instead?


-- 
Rebecca Cran