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.
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?
Note that having a capability bit also helps us to test each machine / platforms separately as we roll this out. The capability will be disabled for all platforms and machines initially and incrementally enabled for those machines and platforms for which we have tested the feature to work.
If QEMU were to merge
> this patch, it would just create a broken device.
Are you talking about CoCo or non-CoCo?
Maybe for non coco case which is a lot simpler, we can add the reset bits as a part of this patchset.
>
> Please make sure that the vmfwupdate device can only be instantiated on
> machine types that it has full support for.
>
>
> 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