Re: [PATCH v2 3/4] virtio-iommu: Support bypass domain

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Tue, Feb 08, 2022 at 02:29:21PM +0100, Eric Auger wrote:
> Hi Jean,
> 
> On 2/8/22 2:09 PM, Dr. David Alan Gilbert wrote:
> > * Jean-Philippe Brucker (jean-philippe@linaro.org) wrote:
> >> On Wed, Feb 02, 2022 at 02:21:37PM +0100, Eric Auger wrote:
> >>>>>> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
> >>>>>> index ec02029bb6..a112428c65 100644
> >>>>>> --- a/hw/virtio/virtio-iommu.c
> >>>>>> +++ b/hw/virtio/virtio-iommu.c
> >>>>>> @@ -43,6 +43,7 @@
> >>>>>>  
> >>>>>>  typedef struct VirtIOIOMMUDomain {
> >>>>>>      uint32_t id;
> >>>>>> +    bool bypass;
> >>>>> I am afraid this will break the migration if you don't change
> >>>>> vmstate_domain.
> >>>>>
> >>>>> See static const VMStateDescription vmstate_domain.
> >>>>> Also you need to migrate the new bypass field.
> >>>>>
> >>>>> Logically we should handle this with a vmstate subsection I think to
> >>>>> handle migration of older devices. However I doubt the device has been
> >>>>> used in production environment supporting migration so my guess is we
> >>>>> may skip that burden and just add the missing field. Adding Juan, Dave &
> >>>>> Peter for advices.
> >>>> I'm not sure about users of this; if no one has used it then yeh; you
> >>>> could bump up the version_id to make it a bit clearer.
> >>> Thank you for your input. Yes to me it sounds OK to only bump the
> >>> version_id while adding the new field.
> >> Ok. Just to make sure we're on the same page, this means we don't support
> >> migration from new->old or old->new instances, since the migration stream
> >> doesn't carry a version ID for the virtio-iommu-device and domain
> >> vmstates, as far as I understand. I also believe backward-incompatible
> >> changes are fine this time around, though I don't have much visibility in
> >> what's being used.
> > I think the stream only has it for top level devices; I've not dug into
> > this device.
> Not sure I get what you meant:
> 
> vmstate_virtio_iommu has a version_id. Also vmstate_domain has one.

These version numbers are not sent as part of the stream, unless I missed
it. On the incoming side, virtio_load() calls vmstate_load_state() for
vmstate_virtio_iommu_device and only looks at the internal version number
(dc->vmsd->version_id), it doesn't check version numbers from the stream.
So if the sender is old and the receiver is new, the stream doesn't have a
bypass field but the receiver will assume it does, and will consider the
stream corrupted.

Since we're not concerned about compatibility for the moment, I think we
could just add the bypass field without bumping the version number, the
behavior will be the same.

Thanks,
Jean