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

[Ani Sinha]

> On 18 Dec 2024, at 11:13 PM, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> 
> 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);
> 
> Shouldn't we use qemu_register_resettable() instead?

OK will do in v3.

> 
>> +}
>> +
>> +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);
>> +}
>> +
>> +static const TypeInfo vmfwupdate_device_info = {
>> +    .name          = TYPE_VMFWUPDATE,
>> +    .parent        = TYPE_DEVICE,
> 
> What is the qdev API used here? Why not use a plain object?

I wrote this taking vmcoreinfo device as starting point. I will leave this as 
is for now unless anyone has strong opinions.

> 
>> +    .instance_size = sizeof(VMFwUpdateState),
>> +    .class_init    = vmfwupdate_device_class_init,
>> +};