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

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

[Rebecca Cran]

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

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

[Rebecca Cran]

I applied the patch and tested it. The UEFI Shell command "acpiview" 
reports an error. I enabled the "acpiview" command with the patch:


diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
index d0d2712c5662..0c04e4834cf7 100644
--- a/OvmfPkg/Bhyve/BhyveX64.dsc
+++ b/OvmfPkg/Bhyve/BhyveX64.dsc
@@ -785,6 +785,7 @@ [Components]
NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1CommandsLib.inf
NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1CommandsLib.inf
NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1CommandsLib.inf
+ 
NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCommandLib.inf
  !if $(NETWORK_IP6_ENABLE) == TRUE
NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2CommandsLib.inf
  !endif


The bhyve command I used is:

bhyve -AHP -s 0:0,hostbridge -s 1:0,lpc -s 2:0,virtio-net,tap1 -s 
3:0,virtio-blk,./guest.img -s 
4:0,ahci-cd,/home/bcran/FreeBSD-14.0-CURRENT-amd64-20230427-60167184abd5-262599-disc1.iso 
-c 4 -m 1G -s 29,fbuf,tcp=0.0.0.0:5900,w=1280,h=1024,wait -s 
30,xhci,tablet -l 
bootrom,/home/bcran/src/uefi/edk2/Build/BhyveX64/DEBUG_GCC5/FV/BHYVE_CODE.fd,/home/bcran/src/uefi/edk2/Build/BhyveX64/DEBUG_GCC5/FV/BHYVE_VARS.fd 
-l com1,stdio guest


MADT                                 :
   Signature                          : APIC
   Length                             : 114
   Revision                           : 1
   Checksum                           : 0x66
   Oem ID                             : BHYVE
   Oem Table ID                       : BVAPIC
   Oem Revision                       : 0x1
   Creator ID                         : BASL
   Creator Revision                   : 0x20220504
   Local Interrupt Controller Address : 0xFEE00000
   Flags                              : 0x1
   PROCESSOR LOCAL APIC               :
     Type                             : 0x0
     Length                           : 8
     ACPI Processor UID               : 0x0
     APIC ID                          : 0x0
     Flags                            : 0x1
       Enabled                        : 1
       Online Capable                 : 0
       Reserved                       : 0

   PROCESSOR LOCAL APIC               :
     Type                             : 0x0
     Length                           : 8
     ACPI Processor UID               : 0x1
     APIC ID                          : 0x1
     Flags                            : 0x1
       Enabled                        : 1
       Online Capable                 : 0
       Reserved                       : 0

   PROCESSOR LOCAL APIC               :
     Type                             : 0x0
     Length                           : 8
     ACPI Processor UID               : 0x2
     APIC ID                          : 0x2
     Flags                            : 0x1
       Enabled                        : 1
       Online Capable                 : 0
       Reserved                       : 0

   PROCESSOR LOCAL APIC               :
     Type                             : 0x0
     Length                           : 8
     ACPI Processor UID               : 0x3
     APIC ID                          : 0x3
     Flags                            : 0x1
       Enabled                        : 1
       Online Capable                 : 0
       Reserved                       : 0

   IO APIC                            :
     Type                             : 0x1
     Length                           : 12
     I/O APIC ID                      : 0x0
     Reserved                         : 0x0
     I/O APIC Address                 : 0xFEC00000
     Global System Interrupt Base     : 0x0
   INTERRUPT SOURCE OVERRIDE          :
     Type                             : 0x2
     Length                           : 10
     Bus                              : 0x0
     Source                           : 0x0
     Global System Interrupt          : 0x2
     Flags                            : 0x5
   INTERRUPT SOURCE OVERRIDE          :
     Type                             : 0x2
     Length                           : 10
     Bus                              : 0x0
     Source                           : 0x9
     Global System Interrupt          : 0x9
     Flags                            : 0xF
ERROR: Unknown Interrupt Controller Structure, Type = 4, Length = 6


-- 

Rebecca Cran


On 4/17/23 05:53, Rebecca Cran wrote:
> 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?
>
>

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

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 6688 bytes --]

On Tue, 2023-05-02 at 20:25 -0600, Rebecca Cran wrote:
> I applied the patch and tested it. The UEFI Shell command "acpiview" 
> reports an error. I enabled the "acpiview" command with the patch:
> 
> 
> diff --git a/OvmfPkg/Bhyve/BhyveX64.dsc b/OvmfPkg/Bhyve/BhyveX64.dsc
> index d0d2712c5662..0c04e4834cf7 100644
> --- a/OvmfPkg/Bhyve/BhyveX64.dsc
> +++ b/OvmfPkg/Bhyve/BhyveX64.dsc
> @@ -785,6 +785,7 @@ [Components]
> NULL|ShellPkg/Library/UefiShellDebug1CommandsLib/UefiShellDebug1Comma
> ndsLib.inf
> NULL|ShellPkg/Library/UefiShellInstall1CommandsLib/UefiShellInstall1C
> ommandsLib.inf
> NULL|ShellPkg/Library/UefiShellNetwork1CommandsLib/UefiShellNetwork1C
> ommandsLib.inf
> + 
> NULL|ShellPkg/Library/UefiShellAcpiViewCommandLib/UefiShellAcpiViewCo
> mmandLib.inf
>   !if $(NETWORK_IP6_ENABLE) == TRUE
> NULL|ShellPkg/Library/UefiShellNetwork2CommandsLib/UefiShellNetwork2C
> ommandsLib.inf
>   !endif
> 
> 
> The bhyve command I used is:
> 
> bhyve -AHP -s 0:0,hostbridge -s 1:0,lpc -s 2:0,virtio-net,tap1 -s 
> 3:0,virtio-blk,./guest.img -s 
> 4:0,ahci-cd,/home/bcran/FreeBSD-14.0-CURRENT-amd64-20230427-
> 60167184abd5-262599-disc1.iso 
> -c 4 -m 1G -s 29,fbuf,tcp=0.0.0.0:5900,w=1280,h=1024,wait -s 
> 30,xhci,tablet -l 
> bootrom,/home/bcran/src/uefi/edk2/Build/BhyveX64/DEBUG_GCC5/FV/BHYVE_
> CODE.fd,/home/bcran/src/uefi/edk2/Build/BhyveX64/DEBUG_GCC5/FV/BHYVE_
> VARS.fd 
> -l com1,stdio guest
> 
> 
> MADT                                 :
>    Signature                          : APIC
>    Length                             : 114
>    Revision                           : 1
>    Checksum                           : 0x66
>    Oem ID                             : BHYVE
>    Oem Table ID                       : BVAPIC
>    Oem Revision                       : 0x1
>    Creator ID                         : BASL
>    Creator Revision                   : 0x20220504
>    Local Interrupt Controller Address : 0xFEE00000
>    Flags                              : 0x1
>    PROCESSOR LOCAL APIC               :
>      Type                             : 0x0
>      Length                           : 8
>      ACPI Processor UID               : 0x0
>      APIC ID                          : 0x0
>      Flags                            : 0x1
>        Enabled                        : 1
>        Online Capable                 : 0
>        Reserved                       : 0
> 
>    PROCESSOR LOCAL APIC               :
>      Type                             : 0x0
>      Length                           : 8
>      ACPI Processor UID               : 0x1
>      APIC ID                          : 0x1
>      Flags                            : 0x1
>        Enabled                        : 1
>        Online Capable                 : 0
>        Reserved                       : 0
> 
>    PROCESSOR LOCAL APIC               :
>      Type                             : 0x0
>      Length                           : 8
>      ACPI Processor UID               : 0x2
>      APIC ID                          : 0x2
>      Flags                            : 0x1
>        Enabled                        : 1
>        Online Capable                 : 0
>        Reserved                       : 0
> 
>    PROCESSOR LOCAL APIC               :
>      Type                             : 0x0
>      Length                           : 8
>      ACPI Processor UID               : 0x3
>      APIC ID                          : 0x3
>      Flags                            : 0x1
>        Enabled                        : 1
>        Online Capable                 : 0
>        Reserved                       : 0
> 
>    IO APIC                            :
>      Type                             : 0x1
>      Length                           : 12
>      I/O APIC ID                      : 0x0
>      Reserved                         : 0x0
>      I/O APIC Address                 : 0xFEC00000
>      Global System Interrupt Base     : 0x0
>    INTERRUPT SOURCE OVERRIDE          :
>      Type                             : 0x2
>      Length                           : 10
>      Bus                              : 0x0
>      Source                           : 0x0
>      Global System Interrupt          : 0x2
>      Flags                            : 0x5
>    INTERRUPT SOURCE OVERRIDE          :
>      Type                             : 0x2
>      Length                           : 10
>      Bus                              : 0x0
>      Source                           : 0x9
>      Global System Interrupt          : 0x9
>      Flags                            : 0xF
> ERROR: Unknown Interrupt Controller Structure, Type = 4, Length = 6
> 
> 

Hi Rebecca,

thanks for checking. Type 4 is the local ACPI NMI entry. See
https://github.com/freebsd/freebsd-src/blob/1d7355248b5cf3b0d09247c6c7c4ff2d40c39945/usr.sbin/bhyve/acpi.c#L627-L636

It's described in ACPI spec 6.5 chapter 5.2.12.7.


-- 
Kind regards,
Corvin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Rebecca Cran]

On 5/3/23 01:43, Corvin Köhne wrote:
>
> thanks for checking. Type 4 is the local ACPI NMI entry. See
> https://github.com/freebsd/freebsd-src/blob/1d7355248b5cf3b0d09247c6c7c4ff2d40c39945/usr.sbin/bhyve/acpi.c#L627-L636
>
> It's described in ACPI spec 6.5 chapter 5.2.12.7.

Thanks! I'll send a patch to add support for it to acpiview.


-- 

Rebecca Cran

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

[Gerd Hoffmann]

On Mon, Apr 17, 2023 at 01:46:01PM +0200, Corvin Köhne wrote:
> It's much easier to create configuration dependend ACPI tables for bhyve
> than for OVMF. For this reason, don't use the statically created ACPI
> tables provided by OVMF. Instead prefer the dynamically created ACPI
> tables of bhyve. If bhyve provides no ACPI tables or we are unable to
> detect those, fall back to OVMF tables.
> 
> Ideally, we use the qemu fwcfg interface to pass the ACPI tables from
> bhyve to OVMF. bhyve will support this in the future. However, current
> bhyve executables don't support passing ACPI tables by the qemu fwcfg
> interface. They just copy the ACPI into main memory. For that reason,
> pick up the ACPI tables from main memory.
> 
> Implementation is similar to OvmfPkg/XenAcpiPlatformDxe/Xen.c.

Can both Xen and Bhyve share the same implementation?

Given in both cases RAM is scanned for the RSDP I'd expect it should
not be very hard and we avoid duplicating the code.

take care,
  Gerd

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

[Corvin Köhne]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

On Mon, 2023-04-17 at 14:14 +0200, Gerd Hoffmann wrote:
> On Mon, Apr 17, 2023 at 01:46:01PM +0200, Corvin Köhne wrote:
> > It's much easier to create configuration dependend ACPI tables for
> > bhyve
> > than for OVMF. For this reason, don't use the statically created
> > ACPI
> > tables provided by OVMF. Instead prefer the dynamically created
> > ACPI
> > tables of bhyve. If bhyve provides no ACPI tables or we are unable
> > to
> > detect those, fall back to OVMF tables.
> > 
> > Ideally, we use the qemu fwcfg interface to pass the ACPI tables
> > from
> > bhyve to OVMF. bhyve will support this in the future. However,
> > current
> > bhyve executables don't support passing ACPI tables by the qemu
> > fwcfg
> > interface. They just copy the ACPI into main memory. For that
> > reason,
> > pick up the ACPI tables from main memory.
> > 
> > Implementation is similar to OvmfPkg/XenAcpiPlatformDxe/Xen.c.
> 
> Can both Xen and Bhyve share the same implementation?
> 
> Given in both cases RAM is scanned for the RSDP I'd expect it should
> not be very hard and we avoid duplicating the code.
> 
> take care,
>   Gerd
> 

Hi Gerd,

thanks for your reply. It's mostly the same code. Only the start and
end of the scanned range differ. So, it does make sense to share the
same implementation.
Where's the right place for this shared implementation? Is it
"OvmfPkg/Library"?


-- 
Kind regards,
Corvin

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Gerd Hoffmann]

> Hi Gerd,
> 
> thanks for your reply. It's mostly the same code. Only the start and
> end of the scanned range differ. So, it does make sense to share the
> same implementation.
> Where's the right place for this shared implementation? Is it
> "OvmfPkg/Library"?

Yes.

take care,
  Gerd