Re: [PATCH v1 3/8] device-context: Define the device context fields for device migration

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Nov 02, 2023 at 03:06:49PM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, November 2, 2023 8:24 PM
> > 
> > On Thu, Nov 02, 2023 at 02:40:57PM +0000, Parav Pandit wrote:
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Thursday, November 2, 2023 7:52 PM
> > > >
> > > > On Sun, Oct 08, 2023 at 02:25:50PM +0300, Parav Pandit wrote:
> > > > > +\begin{lstlisting}
> > > > > +struct virtio_dev_ctx_pci_vq_cfg {
> > > > > +        le16 vq_index;
> > > > > +        le16 queue_size;
> > > > > +        le16 queue_msix_vector;
> > > > > +        le64 queue_descÍ
> > > > > +        le64 queue_driverÍ
> > > > > +        le64 queue_deviceÍ
> > > > > +};
> > > > > +\end{lstlisting}
> > > > > +
> > > > > +One or multiple entries of PCI Virtqueue Configuration Context
> > > > > +may exist, each such entry corresponds to a unique virtqueue
> > > > > +identified by the
> > > > \field{vq_index}.
> > > >
> > > > So consider this example. In practice it is quite possible that
> > > > driver is in the process of specifying e.g. queue_desc, and it set
> > > > queue_desc_hi but not queue_desc_lo. Then what is queue_desc?
> > > > Just a combination of a legal value of queue_desc_hi and illegal one
> > > > of queue_desc_lo? This makes no sense.
> > > > queue_desc is fundamentally undefined until queue is enabled.
> > >
> > > Sure, in next read of the device context the updated value will reflect.
> > > The destination will not work on this vq anyway as the device mode is freeze.
> > > The whole device context is not atomic, so having one field like this as
> > nonatomic similar to others is ok.
> > >
> > > In this example queue_enabled with the partial write should be still set to 0.
> > >
> > > >
> > > > This is why I suggest that we have records that match the transport.
> > > > Offset in structure can then we used as a tag and so we do not need
> > > > to come up with new definitions for each single thing.
> > > >
> > > If I understood you correctly, you prefer to transfer virtio config space as offset
> > and value as tag?
> > > If so, how tag helps if it still transfers partial value?
> > 
> > For example:
> > 
> > struct virtio_pci_common_cfg {
> >         /* About the whole device. */
> >         __le32 device_feature_select;   /* read-write */
> >         __le32 device_feature;          /* read-only */
> >         __le32 guest_feature_select;    /* read-write */
> >         __le32 guest_feature;           /* read-write */
> >         __le16 msix_config;             /* read-write */
> >         __le16 num_queues;              /* read-only */
> >         __u8 device_status;             /* read-write */
> >         __u8 config_generation;         /* read-only */
> > 
> >         /* About a specific virtqueue. */
> >         __le16 queue_select;            /* read-write */
> > 
> >         __le16 queue_size;              /* read-write, power of 2. */
> >         __le16 queue_msix_vector;       /* read-write */
> >         __le16 queue_enable;            /* read-write */
> >         __le16 queue_notify_off;        /* read-only */
> >         __le32 queue_desc_lo;           /* read-write */
> >         __le32 queue_desc_hi;           /* read-write */
> >         __le32 queue_avail_lo;          /* read-write */
> >         __le32 queue_avail_hi;          /* read-write */
> >         __le32 queue_used_lo;           /* read-write */
> >         __le32 queue_used_hi;           /* read-write */
> > };
> > 
> > We would have:
> >     tag: 32 (queue_desc_lo), len: 4
> >     tag: 34 (queue_desc_hi), len: 4
> > 
> So tag is offset. Ok.
> > 
> > The point is that the values programmed just map 1:1 to what is exposed in the
> > transport.
> > 
> For queue addresses, 
> > This means that the structure is different for different transports btw.
> > 
> > 
> > 
> > 
> > 
> > 
> > > > And, this is only an instance of the general principle: do not have
> > > > two definitions of the same thing. In fact I'd argue our transport
> > > > structures are an example of a bad design and the cost is that less
> > > > used ones like mmio and ccw sometimes lag behind on features.
> > >
> > > I totally agree on not duplicating it.
> > >
> > > For 64 VQs their content of struct virtio_dev_ctx_pci_vq_cfg is behind 8
> > registers.
> > > So for them there has contained in their own struct such as struct
> > virtio_dev_ctx_pci_vq_cfg, right?
> > >
> > > And current struct virtio_pci_common_cfg is giving the current snap shot of
> > register file.
> > >
> > > This is why there is some duplication.
> > > To avoid duplication of this registers, we will have to bisect each field of it and
> > omit these 3 address registers.
> > >
> > > Not able to see the gain of that overhead. Do you?
> > 
> > Not bisect. My idea is to just have each register in its own tag+length field. And
> > the gain is that we easily add fields without any pain and any duplication and
> > special documentation effort.
> We have to transport all the vq fields located behind the common config registers as struct anyway, isnât it?
> And if we use tag for virtio common config space (instead of struct), why there won't be duplication? This is the part I still miss.

Not sure I understand the question.  The point is that when we add a new
field to common config we don't need to also add it to the migration
format - it has an offset and that automatically defines the format.

-- 
MST