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

[Parav Pandit <parav@nvidia.com>]

> 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.