On Thu, Dec 19, 2024 at 10:46 PM Philippe Mathieu-Daudé
<phi...@linaro.org> wrote:
>
> On 19/12/24 18:08, Ani Sinha wrote:
> >
> >
> > On Thu, 19 Dec, 2024, 10:21 pm Philippe Mathieu-Daudé,
> > <phi...@linaro.org <mailto:phi...@linaro.org>> wrote:
> >
> > On 19/12/24 17:16, Ani Sinha wrote:
> > >
> > >
> > > On Thu, 19 Dec, 2024, 9:22 pm Philippe Mathieu-Daudé,
> > <phi...@linaro.org <mailto:phi...@linaro.org>
> > > <mailto:phi...@linaro.org <mailto:phi...@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 <anisi...@redhat.com
> > <mailto:anisi...@redhat.com>
> > > <mailto:anisi...@redhat.com <mailto:anisi...@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.
