Re: [PATCH v2 4/5] ramfb: make migration conditional

[Laszlo Ersek <lersek@redhat.com>]

On 10/2/23 21:26, Alex Williamson wrote:
> On Mon, 2 Oct 2023 20:24:11 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/2/23 16:41, Alex Williamson wrote:
>>> On Mon, 2 Oct 2023 15:38:10 +0200
>>> Cédric Le Goater <clg@redhat.com> wrote:
>>>   
>>>> On 10/2/23 13:11, marcandre.lureau@redhat.com wrote:  
>>>>> From: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>>
>>>>> RAMFB migration was unsupported until now, let's make it conditional.
>>>>> The following patch will prevent machines <= 8.1 to migrate it.
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>    
>>>> Maybe localize the new 'ramfb_migrate' attribute close to 'enable_ramfb'
>>>> in VFIOPCIDevice. Anyhow,  
>>>
>>> Shouldn't this actually be tied to whether the device is migratable
>>> (which for GVT-g - the only ramfb user afaik - it's not)?  What does it
>>> mean to have a ramfb-migrate=true property on a device that doesn't
>>> support migration, or false on a device that does support migration.  I
>>> don't understand why this is a user controllable property.  Thanks,  
>>
>> The comments in <https://bugzilla.redhat.com/show_bug.cgi?id=1859424>
>> (which are unfortunately not public :/ ) suggest that ramfb migration
>> was simply forgotten when vGPU migration was implemented. So, "now
>> that vGPU migration is done", this should be added.
>>
>> Comment 8 suggests that the following domain XML snippet
>>
>>     <hostdev mode='subsystem' type='mdev' managed='no'
>> model='vfio-pci' display='on' ramfb='on'> <source>
>>         <address uuid='b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>       </source>
>>       <alias name='ua-b155147a-663a-4009-ae7f-e9a96805b3ce'/>
>>       <address type='pci' domain='0x0000' bus='0x07' slot='0x00'
>> function='0x0'/> </hostdev>
>>
>> is migratable, but the ramfb device malfunctions on the destination
>> host.
>>
>> There's also a huge QEMU cmdline in comment#0 of the bug; I've not
>> tried to read that.
>>
>> AIUI BTW the property is not for the user to control, it's just a
>> compat knob for versioned machine types. AIUI those are usually
>> implemented with such (user-visible / -tweakable) device properties.
> 
> If it's not for user control it's unfortunate that we expose it to the
> user at all, but should it at least use the "x-" prefix to indicate that
> it's not intended to be an API?

I *think* it was your commit db32d0f43839 ("vfio/pci: Add option to
disable GeForce quirks", 2018-02-06) that hda introduced me to the "x-"
prefixed properties!

For some reason though, machine type compat knobs are never named like
that, AFAIR.

> It's still odd to think that we can
> have scenarios of a non-migratable vfio device registering a migratable
> ramfb, and vice versa, but I suppose in the end it doesn't matter.

I do think it matters! For one, if migration is not possible with
vfio-pci-nohotplug, then how can QE (or anyone else) *test* the patch
(i.e. that it makes a difference)? In that case, the ramfb_setup() call
from vfio-pci-nohotplug should just open-code "false" for the
"migratable" parameter.

But, more importantly, I think either we're missing something about RHBZ
1859424, or that use case is just plain wrong. Gerd, any comments perhaps?

Migration certainly makes sense for ramfb-standalone though.

Laszlo