Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
> On 28 Mar 2024, at 16:04, Ani Sinha wrote:
>
>
>
>> On 27 Mar 2024, at 19:43, Roy Hopkins wrote:
>>
>> On Wed, 2024-03-27 at 18:58 +0530, Ani Sinha wrote:
>>>
>>>
On 27 Feb 2024, at 20:20, Roy Hopkins wrote:
When using an IGVM file the configuration of the system firmware is
defined by IGVM directives contained in the file. Therefore the default
system firmware should not be initialized when an IGVM file has been
provided.
This commit checks to see if an IGVM file has been provided and, if it
has then the standard system firmware initialization is skipped and any
prepared flash devices are cleaned up.
Signed-off-by: Roy Hopkins
---
hw/i386/pc.c | 12 ++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index f8eb684a49..17bb211708 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -63,6 +63,7 @@
#include "e820_memory_layout.h"
#include "trace.h"
#include CONFIG_DEVICES
+#include "exec/confidential-guest-support.h"
#ifdef CONFIG_XEN_EMU
#include "hw/xen/xen-legacy-backend.h"
@@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
}
}
-/* Initialize PC system firmware */
-pc_system_firmware_init(pcms, rom_memory);
+/*
+ * If this is a confidential guest configured using IGVM then the IGVM
+ * configuration will include the system firmware. In this case do not
+ * initialise PC system firmware.
+ */
+if (!cgs_is_igvm(machine->cgs)) {
+/* Initialize PC system firmware */
+pc_system_firmware_init(pcms, rom_memory);
+}
>>>
>>> Ok so this makes QEMU mot load the default fw as provided in the QEMU
>>> command
>>> line. It does not specify how the packaged fw specified within IGVM would be
>>> processed and loaded. Am I understanding this right?
>>>
>> Yes. Although as suggested by Daniel, I've now changed this so if firmware is
>> specified on the command line in conflict with the IGVM file then an error is
>> displayed.
>
> Does IGVM _must_ mandatorily contain a firmware? If not, then before error is
> displayed, we should check if the file does have a firmware.
>
>> The IGVM file itself describes how the firmware binary is populated
>> into guest memory and launched.
>
> Ok so I looked at the doc here: https://docs.rs/igvm_defs/0.1.7/igvm_defs/
> I do not see anything related to firmware in the spec.
> Secondly, how the firmware is to be loaded is hypervisor specific. So there
> must be QEMU specific implementation to load the firmware from IGVM.
> Secondly, should a new directive (and associated definitions) be introduced
> that instructs hypervisors to load the firmware present in IGVM?
> Something is missing here it seems and I am willing to dive into filling the
> missing parts.
I guess what I was missing was
https://github.com/roy-hopkins/buildigvm/blob/main/src/ovmf_firmware.rs .
Seems something like this is supposed to load ovmf image into memory.
Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
> On 27 Mar 2024, at 19:43, Roy Hopkins wrote:
>
> On Wed, 2024-03-27 at 18:58 +0530, Ani Sinha wrote:
>>
>>
>>> On 27 Feb 2024, at 20:20, Roy Hopkins wrote:
>>>
>>> When using an IGVM file the configuration of the system firmware is
>>> defined by IGVM directives contained in the file. Therefore the default
>>> system firmware should not be initialized when an IGVM file has been
>>> provided.
>>>
>>> This commit checks to see if an IGVM file has been provided and, if it
>>> has then the standard system firmware initialization is skipped and any
>>> prepared flash devices are cleaned up.
>>>
>>> Signed-off-by: Roy Hopkins
>>> ---
>>> hw/i386/pc.c | 12 ++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>>> index f8eb684a49..17bb211708 100644
>>> --- a/hw/i386/pc.c
>>> +++ b/hw/i386/pc.c
>>> @@ -63,6 +63,7 @@
>>> #include "e820_memory_layout.h"
>>> #include "trace.h"
>>> #include CONFIG_DEVICES
>>> +#include "exec/confidential-guest-support.h"
>>>
>>> #ifdef CONFIG_XEN_EMU
>>> #include "hw/xen/xen-legacy-backend.h"
>>> @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
>>> }
>>> }
>>>
>>> -/* Initialize PC system firmware */
>>> -pc_system_firmware_init(pcms, rom_memory);
>>> +/*
>>> + * If this is a confidential guest configured using IGVM then the IGVM
>>> + * configuration will include the system firmware. In this case do not
>>> + * initialise PC system firmware.
>>> + */
>>> +if (!cgs_is_igvm(machine->cgs)) {
>>> +/* Initialize PC system firmware */
>>> +pc_system_firmware_init(pcms, rom_memory);
>>> +}
>>
>> Ok so this makes QEMU mot load the default fw as provided in the QEMU command
>> line. It does not specify how the packaged fw specified within IGVM would be
>> processed and loaded. Am I understanding this right?
>>
> Yes. Although as suggested by Daniel, I've now changed this so if firmware is
> specified on the command line in conflict with the IGVM file then an error is
> displayed.
Does IGVM _must_ mandatorily contain a firmware? If not, then before error is
displayed, we should check if the file does have a firmware.
> The IGVM file itself describes how the firmware binary is populated
> into guest memory and launched.
Ok so I looked at the doc here: https://docs.rs/igvm_defs/0.1.7/igvm_defs/
I do not see anything related to firmware in the spec.
Secondly, how the firmware is to be loaded is hypervisor specific. So there
must be QEMU specific implementation to load the firmware from IGVM. Secondly,
should a new directive (and associated definitions) be introduced that
instructs hypervisors to load the firmware present in IGVM?
Something is missing here it seems and I am willing to dive into filling the
missing parts.
>
Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
On Wed, 2024-03-27 at 18:58 +0530, Ani Sinha wrote:
>
>
> > On 27 Feb 2024, at 20:20, Roy Hopkins wrote:
> >
> > When using an IGVM file the configuration of the system firmware is
> > defined by IGVM directives contained in the file. Therefore the default
> > system firmware should not be initialized when an IGVM file has been
> > provided.
> >
> > This commit checks to see if an IGVM file has been provided and, if it
> > has then the standard system firmware initialization is skipped and any
> > prepared flash devices are cleaned up.
> >
> > Signed-off-by: Roy Hopkins
> > ---
> > hw/i386/pc.c | 12 ++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f8eb684a49..17bb211708 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -63,6 +63,7 @@
> > #include "e820_memory_layout.h"
> > #include "trace.h"
> > #include CONFIG_DEVICES
> > +#include "exec/confidential-guest-support.h"
> >
> > #ifdef CONFIG_XEN_EMU
> > #include "hw/xen/xen-legacy-backend.h"
> > @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
> > }
> > }
> >
> > - /* Initialize PC system firmware */
> > - pc_system_firmware_init(pcms, rom_memory);
> > + /*
> > + * If this is a confidential guest configured using IGVM then the IGVM
> > + * configuration will include the system firmware. In this case do not
> > + * initialise PC system firmware.
> > + */
> > + if (!cgs_is_igvm(machine->cgs)) {
> > + /* Initialize PC system firmware */
> > + pc_system_firmware_init(pcms, rom_memory);
> > + }
>
> Ok so this makes QEMU mot load the default fw as provided in the QEMU command
> line. It does not specify how the packaged fw specified within IGVM would be
> processed and loaded. Am I understanding this right?
>
Yes. Although as suggested by Daniel, I've now changed this so if firmware is
specified on the command line in conflict with the IGVM file then an error is
displayed. The IGVM file itself describes how the firmware binary is populated
into guest memory and launched.
Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
> On 27 Feb 2024, at 20:20, Roy Hopkins wrote:
>
> When using an IGVM file the configuration of the system firmware is
> defined by IGVM directives contained in the file. Therefore the default
> system firmware should not be initialized when an IGVM file has been
> provided.
>
> This commit checks to see if an IGVM file has been provided and, if it
> has then the standard system firmware initialization is skipped and any
> prepared flash devices are cleaned up.
>
> Signed-off-by: Roy Hopkins
> ---
> hw/i386/pc.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f8eb684a49..17bb211708 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -63,6 +63,7 @@
> #include "e820_memory_layout.h"
> #include "trace.h"
> #include CONFIG_DEVICES
> +#include "exec/confidential-guest-support.h"
>
> #ifdef CONFIG_XEN_EMU
> #include "hw/xen/xen-legacy-backend.h"
> @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
> }
> }
>
> -/* Initialize PC system firmware */
> -pc_system_firmware_init(pcms, rom_memory);
> +/*
> + * If this is a confidential guest configured using IGVM then the IGVM
> + * configuration will include the system firmware. In this case do not
> + * initialise PC system firmware.
> + */
> +if (!cgs_is_igvm(machine->cgs)) {
> +/* Initialize PC system firmware */
> +pc_system_firmware_init(pcms, rom_memory);
> +}
Ok so this makes QEMU mot load the default fw as provided in the QEMU command
line. It does not specify how the packaged fw specified within IGVM would be
processed and loaded. Am I understanding this right?
Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
On Fri, 2024-03-01 at 16:54 +, Daniel P. Berrangé wrote:
> On Tue, Feb 27, 2024 at 02:50:12PM +, Roy Hopkins wrote:
> > When using an IGVM file the configuration of the system firmware is
> > defined by IGVM directives contained in the file. Therefore the default
> > system firmware should not be initialized when an IGVM file has been
> > provided.
> >
> > This commit checks to see if an IGVM file has been provided and, if it
> > has then the standard system firmware initialization is skipped and any
> > prepared flash devices are cleaned up.
> >
> > Signed-off-by: Roy Hopkins
> > ---
> > hw/i386/pc.c | 12 ++--
> > 1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index f8eb684a49..17bb211708 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -63,6 +63,7 @@
> > #include "e820_memory_layout.h"
> > #include "trace.h"
> > #include CONFIG_DEVICES
> > +#include "exec/confidential-guest-support.h"
> >
> > #ifdef CONFIG_XEN_EMU
> > #include "hw/xen/xen-legacy-backend.h"
> > @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
> > }
> > }
> >
> > - /* Initialize PC system firmware */
> > - pc_system_firmware_init(pcms, rom_memory);
> > + /*
> > + * If this is a confidential guest configured using IGVM then the IGVM
> > + * configuration will include the system firmware. In this case do not
> > + * initialise PC system firmware.
> > + */
> > + if (!cgs_is_igvm(machine->cgs)) {
> > + /* Initialize PC system firmware */
> > + pc_system_firmware_init(pcms, rom_memory);
> > + }
>
> If the user has given explicit pflash config I think we should be
> reporting an error for the invalid configuration, rather than
> silently ignoring their mistake.
>
Ok. In that case, I'll replace this patch and move the check into
pc_system_firmware_init() in "pc_sysfw.c". I can then check for the presence of
an IGVM file after the firmware configuration has completed, generating an error
if pflash has been configured when an IGVM file is provided.
> >
> > option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> > memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> > --
> > 2.43.0
> >
> >
>
> With regards,
> Daniel
Many thanks,
Roy
Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM
On Tue, Feb 27, 2024 at 02:50:12PM +, Roy Hopkins wrote:
> When using an IGVM file the configuration of the system firmware is
> defined by IGVM directives contained in the file. Therefore the default
> system firmware should not be initialized when an IGVM file has been
> provided.
>
> This commit checks to see if an IGVM file has been provided and, if it
> has then the standard system firmware initialization is skipped and any
> prepared flash devices are cleaned up.
>
> Signed-off-by: Roy Hopkins
> ---
> hw/i386/pc.c | 12 ++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index f8eb684a49..17bb211708 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -63,6 +63,7 @@
> #include "e820_memory_layout.h"
> #include "trace.h"
> #include CONFIG_DEVICES
> +#include "exec/confidential-guest-support.h"
>
> #ifdef CONFIG_XEN_EMU
> #include "hw/xen/xen-legacy-backend.h"
> @@ -1023,8 +1024,15 @@ void pc_memory_init(PCMachineState *pcms,
> }
> }
>
> -/* Initialize PC system firmware */
> -pc_system_firmware_init(pcms, rom_memory);
> +/*
> + * If this is a confidential guest configured using IGVM then the IGVM
> + * configuration will include the system firmware. In this case do not
> + * initialise PC system firmware.
> + */
> +if (!cgs_is_igvm(machine->cgs)) {
> +/* Initialize PC system firmware */
> +pc_system_firmware_init(pcms, rom_memory);
> +}
If the user has given explicit pflash config I think we should be
reporting an error for the invalid configuration, rather than
silently ignoring their mistake.
>
> option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE,
> --
> 2.43.0
>
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
