Re: [PATCH V7 19/29] vfio-pci: cpr part 1 (fd and dma)

[Steven Sistare]

On 3/10/2022 1:35 PM, Alex Williamson wrote:
> On Thu, 10 Mar 2022 10:00:29 -0500
> Steven Sistare <steven.sistare@oracle.com> wrote:
> 
>> On 3/7/2022 5:16 PM, Alex Williamson wrote:
>>> On Wed, 22 Dec 2021 11:05:24 -0800
>>> Steve Sistare <steven.sistare@oracle.com> wrote:
>>>> @@ -1878,6 +1908,18 @@ static int vfio_init_container(VFIOContainer 
>>>> *container, int group_fd,
>>>>  {
>>>>      int iommu_type, ret;
>>>>  
>>>> +    /*
>>>> +     * If container is reused, just set its type and skip the ioctls, as 
>>>> the
>>>> +     * container and group are already configured in the kernel.
>>>> +     * VFIO_TYPE1v2_IOMMU is the only type that supports reuse/cpr.
>>>> +     * If you ever add new types or spapr cpr support, kind reader, please
>>>> +     * also implement VFIO_GET_IOMMU.
>>>> +     */  
>>>
>>> VFIO_CHECK_EXTENSION should be able to tell us this, right?  Maybe the
>>> problem is that vfio_iommu_type1_check_extension() should actually base
>>> some of the details on the instantiated vfio_iommu, ex.
>>>
>>>     switch (arg) {
>>>     case VFIO_TYPE1_IOMMU:
>>>             return (iommu && iommu->v2) ? 0 : 1;
>>>     case VFIO_UNMAP_ALL:
>>>     case VFIO_UPDATE_VADDR:
>>>     case VFIO_TYPE1v2_IOMMU:
>>>             return (iommu && !iommu->v2) ? 0 : 1;
>>>     case VFIO_TYPE1_NESTING_IOMMU:
>>>             return (iommu && !iommu->nesting) ? 0 : 1;
>>>     ...
>>>
>>> We can't support v1 if we've already set a v2 container and vice versa.
>>> There are probably some corner cases and compatibility to puzzle
>>> through, but I wouldn't think we need a new ioctl to check this.  
>>
>> That change makes sense, and may be worth while on its own merits, but does 
>> not
>> solve the problem, which is that qemu will not be able to infer iommu_type in
>> the future if new types are added.  Given:
>>   * a new kernel supporting shiny new TYPE1v3
>>   * old qemu starts and selects TYPE1v2 in vfio_get_iommu_type because it 
>> has no
>>     knowledge of v3
>>   * live update to qemu which supports v3, which will be listed first in 
>> vfio_get_iommu_type.
>>
>> Then the new qemu has no way to infer iommu_type.  If it has code that makes 
>> decisions based on iommu_type (eg, VFIO_SPAPR_TCE_v2_IOMMU in 
>> vfio_container_region_add,
>> or vfio_ram_block_discard_disable, or ...), then new qemu cannot function 
>> correctly.
>>
>> For that, VFIO_GET_IOMMU would be the cleanest solution, to be added the 
>> same time our
>> hypothetical future developer adds TYPE1v3.  The current inability to ask 
>> the kernel
>> "what are you" about a container feels like a bug to me.
> 
> Hmm, I don't think the kernel has an innate responsibility to remind
> the user of a configuration that they've already made.  

No, but it can make userland cleaner.  For example, CRIU checkpoint/restart 
queries
the kernel to save process state, and later makes syscalls to restore it.  
Where the
kernel does not export sufficient information, CRIU must provide interpose 
libraries
so it can remember state internally on its way to the kernel.  And applications 
must
link against the interpose libraries.

> But I also
> don't follow your TYPE1v3 example.  If we added such a type, I imagine
> the switch would change to:
> 
>       switch (arg)
>       case VFIO_TYPE1_IOMMU:
>               return (iommu && (iommu->v2 || iommu->v3) ? 0 : 1;
>       case VFIO_UNMAP_ALL:
>       case VFIO_UPDATE_VADDR:
>               return (iommu && !(iommu-v2 || iommu->v3) ? 0 : 1;
>       case VFIO_TYPE1v2_IOMMU:
>               return (iommu && !iommu-v2) ? 0 : 1;
>       case VFIO_TYPE1v3_IOMMU:
>               return (iommu && !iommu->v3) ? 0 : 1;
>       ...
> 
> How would that not allow exactly the scenario described, ie. new QEMU
> can see that old QEMU left it a v2 IOMMU.

OK, that works as long as the switch returns true for all options before
VFIO_SET_IOMMU is called.  I guess your test for "iommu" above does that,
which I missed before.  If we are on the same page now, I will modify my
comment "please also implement VFIO_GET_IOMMU".

> ...
>>>> +
>>>> +bool vfio_is_cpr_capable(VFIOContainer *container, Error **errp)
>>>> +{
>>>> +    if (!ioctl(container->fd, VFIO_CHECK_EXTENSION, VFIO_UPDATE_VADDR) ||
>>>> +        !ioctl(container->fd, VFIO_CHECK_EXTENSION, VFIO_UNMAP_ALL)) {
>>>> +        error_setg(errp, "VFIO container does not support 
>>>> VFIO_UPDATE_VADDR "
>>>> +                         "or VFIO_UNMAP_ALL");
>>>> +        return false;
>>>> +    } else {
>>>> +        return true;
>>>> +    }
>>>> +}  
>>>
>>> We could have minimally used this where we assumed a TYPE1v2 container.  
>>
>> Are you referring to vfio_init_container (discussed above)?
>> Are you suggesting that, if reused is true, we validate those extensions are
>> present, before setting iommu_type = VFIO_TYPE1v2_IOMMU?
> 
> Yeah, though maybe it's not sufficiently precise to be worthwhile given
> the current kernel behavior.
> 
>>>> +
>>>> +/*
>>>> + * Verify that all containers support CPR, and unmap all dma vaddr's.
>>>> + */
>>>> +int vfio_cpr_save(Error **errp)
>>>> +{
>>>> +    ERRP_GUARD();
>>>> +    VFIOAddressSpace *space;
>>>> +    VFIOContainer *container;
>>>> +
>>>> +    QLIST_FOREACH(space, &vfio_address_spaces, list) {
>>>> +        QLIST_FOREACH(container, &space->containers, next) {
>>>> +            if (!vfio_is_cpr_capable(container, errp)) {
>>>> +                return -1;
>>>> +            }
>>>> +            if (vfio_dma_unmap_vaddr_all(container, errp)) {
>>>> +                return -1;
>>>> +            }
>>>> +        }
>>>> +    }  
>>>
>>> Seems like we ought to validate all containers support CPR before we
>>> start blasting vaddrs.  It looks like qmp_cpr_exec() simply returns if
>>> this fails with no attempt to unwind!  Yikes!  Wouldn't we need to
>>> replay the listeners to remap the vaddrs in case of an error?  
>>
>> Already done.  I refactored that code into a separate patch to tease out some
>> of the complexity:
>>   vfio-pci: recover from unmap-all-vaddr failure
> 
> Sorry, didn't get to that one til after I'd sent comments here.
> 
> ...
>>>> diff --git a/include/migration/cpr.h b/include/migration/cpr.h
>>>> index a4da24e..a4007cf 100644
>>>> --- a/include/migration/cpr.h
>>>> +++ b/include/migration/cpr.h
>>>> @@ -25,4 +25,7 @@ int cpr_state_save(Error **errp);
>>>>  int cpr_state_load(Error **errp);
>>>>  void cpr_state_print(void);
>>>>  
>>>> +int cpr_vfio_save(Error **errp);
>>>> +int cpr_vfio_load(Error **errp);
>>>> +
>>>>  #endif
>>>> diff --git a/migration/cpr.c b/migration/cpr.c
>>>> index 37eca66..cee82cf 100644
>>>> --- a/migration/cpr.c
>>>> +++ b/migration/cpr.c
>>>> @@ -7,6 +7,7 @@
>>>>  
>>>>  #include "qemu/osdep.h"
>>>>  #include "exec/memory.h"
>>>> +#include "hw/vfio/vfio-common.h"
>>>>  #include "io/channel-buffer.h"
>>>>  #include "io/channel-file.h"
>>>>  #include "migration.h"
>>>> @@ -101,7 +102,9 @@ void qmp_cpr_exec(strList *args, Error **errp)
>>>>          error_setg(errp, "cpr-exec requires cpr-save with restart mode");
>>>>          return;
>>>>      }
>>>> -
>>>> +    if (cpr_vfio_save(errp)) {
>>>> +        return;
>>>> +    }  
>>>
>>> Why is vfio so unique that it needs separate handlers versus other
>>> devices?  Thanks,  
>>
>> In earlier patches these functions fiddled with more objects, but at this 
>> point
>> they are simple enough to convert to pre_save and post_load vmstate handlers 
>> for
>> the container and group objects.  However, we would still need to call 
>> special 
>> functons for vfio from qmp_cpr_exec:
>>
>>   * validate all containers support CPR before we start blasting vaddrs
>>     However, I could validate all, in every call to pre_save for each 
>> container.
>>     That would be less efficient, but fits the vmstate model.
> 
> Would it be a better option to mirror the migration blocker support, ie.
> any device that doesn't support cpr registers a blocker and generic
> code only needs to keep track of whether any blockers are registered.

We cannot specifically use migrate_add_blocker(), because it is checked in
the migration specific function migrate_prepare(), in a layer of functions 
above the simpler qemu_save_device_state() used in cpr.  But yes, we could
do something similar for vfio.  Increment a global counter in vfio_realize
if the container does not support cpr, and decrement it when the container is
destroyed.  pre_save could just check the counter.

>>   * restore all vaddr's if qemu_save_device_state fails.
>>     However, I could recover for all containers inside pre_save when one 
>> container fails.
>>     Feels strange touching all objects in a function for one, but there is 
>> no real
>>     downside.
> 
> I'm not as familiar as I should be with migration callbacks, thanks to
> mostly not supporting it for vfio devices, but it seems strange to me
> that there's no existing callback or notifier per device to propagate
> save failure.  Do we not at least get some sort of resume callback in
> that case?

We do not:
    struct VMStateDescription {
        int (*pre_load)(void *opaque);
        int (*post_load)(void *opaque, int version_id);
        int (*pre_save)(void *opaque);
        int (*post_save)(void *opaque);

The handler returns an error, which stops further saves and is propagated back
to the top level caller qemu_save_device_state().

The vast majority of handlers do not have side effects, with no need to unwind 
anything on failure.

This raises another point.  If pre_save succeeds for all the containers,
but fails for some non-vfio object, then the overall operation is abandoned,
but we do not restore the vaddr's.  To plug that hole, we need to call the
unwind code from qmp_cpr_save, or implement your alternative below.

> As an alternative, maybe each container could register a vm change
> handler that would trigger reloading vaddrs if we move to a running
> state and a flag on the container indicates vaddrs were invalidated?
> Thanks,

That works and is modular, but I dislike that it adds checks on the
happy path for a case that will rarely happen, and it pushes recovery from
failure further away from the original failure, which would make debugging
cascading failures more difficult.

- Steve