Re: [PATCH 6/9] i386/pc: Skip initialization of system FW when using IGVM

[Ani Sinha]

> On 28 Mar 2024, at 16:04, Ani Sinha <anisinha@redhat.com> wrote:
> 
> 
> 
>> On 27 Mar 2024, at 19:43, Roy Hopkins <roy.hopkins@suse.com> wrote:
>> 
>> On Wed, 2024-03-27 at 18:58 +0530, Ani Sinha wrote:
>>> 
>>> 
>>>> On 27 Feb 2024, at 20:20, Roy Hopkins <roy.hopkins@suse.com> 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 <roy.hopkins@suse.com>
>>>> ---
>>>> 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.