Re: [RESEND PATCH v6] virtio: introduce SUSPEND bit in device status

[Cornelia Huck <cohuck@redhat.com>]

On Thu, Jul 04 2024, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Jul 04, 2024 at 05:39:32PM +0900, David Stevens wrote:
>> On Wed, Jul 3, 2024 at 6:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> >
>> > On Wed, Jul 03, 2024 at 11:10:57AM +0800, Zhu Lingshan wrote:
>> > > +\item The driver MUST NOT access Device Configuration Space except for the field \field {device status},
>> > > +if it is implemented in the Config Space.
>> >
>> > what is "the Config Space"? device status is never in
>> > a device configuration space, it is part of the common configuration.
>> > so what exactly are you forbidding here?
>> 
>> IIUC, referring to the common configuration here wouldn't make sense.
>> The common configuration is part of the PCI transport specification,
>> so in this part of the specification it should only be referred to via
>> the "transport specific" phrasing.
>
> Good point.  We can easily fix this for MMIO though.
>
>  
> MMIO virtio devices provide a set of memory mapped control
> registers followed by a device-specific configuration space,
> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
>
> ->
>
> MMIO virtio devices provide memory mapped control
> including a set of common configuration
> registers followed by a device-specific configuration space,
> described in the table~\ref{tab:Virtio Transport Options / Virtio Over MMIO / MMIO Device Register Layout}.
>
>
>
> Less sure about CCW.
> Maybe:
>
> In addition to the basic channel commands, virtio-ccw defines a
> set of channel commands related to configuration and operation of
> virtio:
>
> \begin{lstlisting}
> #define CCW_CMD_SET_VQ 0x13
> #define CCW_CMD_VDEV_RESET 0x33
> #define CCW_CMD_SET_IND 0x43
> #define CCW_CMD_SET_CONF_IND 0x53
> #define CCW_CMD_SET_IND_ADAPTER 0x73
> #define CCW_CMD_READ_FEAT 0x12
> #define CCW_CMD_WRITE_FEAT 0x11
> #define CCW_CMD_READ_CONF 0x22
> #define CCW_CMD_WRITE_CONF 0x21
> #define CCW_CMD_WRITE_STATUS 0x31
> #define CCW_CMD_READ_VQ_CONF 0x32
> #define CCW_CMD_SET_VIRTIO_REV 0x83
> #define CCW_CMD_READ_STATUS 0x72
> \end{lstlisting}
>
> ->
>
>
> In addition to the basic channel commands, virtio-ccw defines
> channel commands related to configuration and operation of
> virtio - a set of commands to access common configuration of
> the device:

Hm, I'm not sure whether "common configuration" is the best wording
here; resetting a device, for example, is not something I'd call
configuration. If we don't manage to come up with anything better, I can
live with it, though.

>
> \begin{lstlisting}
> #define CCW_CMD_SET_VQ 0x13
> #define CCW_CMD_VDEV_RESET 0x33
> #define CCW_CMD_SET_IND 0x43
> #define CCW_CMD_SET_CONF_IND 0x53
> #define CCW_CMD_SET_IND_ADAPTER 0x73
> #define CCW_CMD_READ_FEAT 0x12
> #define CCW_CMD_WRITE_FEAT 0x11
> #define CCW_CMD_WRITE_STATUS 0x31
> #define CCW_CMD_READ_VQ_CONF 0x32
> #define CCW_CMD_SET_VIRTIO_REV 0x83
> #define CCW_CMD_READ_STATUS 0x72
> \end{lstlisting}
>
> and additionally, two commands to access the device
> specification configuration space:
>
> \begin{lstlisting}
> #define CCW_CMD_READ_CONF 0x22
> #define CCW_CMD_WRITE_CONF 0x21
> \end{lstlisting}
>
>
> And now we have common configuration defined for all transports.
> Cornelia, WDYT?
>
>
>> This wording appears to be taken
>> from Cornelia's feedback on v5. Specifically:
>> 
>> > > +\item The driver MUST NOT access Device Configuration Space.
>> > ...except for the status field, if it is part of the config space?
>> 
>> I asked for clarification [1], because Cornelia's feedback seems to
>> contradict your feedback here and on the (unfortunately unarchived) v3
>> patch. What is the definitive statement agreed upon by both editors of
>> the virtio specification as to whether or not the device status field
>> is part of the device configuration space?
>> 
>> [1] https://lore.kernel.org/all/CAD=HUj4RBeLS4L=ehyPGtejeu+sGZ5j8PRtrK8AvxjEgEBd5ZA@mail.gmail.com/
>> 
>> -David
>
> I don't know what did Cornelia mean. Cornelia?

What I meant (I think...) was that accessing the status field is fine,
even if it is implemented as part of the config space.