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

[Alexander Graf]

On 20.12.24 14:31, Ani Sinha wrote:
> On Fri, Dec 20, 2024 at 5:03 PM Alexander Graf <graf@amazon.com> wrote:
> > On 20.12.24 11:00, Ani Sinha wrote:
> > > > > 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;
> > > For the records, I tested this with arm and the following command line
> > > does not crash QEMU;
> > >
> > > ./qemu-system-arm -machine virt-9.2 -device vmfwupdate
> > >
> > > I have also added a separate functional test to exercise basic device
> > > creation which will be part of v5 when I send it out.
> >
> > You are currently not implementing the reset logic required to actually
> > make vmfwupdate work.
> Yes that is correct and that is by design. The reset logic on CoCo
> depends on the larger piece of work on how to enable reset and
> re-instantiation of the VM without simply terminating. I did not want
> to wait until all those complicated bits were sorted out first. I
> wanted to make sure that the hypervisor/guest interface is put in
> place.


This logic has no dependency on CoCo.


> That means technically, the device should not be
> > instantiable on *any* platform at the moment. For example with the
> > command line above you would advertise the capability to update firmware
> > in fw-cfg, but then not back it by functionality.
> OK so if we wanted to put this work peace meal in smaller chunks, can
> we have an additional capability bit that actually advertizes this
> functionality on certain machine types/platforms?


Maybe. The property we need is that spawning this device on a machine 
type that does not support reset enlightenment for it should fail 
launching. I don't really care how you build that. But today, adding 
-device vmfwupdate should fail on any target system.


> If QEMU were to merge
> > this patch, it would just create a broken device.
> Are you talking about CoCo or non-CoCo?


Always. The guest would see the interface, try to drive it and then we 
ignore its inputs. We have to prevent that from happening if we want to 
merge the base logic without board backend code.

I personally think the board backend code for pc is simple enough that 
it makes sense to fold into this patch though so that you have at least 
one working board.


Alex





Amazon Web Services Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597