qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Philippe Mathieu-Daudé
Subject: Re: [PATCH v2] hw/misc/vmfwupdate: Introduce hypervisor fw-cfg interface support
Date: Thu, 19 Dec 2024 18:16:04 +0100
User-agent: Mozilla Thunderbird

On 19/12/24 18:08, Ani Sinha wrote:


On Thu, 19 Dec, 2024, 10:21 pm Philippe Mathieu-Daudé, <philmd@linaro.org <mailto:philmd@linaro.org>> wrote:

    On 19/12/24 17:16, Ani Sinha wrote:
     >
     >
     > On Thu, 19 Dec, 2024, 9:22 pm Philippe Mathieu-Daudé,
    <philmd@linaro.org <mailto:philmd@linaro.org>
     > <mailto:philmd@linaro.org <mailto:philmd@linaro.org>>> wrote:
     >
     >     On 16/12/24 12:48, Ani Sinha wrote:
     >
     >      > diff --git a/hw/misc/meson.build b/hw/misc/meson.build
     >      > index d02d96e403..4c5bdb0de2 100644
     >      > --- a/hw/misc/meson.build
     >      > +++ b/hw/misc/meson.build
     >      > @@ -148,6 +148,8 @@ specific_ss.add(when: 'CONFIG_MAC_VIA',
     >     if_true: files('mac_via.c'))
     >      >   specific_ss.add(when: 'CONFIG_MIPS_CPS', if_true:
     >     files('mips_cmgcr.c', 'mips_cpc.c'))
     >      >   specific_ss.add(when: 'CONFIG_MIPS_ITU', if_true:
     >     files('mips_itu.c'))
     >      >
     >      > +specific_ss.add(when: 'CONFIG_FW_CFG_DMA', if_true:
     >     files('vmfwupdate.c'))
     >
     >     FW_CFG_DMA is offered by multiple targets ...:
     >
     >     $ git grep -w FW_CFG_DMA
     >     hw/arm/Kconfig:19:    select FW_CFG_DMA
     >     hw/i386/Kconfig:82:    select FW_CFG_DMA
     >     hw/i386/Kconfig:113:    select FW_CFG_DMA
     >     hw/loongarch/Kconfig:22:    select FW_CFG_DMA
     >     hw/riscv/Kconfig:59:    select FW_CFG_DMA
     >
     >      > diff --git a/hw/misc/vmfwupdate.c b/hw/misc/vmfwupdate.c
     >      > new file mode 100644
     >      > index 0000000000..1e29a610c0
     >      > --- /dev/null
     >      > +++ b/hw/misc/vmfwupdate.c
     >      > @@ -0,0 +1,157 @@
     >      > +/*
     >      > + * Guest driven VM boot component update device
     >      > + * For details and specification, please look at docs/specs/
     >     vmfwupdate.rst.
     >      > + *
     >      > + * Copyright (C) 2024 Red Hat, Inc.
     >      > + *
     >      > + * Authors: Ani Sinha <anisinha@redhat.com
    <mailto:anisinha@redhat.com>
     >     <mailto:anisinha@redhat.com <mailto:anisinha@redhat.com>>>
     >      > + *
     >      > + * This work is licensed under the terms of the GNU GPL,
    version
     >     2 or later.
     >      > + * See the COPYING file in the top-level directory.
     >      > + *
     >      > + */
     >      > +
     >      > +#include "qemu/osdep.h"
     >      > +#include "qapi/error.h"
     >      > +#include "qemu/module.h"
     >      > +#include "sysemu/reset.h"
     >      > +#include "hw/nvram/fw_cfg.h"
     >      > +#include "hw/i386/pc.h"
     >
     >     ... however ...
     >
     >      > +#include "hw/qdev-properties.h"
     >      > +#include "hw/misc/vmfwupdate.h"
     >      > +#include "qemu/error-report.h"
     >      > +
     >      > +static void fw_update_reset(void *dev)
     >      > +{
     >      > +    /* a NOOP at present */
     >      > +    return;
     >      > +}
     >      > +
     >      > +
     >      > +static uint64_t get_max_fw_size(void)
     >      > +{
     >      > +    Object *m_obj = qdev_get_machine();
     >      > +    PCMachineState *pcms = PC_MACHINE(m_obj);
     >      > +
     >      > +    if (pcms) {
     >      > +        return pcms->max_fw_size;
     >
     >     ... this code depends on x86/PC.
     >
     >     Could it be wiser to add a new VM_FWUPDATE Kconfig
     >     symbol, having it depending on FW_CFG_DMA && I386?
     >
     >
     > There is no reason why vmfwupdate would be limited to x86 only.
    There is
     > minimal support needed from hypervisor side for this mechanism. That
     > mechanism has little dependency on specific platform.

    OK, then please remove that PCMachineState use


That is used because the max_fw_size property only exists for pc machines. Do you have a better idea to get this value in a platform agnostic way? Like I said in the previous reply I do not know how to get this value reliably for all machines. If anyone has better ideas, I'm all ears.

Either add the I386 dependency or don't use PC_MACHINE, because on
non-x86 targets PC_MACHINE(qdev_get_machine()) will crash.

    What about the FW_CFG_DMA dependency?


I wasted enough time on this so I'll stop reviewing this work.

Regards,

Phil.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]