Re: [PATCH v2] hw/misc/vmfwupdate: Introduce hypervisor fw-cfg interface support

[Ani Sinha]

> On 17 Dec 2024, at 4:11 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> On 17/12/24 11:06, Ani Sinha wrote:
>>> On 16 Dec 2024, at 8:35 PM, Philippe Mathieu-Daudé <philmd@linaro.org> 
>>> wrote:
>>> 
>>> Hi Ani,
>>> 
>>> On 16/12/24 12:48, Ani Sinha wrote:
>>>> VM firmware update is a mechanism where the virtual machines can use their
>>>> preferred and trusted firmware image in their execution environment without
>>>> having to depend on a untrusted party to provide the firmware bundle. This 
>>>> is
>>>> particularly useful for confidential virtual machines that are deployed in 
>>>> the
>>>> cloud where the tenant and the cloud provider are two different entities. 
>>>> In
>>>> this scenario, virtual machines can bring their own trusted firmware image
>>>> bundled as a part of their filesystem (using UKIs for example[1]) and then 
>>>> use
>>>> this hypervisor interface to update to their trusted firmware image. This 
>>>> also
>>>> allows the guests to have a consistent measurements on the firmware image.
>>>> This change introduces basic support for the fw-cfg based hypervisor 
>>>> interface
>>>> and the corresponding device. The change also includes the
>>>> specification document for this interface. The interface is made generic
>>>> enough so that guests are free to use their own ABI to pass required
>>>> information between initial and trusted execution contexts (where they are
>>>> running their own trusted firmware image) without the hypervisor getting
>>>> involved in between. In subsequent patches, we will introduce other minimal
>>>> changes on the hypervisor that are required to make the mechanism work.
>>>> [1] See systemd pull requests https://github.com/systemd/systemd/pull/35091
>>>> and https://github.com/systemd/systemd/pull/35281 for some discussions on
>>>> how we can bundle firmware image within an UKI.
>>>> CC: Alex Graf <graf@amazon.com>
>>>> CC: Paolo Bonzini <pbonzini@redhat.com>
>>>> CC: Gerd Hoffman <kraxel@redhat.com>
>>>> CC: Igor Mammedov <imammedo@redhat.com>
>>>> CC: Vitaly Kuznetsov <vkuznets@redhat.com>
>>>> Signed-off-by: Ani Sinha <anisinha@redhat.com>
>>>> ---
>>>>  MAINTAINERS                  |   9 ++
>>>>  docs/specs/index.rst         |   1 +
>>>>  docs/specs/vmfwupdate.rst    | 109 ++++++++++++++++++++++++
>>>>  hw/misc/meson.build          |   2 +
>>>>  hw/misc/vmfwupdate.c         | 157 +++++++++++++++++++++++++++++++++++
>>>>  include/hw/misc/vmfwupdate.h | 103 +++++++++++++++++++++++
>>>>  6 files changed, 381 insertions(+)
>>>>  create mode 100644 docs/specs/vmfwupdate.rst
>>>>  create mode 100644 hw/misc/vmfwupdate.c
>>>>  create mode 100644 include/hw/misc/vmfwupdate.h
> 
> 
>>>> +static void vmfwupdate_realize(DeviceState *dev, Error **errp)
>>>> +{
>>>> +    VMFwUpdateState *s = VMFWUPDATE(dev);
>>>> +    FWCfgState *fw_cfg = fw_cfg_find();
>>>> +
>>>> +    /* multiple devices are not supported */
>>>> +    if (!vmfwupdate_find()) {
>>>> +        error_setg(errp, "at most one %s device is permitted",
>>>> +                   TYPE_VMFWUPDATE);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* fw_cfg with DMA support is necessary to support this device */
>>>> +    if (!fw_cfg || !fw_cfg_dma_enabled(fw_cfg)) {
>>>> +        error_setg(errp, "%s device requires fw_cfg",
>>>> +                   TYPE_VMFWUPDATE);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    memset(&s->fw_blob, 0, sizeof(s->fw_blob));
>>>> +    memset(&s->opaque_blobs, 0, sizeof(s->opaque_blobs));
>>>> +
>>>> +    fw_cfg_add_file_callback(fw_cfg, FILE_VMFWUPDATE_OBLOB,
>>>> +                             NULL, NULL, s,
>>>> +                             &s->opaque_blobs,
>>>> +                             sizeof(s->opaque_blobs),
>>>> +                             false);
>>>> +
>>>> +    fw_cfg_add_file_callback(fw_cfg, FILE_VMFWUPDATE_FWBLOB,
>>>> +                             NULL, fw_blob_write, s,
>>>> +                             &s->fw_blob,
>>>> +                             sizeof(s->fw_blob),
>>>> +                             false);
>>>> +
>>>> +    /*
>>>> +     * Add global capability fw_cfg file. This will be used by the guest 
>>>> to
>>>> +     * check capability of the hypervisor.
>>>> +     */
>>>> +    s->capability = cpu_to_le16(CAP_VMFWUPD_MASK | VMFWUPDATE_CAP_EDKROM);
>>>> +    fw_cfg_add_file(fw_cfg, FILE_VMFWUPDATE_CAP,
>>>> +                    &s->capability, sizeof(s->capability));
>>>> +
>>>> +    s->plat_bios_size = get_max_fw_size(); /* for non-pc, this is 0 */
>>>> +    /* size of bios region for the platform - read only by the guest */
>>>> +    fw_cfg_add_file(fw_cfg, FILE_VMFWUPDATE_BIOS_SIZE,
>>>> +                    &s->plat_bios_size, sizeof(s->plat_bios_size));
>>>> +    /*
>>>> +     * add fw cfg control file to disable the hypervisor interface.
>>>> +     */
>>>> +    fw_cfg_add_file_callback(fw_cfg, FILE_VMFWUPDATE_CONTROL,
>>>> +                             NULL, NULL, s,
>>>> +                             &s->disable,
>>>> +                             sizeof(s->disable),
>>>> +                             false);
>>>> +    /*
>>>> +     * This device requires to register a global reset because it is
>>>> +     * not plugged to a bus (which, as its QOM parent, would reset it).
>>>> +     */
>>>> +    qemu_register_reset(fw_update_reset, dev);
>>>> +}
>>>> +
>>>> +static Property vmfwupdate_properties[] = {
>>>> +    DEFINE_PROP_UINT8("disable", VMFwUpdateState, disable, 0),
>>>> +    DEFINE_PROP_END_OF_LIST(),
>>>> +};
>>>> +
>>>> +static void vmfwupdate_device_class_init(ObjectClass *klass, void *data)
>>>> +{
>>>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>>>> +
>>>> +    /* we are not interested in migration - so no need to populate 
>>>> dc->vmsd */
>>>> +    dc->desc = "VM firmware blob update device";
>>>> +    dc->realize = vmfwupdate_realize;
>>>> +    dc->hotpluggable = false;
>>>> +    device_class_set_props(dc, vmfwupdate_properties);
>>>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>>> 
>>> How is this device instantiated?
>> Something like this:
>> $ ./qemu-system-x86_64 -device vmfwupdate
>> VNC server running on ::1:5900
> 
> But this device is not marked as allowed to be created on the
> command line with:
> 
>    dc->user_creatable = true;
> 
> Am I missing something?

Isnt’ it true by default? See device_class_init(). Only when it’s a private 
device we need to set it explicitly to false.

Let me know if its me who is missing something :-)

> 
>> And we can maybe add a basic test for this scenario:
>> $ ./qemu-system-x86_64 -device vmfwupdate -device vmfwupdate
>> qemu-system-x86_64: -device vmfwupdate: at most one vmfwupdate device is 
>> permitted
>> To exercise the fwcfg files, guest support is needed as I said above.