RE: [virtio-comment] 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: Monday, October 9, 2023 9:25 PM
> 
> On Mon, Oct 09, 2023 at 04:15:01AM +0000, Parav Pandit wrote:
> > Hi Michael,
> >
> > > From: virtio-comment@lists.oasis-open.org
> > > <virtio-comment@lists.oasis- open.org> On Behalf Of Michael S.
> > > Tsirkin
> > > Sent: Sunday, October 8, 2023 5:12 PM
> >
> > [..]
> > > > +\begin{tabularx}{\textwidth}{ |l||l|X| } \hline type & Name &
> > > > +Description \\ \hline \hline
> > > > +0x0 & VIRTIO_DEV_CTX_PCI_COMMON_RUNTIME_CFG & Provides
> common
> > > > +configuration space of device for PCI transport \\ \hline
> > > > +0x1 & VIRTIO_DEV_CTX_DEV_CFG_LAYOUT & Provides device specific
> > > > +configuration layout \\ \hline
> > > > +0x2 & VIRTIO_DEV_CTX_DEV_FEATURES & Provides device features \\
> > > > +\hline
> > > > +0x3 & VIRTIO_DEV_CTX_PCI_VQ_CFG & Provides Virtqueue
> > > > +configuration for PCI transport \\ \hline
> > > > +0x4 & VIRTIO_DEV_CTX_VQ_SPLIT_RUNTIME_CFG & Provides Queue run
> > > time
> > > > +state \\ \hline
> > > > +0x5 & VIRTIO_DEV_CTX_VQ_SPLIT_DEV_OWN_DESC & Provides list of
> > > > +virtqueue descriptors owned by device  \\ \hline
> > > > +0x6 - 0xFFFFFFFF & - & Reserved for future types \\ \hline
> > > > +\end{tabularx}
> > >
> > >
> > > I don't think this is enough, e.g. virtio net has internal state
> > > controlled thought CVQ commands. how do you intend to
> > > address/migrate these?
> > >
> > Post this series, the 32-bit type field will be split into two ranges.
> > First range (existing) to cover common content across all device type.
> > Second range to contain device specific content, containing non internal fields
> such as fields setup by the guest directly over CVQ.
> 
> How will all this be added though? You probably have a clear picture in your
> head but I (and likely other tc members) don't.
>
An example is,
Range 0 to 0xffff are reserved for common device type.
0x1_0000 to 0x1_ffff is per device type.
So say for net device,
0x1_0000 represents, RSS configuration.
0x1_0001 will represents, flow filter configuration
0x1_0002 will represents, stats.

For crypto device,
0x1_000 represents session information.

Etc.
 
> > > > +\subsubsection{Device Context Fields}\label{sec:Basic Facilities
> > > > +of a Virtio
> > > Device / Device Context / Device Context Fields}
> > > > +
> > > > +\paragraph{PCI Common Configuration Context} \label{par:Basic
> > > > +Facilities of a Virtio Device / Device Context / Device Context
> > > Fields/ PCI Common Configuration Context}
> > > > +
> > > > +For the field VIRTIO_DEV_CTX_PCI_COMMON_RUNTIME_CFG,
> \field{type}
> > > is set to 0x0.
> 
> Not sure what does RUNTIME do here.
> 
Runtime is just the prefix to denote that things like runtime are more probable to change as opposed to config which does not have it.

> > > > +The \field{value} is in format of \field{struct virtio_pci_common_cfg}.
> > > > +The \field{length} is the length of \field{struct virtio_pci_common_cfg}.
> > > > +
> > > > +\paragraph{Device Configuration Layout Context} \label{par:Basic
> > > > +Facilities of a Virtio Device / Device Context / Device Context
> > > Fields/ Device Configuration Layout Context}
> > > > +
> > > > +For the field VIRTIO_DEV_CTX_DEV_CFG_LAYOUT, \field{type} is set to
> 0x1.
> 
> This name is quite confusing. I see now you just mean this is device config?
>
Yes, the "device configuration layout fields".
So just do DEV_CFG?
 
> > > > +The \field{value} is in format of device specific configuration
> > > > +layout listed in each of the device's device configuration layout section.
> > > > +The \field{length} is the length of the device configuration layout data.
> > >
> > > Unclear. I am guessing it's doing things like setting up RO fields?
> > > This needs to be specified per device really.
> > > Also how some fields behave might depend on features.
> > In practice fields in this area do not change a lot, but it can for example the
> link status/speed of net device.
> > So it is not RO per say.
> >
> > Regarding it be device specific or just a config_length blob, I think
> config_length blob is just fine for device_context use.
> > This is because there isnât a need for migration driver to parse any of these
> fields.
> 
> Which driver parses what in your current stack is immaterial.  We need to
> document all content, just a length is not going to work.

Sure it is immaterial.
The length covers the length of the dev config space data.
I will add the example containing the link.
Since we want to avoid duplicating the content of device config layout, the example will help to make this very clear.