Re: [PATCH RFC] vfio: Move the saving of the config space to the right place in VFIO migration

[Shenming Lu]

On 2020/12/2 6:21, Alex Williamson wrote:
> On Tue, 1 Dec 2020 14:37:52 +0800
> Shenming Lu <lushenming@huawei.com> wrote:
> 
>> On 2020/12/1 1:03, Alex Williamson wrote:
>>> On Thu, 26 Nov 2020 14:56:17 +0800
>>> Shenming Lu <lushenming@huawei.com> wrote:
>>>   
>>>> Hi,
>>>>
>>>> After reading everyone's opinions, we have a rough idea for this issue.
>>>>
>>>> One key point is whether it is necessary to setup the config space before
>>>> the device can accept further migration data. I think it is decided by
>>>> the vendor driver, so we can simply ask the vendor driver about it in
>>>> .save_setup, which could avoid a lot of unnecessary copies and settings.
>>>> Once we have known the need, we can iterate the config space (before)
>>>> along with the device migration data in .save_live_iterate and
>>>> .save_live_complete_precopy, and if not needed, we can only migrate the
>>>> config space in .save_state.
>>>>
>>>> Another key point is that the interrupt enabling should be after the
>>>> restoring of the interrupt controller (might not only interrupts).
>>>> My solution is to add a subflag at the beginning of the config data
>>>> (right after VFIO_MIG_FLAG_DEV_CONFIG_STATE) to indicate the triggered
>>>> actions on the dst (such as whether to enable interrupts).
>>>>
>>>> Below is it's workflow.
>>>>
>>>> On the save path:
>>>>    In vfio_save_setup():
>>>>    Ask the vendor driver if it needs the config space setup before it
>>>>    can accept further migration data.  
>>>
>>> How does "ask the vendor driver" actually work?  
>>
>> Maybe via a ioctl?
>> Oh, it seems that we have to ask the dst vendor driver (in vfio_load_setup)
>> and send the config data (before) along with the device migration data
>> every time?...
> 
> Migration data streams are unidirectional, otherwise we break offline
> migration.  There are various ways other than an ioctl for a vendor
> driver to advertise requirements, ex. a capability on the migration
> region info, but it's not clear to me that we really need this info yet.

Ok, this is not clear yet.

> 
>>>>            |
>>>>    In vfio_save_iterate() (pre-copy):
>>>>    If *needed*, save the config space which would be setup on the dst
>>>>    before the migration data, but send with a subflag to instruct not
>>>>    to (such as) enable interrupts.  
>>>
>>> If not for triggering things like MSI/X configuration, isn't config
>>> space almost entirely virtual?  What visibility does the vendor driver
>>> have to the VM machine dependencies regarding device interrupt versus
>>> interrupt controller migration?  
>>
>> My thought is that the vendor driver only decides the order of the config
>> space setup and the receiving of the migration data, but leaves the VM
>> machine dependencies to QEMU.
> 
> But again, config space is largely virtual, the vendor driver doesn't
> have access to it, it's only the effects of config space, like
> representing the interrupt mode and configuring it on the device or
> specific registers that QEMU writes through that the vendor driver
> sees.  So how is it that the vendor driver decides the order?

As you said above, a vendor driver could advertise its requirements via
the migration region...

> The
> vendor driver doesn't have visibility to the VM machine dependencies,
> like when the interrupt controller is sufficiently configured to enable
> device interrupts.  It seems that a vendor driver that depends on QEMU
> enabling interrupts at a specific point is inherently dependent on
> assumptions in the machine configuration.

Yeah, there might be more than one dependency.

> 
>>>>            |
>>>>    In vfio_save_complete_precopy() (stop-and-copy, iterable process):
>>>>    The same as that in vfio_save_iterate().
>>>>            |
>>>>    In .save_state (stop-and-copy, non-iterable process):
>>>>    If *needed*, only send a subflag to instruct to enable interrupts.
>>>>    If *not needed*, save the config space and setup everything on the dst. 
>>>>  
>>>
>>> Again, how does the vendor driver have visibility to know when the VM
>>> machine can enable interrupts?  
>>
>> It seems troubling if the vendor driver needs the interrupts to be enabled
>> first...
> 
> Yes.
> 
>>>> Besides the above idea, we might be able to choose to let the vendor 
>>>> driver do
>>>> more: qemu just sends and writes the config data (before) along with the 
>>>> device
>>>> migration data every time, and it's up to the vendor driver to filter 
>>>> out/buffer
>>>> the received data or reorder the settings...  
>>>
>>> There is no vendor driver in QEMU though, so are you suggesting that
>>> QEMU follows a standard protocol and the vendor driver chooses when to
>>> enable specific features?  For instance, QEMU would call SET_IRQS and
>>> the driver would return success, but defer that setup if necessary?
>>> That seems quite troubling as we then have ioctls that behave
>>> differently depending on the device state and we have no error path to
>>> userspace should that setup fail later.  The vendor driver does have
>>> its own data stream for migration, so the vendor driver could tell the
>>> destination version of itself what type of interrupt to use, which
>>> might be sufficient if we were to ignore the latency if QEMU were to
>>> defer interrupt setup until stop-and-copy.  
>>
>> Did you mean that we could only enable MSI-X during the iterable phase, but
>> leave the setup of these unmasked vectors to the non-iterable phase?
>> It looks good to me.
> 
> On x86 the vfio SET_IRQS ioctl is independent of the VM interrupt
> setup, so we could decouple configuring the device interrupts from
> plumbing them through the VM.

On ARM64 the setting of the IRQ bypass in the vfio SET_IRQS ioctl is dependent
on the VM interrupt setup...

> However, doesn't ARM64 require mapping
> an MSI doorbell page via vfio?  Where does is that configured during the
> machine load versus the device iterative phase?

Isn't the mapping of the MSI doorbell arch independent in vfio? -_-

> 
>>> Is the question of when to setup device interrupts versus the interrupt
>>> controller state largely a machine issue within QEMU?  If so, shouldn't
>>> it be at QEMU's determination when to act on the config space
>>> information on the target?  
>>
>> I think it would be simpler if ensuring the proper calling order in QEMU...
>>
>>> IOW, if a vendor driver has a dependency on
>>> interrupt configuration, they need to include it in their own pre-copy
>>> data stream and decouple that dependency from userspace interrupt
>>> configuration via the SET_IRQS ioctl.  Is that possible?  Thanks,
>>>   
>>
>> I don't understand what the decoupling that dependency from userspace
>> interrupt configuration means...
> 
> I mean that the vendor driver cannot depend on when QEMU calls
> SET_IRQS, the vendor specific migration stream contains any information
> the vendor driver needs about interrupt configuration such that QEMU
> can call SET_IRQS whenever it chooses.  Thanks,

Oh, that sounds great.
By this way, it seems that we only need to migrate the QEMU config space in
the non-iterable process once.

> 
> Alex
> 
> .
>