* Re: [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables
[not found] <20230417114601.398383-1-corvink@FreeBSD.org>
@ 2023-04-17 11:53 ` Rebecca Cran
2023-05-03 2:25 ` Rebecca Cran
2023-04-17 12:14 ` Gerd Hoffmann
1 sibling, 1 reply; 7+ messages in thread
From: Rebecca Cran @ 2023-04-17 11:53 UTC (permalink / raw)
To: Corvin Köhne, devel
Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Ard Biesheuvel,
Jiewen Yao, Jordan Justen, Gerd Hoffmann, Peter Grehan
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables
[not found] <20230417114601.398383-1-corvink@FreeBSD.org>
2023-04-17 11:53 ` [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables Rebecca Cran
@ 2023-04-17 12:14 ` Gerd Hoffmann
2023-04-17 12:45 ` Corvin Köhne
1 sibling, 1 reply; 7+ messages in thread
From: Gerd Hoffmann @ 2023-04-17 12:14 UTC (permalink / raw)
To: Corvin Köhne
Cc: devel, Michael D Kinney, Liming Gao, Zhiguang Liu, Ard Biesheuvel,
Jiewen Yao, Jordan Justen, Rebecca Cran, Peter Grehan
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables
2023-04-17 12:14 ` Gerd Hoffmann
@ 2023-04-17 12:45 ` Corvin Köhne
2023-04-17 15:15 ` Gerd Hoffmann
0 siblings, 1 reply; 7+ messages in thread
From: Corvin Köhne @ 2023-04-17 12:45 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: devel, Michael D Kinney, Liming Gao, Zhiguang Liu, Ard Biesheuvel,
Jiewen Yao, Jordan Justen, Rebecca Cran, Peter Grehan
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables
2023-04-17 12:45 ` Corvin Köhne
@ 2023-04-17 15:15 ` Gerd Hoffmann
0 siblings, 0 replies; 7+ messages in thread
From: Gerd Hoffmann @ 2023-04-17 15:15 UTC (permalink / raw)
To: Corvin Köhne
Cc: devel, Michael D Kinney, Liming Gao, Zhiguang Liu, Ard Biesheuvel,
Jiewen Yao, Jordan Justen, Rebecca Cran, Peter Grehan
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables
2023-04-17 11:53 ` [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables Rebecca Cran
@ 2023-05-03 2:25 ` Rebecca Cran
2023-05-03 7:43 ` Corvin Köhne
0 siblings, 1 reply; 7+ messages in thread
From: Rebecca Cran @ 2023-05-03 2:25 UTC (permalink / raw)
To: Corvin Köhne, devel
Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Ard Biesheuvel,
Jiewen Yao, Jordan Justen, Gerd Hoffmann, Peter Grehan
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?
>
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables
2023-05-03 2:25 ` Rebecca Cran
@ 2023-05-03 7:43 ` Corvin Köhne
2023-05-03 14:50 ` Rebecca Cran
0 siblings, 1 reply; 7+ messages in thread
From: Corvin Köhne @ 2023-05-03 7:43 UTC (permalink / raw)
To: Rebecca Cran, devel
Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Ard Biesheuvel,
Jiewen Yao, Jordan Justen, Gerd Hoffmann, Peter Grehan
[-- 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 --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables
2023-05-03 7:43 ` Corvin Köhne
@ 2023-05-03 14:50 ` Rebecca Cran
0 siblings, 0 replies; 7+ messages in thread
From: Rebecca Cran @ 2023-05-03 14:50 UTC (permalink / raw)
To: Corvin Köhne, devel
Cc: Michael D Kinney, Liming Gao, Zhiguang Liu, Ard Biesheuvel,
Jiewen Yao, Jordan Justen, Gerd Hoffmann, Peter Grehan
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-05-03 14:50 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20230417114601.398383-1-corvink@FreeBSD.org>
2023-04-17 11:53 ` [PATCH 1/1] OvmfPkg/BhyveBhf: install bhyve's ACPI tables Rebecca Cran
2023-05-03 2:25 ` Rebecca Cran
2023-05-03 7:43 ` Corvin Köhne
2023-05-03 14:50 ` Rebecca Cran
2023-04-17 12:14 ` Gerd Hoffmann
2023-04-17 12:45 ` Corvin Köhne
2023-04-17 15:15 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox