Re: [PATCH V8 30/39] vfio-pci: recover from unmap-all-vaddr failure

[Steven Sistare]

On 6/29/2022 6:58 PM, Alex Williamson wrote:
> On Wed, 15 Jun 2022 07:52:17 -0700
> Steve Sistare <steven.sistare@oracle.com> wrote:
> 
>> If vfio_cpr_save fails to unmap all vaddr's, then recover by walking all
>> flat sections to restore the vaddr for each.  Do so by invoking the
>> vfio listener callback, and passing a new "replay" flag that tells it
>> to replay a mapping without re-allocating new userland data structures.
> 
> Is this comment accurate?  I thought we had unwind in the kernel for
> vaddr invalidation, and the notifier here is hooked up to any fault, so
> it's at least misleading regarding vaddr.  

The comment is misleading, I'll fix it.  If there are multiple containers and 
unmap-all fails for some container, we need to remap vaddr for the other
containers for which unmap-all succeeded.

> The replay option really
> needs some documentation in comments.

Will do.

>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  hw/vfio/common.c              | 66 
>> ++++++++++++++++++++++++++++++++-----------
>>  hw/vfio/cpr.c                 | 29 +++++++++++++++++++
>>  include/hw/vfio/vfio-common.h |  2 +-
>>  3 files changed, 80 insertions(+), 17 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index c7d73b6..5f2bd50 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -895,15 +895,35 @@ static bool 
>> vfio_known_safe_misalignment(MemoryRegionSection *section)
>>      return true;
>>  }
>>  
>> +static VFIORamDiscardListener *vfio_find_ram_discard_listener(
>> +    VFIOContainer *container, MemoryRegionSection *section)
>> +{
>> +    VFIORamDiscardListener *vrdl = NULL;
> 
> This initialization was copied from current code, but...
> 
> #define QLIST_FOREACH(var, head, field)                                 \
>         for ((var) = ((head)->lh_first);                                \
>                ...
> 
> it doesn't look necessary.  Thanks,

Sure, will remove it.

- Steve
 
>> +
>> +    QLIST_FOREACH(vrdl, &container->vrdl_list, next) {
>> +        if (vrdl->mr == section->mr &&
>> +            vrdl->offset_within_address_space ==
>> +            section->offset_within_address_space) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!vrdl) {
>> +        hw_error("vfio: Trying to sync missing RAM discard listener");
>> +        /* does not return */
>> +    }
>> +    return vrdl;
>> +}
>> +
[...]