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


The point is that the values programmed just map 1:1 to
what is exposed in the transport.

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.

-- 
MST