Re: [PATCH 2/2] hw/i386: support loading OVMF using -bios too
On Sun, Jan 16, 2022 at 10:05:58PM +0100, Philippe Mathieu-Daudé wrote: > On 14/1/22 19:07, Michael S. Tsirkin wrote: > > On Thu, Jan 13, 2022 at 04:55:11PM +, Daniel P. Berrangé wrote: > > > Traditionally the OVMF firmware has been loaded using the pflash > > > mechanism. This is because it is usually provided as a pair of > > > files, one read-only containing the code and one writable to > > > provided persistence of non-volatile firmware variables. > > > > > > The AMD SEV build of EDK2, however, is provided as a single > > > file that contains only the code. This is intended to be used > > > read-only and explicitly does not provide any ability for > > > persistance of non-volatile firmware variables. While it is > > > possible to configure this with the pflash mechanism, by only > > > providing one of the 2 pflash blobs, conceptually it is a > > > little strange to use pflash if there won't be any persistent > > > data. > > It certainly would be simpler to have a ROM for the CODE part. > IIUC using CFI pflash allows the firmware to poll the underlying > device size. > > > > A stateless OVMF build can be loaded with -bios, however, QEMU > > > does not currently initialize SEV in that scenario. This patch > > > introduces the call needed for SEV initialization of the > > > firmware. > > > > > > Signed-off-by: Daniel P. Berrangé > > > --- > > > hw/i386/x86.c | 5 + > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > > > index b84840a1bb..c79d84936f 100644 > > > --- a/hw/i386/x86.c > > > +++ b/hw/i386/x86.c > > > @@ -45,6 +45,7 @@ > > > #include "target/i386/cpu.h" > > > #include "hw/i386/topology.h" > > > #include "hw/i386/fw_cfg.h" > > > +#include "hw/i386/pc.h" > > > #include "hw/intc/i8259.h" > > > #include "hw/rtc/mc146818rtc.h" > > > #include "target/i386/sev.h" > > > > This builds fine because there's a stub in pc_sysfw_ovmf-stubs.c > > > > The unfortunate thing about this however is that it's too easy to pull > > in a PC dependency, and people building with CONFIG_PC will not notice > > until it breaks for others. > > > > Is it time we split pc.h further and had pc_sysfw_ovmf.h ? > > While "pc*" is specific to the PC machines, "x86*" is shared between > PC and microvm. "pc.h" must not be included in "x86.c". The shared > method introduced in the previous patch becomes > x86_system_ovmf_initialize_sev(). The dual pflash mechanism is proper > to OVMF, so having this method in "x86.h" seems correct. > > Phil. Well. E.g. pc_system_parse_ovmf_flash is defined in hw/i386/pc_sysfw_ovmf.c and declared in pc.h. If you want to move all pc_sysfw_ovmf.c declarations to x86.h that might be fine, but please do not split declarations between multiple headers, that's messy. And really, when in doubt do a separate header is a good rule. -- MST
Re: [PATCH 2/2] hw/i386: support loading OVMF using -bios too
On 14/1/22 19:07, Michael S. Tsirkin wrote: On Thu, Jan 13, 2022 at 04:55:11PM +, Daniel P. Berrangé wrote: Traditionally the OVMF firmware has been loaded using the pflash mechanism. This is because it is usually provided as a pair of files, one read-only containing the code and one writable to provided persistence of non-volatile firmware variables. The AMD SEV build of EDK2, however, is provided as a single file that contains only the code. This is intended to be used read-only and explicitly does not provide any ability for persistance of non-volatile firmware variables. While it is possible to configure this with the pflash mechanism, by only providing one of the 2 pflash blobs, conceptually it is a little strange to use pflash if there won't be any persistent data. It certainly would be simpler to have a ROM for the CODE part. IIUC using CFI pflash allows the firmware to poll the underlying device size. A stateless OVMF build can be loaded with -bios, however, QEMU does not currently initialize SEV in that scenario. This patch introduces the call needed for SEV initialization of the firmware. Signed-off-by: Daniel P. Berrangé --- hw/i386/x86.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/i386/x86.c b/hw/i386/x86.c index b84840a1bb..c79d84936f 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -45,6 +45,7 @@ #include "target/i386/cpu.h" #include "hw/i386/topology.h" #include "hw/i386/fw_cfg.h" +#include "hw/i386/pc.h" #include "hw/intc/i8259.h" #include "hw/rtc/mc146818rtc.h" #include "target/i386/sev.h" This builds fine because there's a stub in pc_sysfw_ovmf-stubs.c The unfortunate thing about this however is that it's too easy to pull in a PC dependency, and people building with CONFIG_PC will not notice until it breaks for others. Is it time we split pc.h further and had pc_sysfw_ovmf.h ? While "pc*" is specific to the PC machines, "x86*" is shared between PC and microvm. "pc.h" must not be included in "x86.c". The shared method introduced in the previous patch becomes x86_system_ovmf_initialize_sev(). The dual pflash mechanism is proper to OVMF, so having this method in "x86.h" seems correct. Phil.
Re: [PATCH 2/2] hw/i386: support loading OVMF using -bios too
On Thu, Jan 13, 2022 at 04:55:11PM +, Daniel P. Berrangé wrote: > Traditionally the OVMF firmware has been loaded using the pflash > mechanism. This is because it is usually provided as a pair of > files, one read-only containing the code and one writable to > provided persistence of non-volatile firmware variables. > > The AMD SEV build of EDK2, however, is provided as a single > file that contains only the code. This is intended to be used > read-only and explicitly does not provide any ability for > persistance of non-volatile firmware variables. While it is > possible to configure this with the pflash mechanism, by only > providing one of the 2 pflash blobs, conceptually it is a > little strange to use pflash if there won't be any persistent > data. > > A stateless OVMF build can be loaded with -bios, however, QEMU > does not currently initialize SEV in that scenario. This patch > introduces the call needed for SEV initialization of the > firmware. > > Signed-off-by: Daniel P. Berrangé > --- > hw/i386/x86.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index b84840a1bb..c79d84936f 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -45,6 +45,7 @@ > #include "target/i386/cpu.h" > #include "hw/i386/topology.h" > #include "hw/i386/fw_cfg.h" > +#include "hw/i386/pc.h" > #include "hw/intc/i8259.h" > #include "hw/rtc/mc146818rtc.h" > #include "target/i386/sev.h" This builds fine because there's a stub in pc_sysfw_ovmf-stubs.c The unfortunate thing about this however is that it's too easy to pull in a PC dependency, and people building with CONFIG_PC will not notice until it breaks for others. Is it time we split pc.h further and had pc_sysfw_ovmf.h ? > @@ -1157,6 +1158,10 @@ void x86_bios_rom_init(MachineState *ms, const char > *default_firmware, > memory_region_add_subregion(rom_memory, > (uint32_t)(-bios_size), > bios); > + > +pc_system_ovmf_initialize_sev( > +rom_ptr((uint32_t)-bios_size, bios_size), > +bios_size); Just curious about the formatting here: pc_system_ovmf_initialize_sev(rom_ptr((uint32_t)-bios_size, bios_size), bios_size); would be prettier ... > } > > bool x86_machine_is_smm_enabled(const X86MachineState *x86ms) > -- > 2.33.1
[PATCH 2/2] hw/i386: support loading OVMF using -bios too
Traditionally the OVMF firmware has been loaded using the pflash mechanism. This is because it is usually provided as a pair of files, one read-only containing the code and one writable to provided persistence of non-volatile firmware variables. The AMD SEV build of EDK2, however, is provided as a single file that contains only the code. This is intended to be used read-only and explicitly does not provide any ability for persistance of non-volatile firmware variables. While it is possible to configure this with the pflash mechanism, by only providing one of the 2 pflash blobs, conceptually it is a little strange to use pflash if there won't be any persistent data. A stateless OVMF build can be loaded with -bios, however, QEMU does not currently initialize SEV in that scenario. This patch introduces the call needed for SEV initialization of the firmware. Signed-off-by: Daniel P. Berrangé --- hw/i386/x86.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/i386/x86.c b/hw/i386/x86.c index b84840a1bb..c79d84936f 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -45,6 +45,7 @@ #include "target/i386/cpu.h" #include "hw/i386/topology.h" #include "hw/i386/fw_cfg.h" +#include "hw/i386/pc.h" #include "hw/intc/i8259.h" #include "hw/rtc/mc146818rtc.h" #include "target/i386/sev.h" @@ -1157,6 +1158,10 @@ void x86_bios_rom_init(MachineState *ms, const char *default_firmware, memory_region_add_subregion(rom_memory, (uint32_t)(-bios_size), bios); + +pc_system_ovmf_initialize_sev( +rom_ptr((uint32_t)-bios_size, bios_size), +bios_size); } bool x86_machine_is_smm_enabled(const X86MachineState *x86ms) -- 2.33.1
