Re: [PATCH v11 08/11] vfio/migration: Implement VFIO migration protocol v2

[Alex Williamson]

On Thu, 16 Feb 2023 17:52:33 +0100
Juan Quintela <quintela@redhat.com> wrote:

> Avihai Horon <avihaih@nvidia.com> wrote:
> > On 16/02/2023 17:43, Juan Quintela wrote:  
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> Avihai Horon <avihaih@nvidia.com> wrote:
> >>
> >> Reviewed-by: Juan Quintela <quintela@redhat.com>.
> >>
> >>
> >> 1st comment is a bug, but as you just remove it.
> >>
> >>
> >> Not that it matters a lot in the end (you are removing v1 on the
> >> following patch).
> >>  
> >>> @@ -438,7 +445,13 @@ static bool 
> >>> vfio_devices_all_running_and_mig_active(VFIOContainer *container)
> >>>                   return false;
> >>>               }
> >>>
> >>> -            if (migration->device_state_v1 & 
> >>> VFIO_DEVICE_STATE_V1_RUNNING) {
> >>> +            if (!migration->v2 &&
> >>> +                migration->device_state_v1 & 
> >>> VFIO_DEVICE_STATE_V1_RUNNING) {
> >>> +                continue;
> >>> +            }  
> >> Are you missing here:
> >>
> >>
> >>                 } else {
> >>                     return false;
> >>                 }
> >>
> >> Or I am missunderstanding the code.  
> >
> > I think the code is OK:
> >
> > If the device uses v1 migration and is running, the first if is true
> > and we continue.
> > If the device uses v1 migration and is not running, the first if is
> > false and the second if is false (since the device doesn't use v2
> > migration) and we return false.
> >
> > If the device uses v2 migration and is running, the first if is false
> > and the second if is true, so we continue.
> > If the device uses v2 migration and is not running, first if is false
> > and the second if is false, so we return false.  
> 
> You win O:-)
> 
> I was looking at C level, not at the "semantic" level.
> 
> >>
> >>         size_t size = DIV_ROUND_UP(sizeof(struct vfio_device_feature) +
> >>                                   sizeof(struct 
> >> vfio_device_feature_mig_data_size),
> >>                                   sizeof(uint64_t));
> >>         g_autofree struct vfio_device_feature *feature = g_malloc0(size * 
> >> sizeof(uint64_t));
> >>
> >> I have to reread several times to see what was going on there.
> >>
> >>
> >> And call it a day?  
> >
> > I don't have a strong opinion here.
> > We can do whatever you/Alex like more.  
> 
> I think it is more readable, and we don't put (normally) big chunks in
> the stack, but once told that, who wrotes the code has more to say O:-)

This is not a huge amount of data on the stack, so I'm fine with it as
is.  Thanks,

Alex