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

[Alexander Graf <graf@amazon.com>]

On 14.02.25 16:35, Ani Sinha wrote:
> On Mon, Feb 3, 2025 at 3:50 AM Alexander Graf <graf@amazon.com> wrote:
>> Hey Ani!
>>
>> On 28.01.25 22:31, Ani Sinha wrote:


[...]


>>> diff --git a/hw/core/machine.c b/hw/core/machine.c
>>> index c23b399496..0eaf8aa3ba 100644
>>> --- a/hw/core/machine.c
>>> +++ b/hw/core/machine.c
>>> @@ -34,6 +34,7 @@
>>>    #include "hw/virtio/virtio-pci.h"
>>>    #include "hw/virtio/virtio-net.h"
>>>    #include "hw/virtio/virtio-iommu.h"
>>> +#include "hw/misc/vmfwupdate.h"
>>>    #include "audio/audio.h"
>>>
>>>    GlobalProperty hw_compat_9_2[] = {
>>> @@ -252,6 +253,7 @@ GlobalProperty hw_compat_2_8[] = {
>>>        { "virtio-pci", "x-pcie-pm-init", "off" },
>>>        { "cirrus-vga", "vgamem_mb", "8" },
>>>        { "isa-cirrus-vga", "vgamem_mb", "8" },
>>> +    {TYPE_VMFWUPDATE, "disable", "1"},
>>
>> If vmfwupdate is opt-in anyway, why disable for older machines?
> Some older machines does not have enough slots for new fw-cfg files.
> For those machines, we cannot add this device. Unless we disable the
> device, QEMU would crash on an assertion.
> This particular case was caught because the CI pipeline was failing.
> There might be other cases where we need to disable it but CI does not
> have a test case for it.


Please find a better way to fix that. You can have better error 
propagation when fw-cfg fills up so it's clear that the problem is the 
additional vmfwupdate device. But silently changing the behavior from 
"Device does what I want it to do" to "Device is attached, but not 
functional" based on machine version is a big no-go. You *must* fail 
loudly, because you can not honor the user's wish (expose vmfwupdate).


Alex