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

[Ani Sinha]

On Thu, Dec 19, 2024 at 10:46 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> 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.

Ah this is where we have a disconnect. I assumed that
> pcms = PC_MACHINE(m_obj)

would return NULL on non-x86.

Seems a better way to do this (as is done in vga.c) is to use
object_dynamic_cast()
How about

diff --git a/hw/misc/vmfwupdate.c b/hw/misc/vmfwupdate.c
index 0e90bd10e1..19d042b929 100644
--- a/hw/misc/vmfwupdate.c
+++ b/hw/misc/vmfwupdate.c
@@ -32,9 +32,11 @@ static inline VMFwUpdateState *vmfwupdate_find(void)
 static uint64_t get_max_fw_size(void)
 {
     Object *m_obj = qdev_get_machine();
-    PCMachineState *pcms = PC_MACHINE(m_obj);
+    MachineState *ms = MACHINE(m_obj);
+    PCMachineState *pcms;
-    if (pcms) {
+    if (object_dynamic_cast(OBJECT(ms), TYPE_X86_MACHINE)) {
+        pcms = PC_MACHINE(m_obj);
         return pcms->max_fw_size;
     } else {
         return 0;

As I said before, if this is not the best way, please suggest an
alternative to get max_fw_size for any platform.

>
> >     What about the FW_CFG_DMA dependency?

If you read the spec,

+ The ``fw-cfg`` interface must support the
+     DMA interface. It may only support the DMA interface for write operations.

So we need that.

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

Well up to you but I have tried to incorporate most of your
suggestions here (as it has been with other patches that you review).
I am off for the year-end break so won't be sending any more versions
of this patch in the next few days. So if you change your mind, feel
free to comment.
Have a good year-end break.