Re: [virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support

["Liu, Jing2" <jing2.liu@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 8831 bytes --]

>>>> @@ -1597,9 +1597,9 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>>>>      }
>>>>      \hline
>>>>      \mmioreg{Version}{Device version number}{0x004}{R}{%
>>>> -    0x2.
>>>> +    0x3.
>>>>        \begin{note}
>>>> -      Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1.
>>>> +      Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1 or 0x2.
>>> "Legacy devices" refers to pre-VIRTIO 1.0 devices.  0x2 is VIRTIO 1.0
>>> and therefore not "Legacy".  I suggest the following wording:
>>>
>>>         Legacy devices (see \ref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}~\nameref{sec:Virtio Transport Options / Virtio Over MMIO / Legacy interface}) used 0x1.
>>> +     VIRTIO 1.0 and 1.1 used 0x2.
>> Thanks for the guide.
>>> Did you consider using a transport feature bit instead of changing the
>>> device version number?  Feature bits allow more graceful compatibility:
>>> old drivers will continue to work with new devices and new drivers will
>>> continue to work with old devices.
>> Yes, we considered using a feature bit from 24~37 or above, while a concern
>> is that,
>>
>> the device which uses such bit only represents the behavior of mmio
>> transport layer but not common behavior
>>
>> of virtio device. Or am I missing some "transport" feature bit range?
> https://docs.oasis-open.org/virtio/virtio/v1.1/cs01/virtio-v1.1-cs01.html#x1-4100006
>
> Feature bit 39 is reserved and can be used.
>
> This is similar to when the SR_IOV (37) feature bit was added for
> virtio-pci.
That's great. Let's try to use bit 39 for MMIO MSI and bit 40 for MMIO 
notifications capabilities.
>>>>        \end{note}
>>>>      }
>>>>      \hline
>>>> @@ -1671,25 +1671,23 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>>>>        accesses apply to the queue selected by writing to \field{QueueSel}.
>>>>      }
>>>>      \hline
>>>> -  \mmioreg{QueueNotify}{Queue notifier}{0x050}{W}{%
>>>> -    Writing a value to this register notifies the device that
>>>> -    there are new buffers to process in a queue.
>>>> +  \mmioreg{QueueNotify}{Queue notifier}{0x050}{RW}{%
>>>> +    Reading from the register returns the virtqueue notification configuration.
>>>> -    When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
>>>> -    the value written is the queue index.
>>>> +    See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Notification Address}
>>>> +    for the configuration format.
>>>> -    When VIRTIO_F_NOTIFICATION_DATA has been negotiated,
>>>> -    the \field{Notification data} value has the following format:
>>>> +    Writing when the notification address calculated by the notification configuration
>>>> +    is just located at this register.
>>> I don't understand this sentence.  What happens when the driver writes
>>> to this register?
>> We're trying to define the notification mechanism that, driver MUST read
>> 0x50 to get the notification configuration
>>
>> and calculate the notify address. The writing case here is that, the notify
>> address is just located here e.g. notify_base=0x50, notify_mul=0.
> I still don't understand what this means.  It's just an English issue
> and it will become clear if you can rephrase what you're saying.

Sure, let me try to explain it. :)

The different notification locations are calculated via the structure 
returned by reading this register.

le32 {
     notify_base : 16;
     notify_multiplier : 16;
};

location=notify_base + queue_index * notify_multiplier

The location might be the same when mul=0, and furthermore, it might be 
equal to 0x50 (notify_base=0x50, notify_mul=0) so we make this register 
W too.

So we said, the register is RW and W is only for such scenario.

Feel free to tell me if it's still confusing.

>>>> -    \lstinputlisting{notifications-le.c}
>>>> -
>>>> -    See \ref{sec:Virtqueues / Driver notifications}~\nameref{sec:Virtqueues / Driver notifications}
>>>> -    for the definition of the components.
>>>> +    See \ref{sec:Virtio Transport Options / Virtio Over MMIO / MMIO-specific Initialization And Device Operation / Available Buffer Notifications}
>>>> +    to see the notification data format.
>>>>      }
>>>>      \hline
>>>>      \mmioreg{InterruptStatus}{Interrupt status}{0x60}{R}{%
>>>>        Reading from this register returns a bit mask of events that
>>>> -    caused the device interrupt to be asserted.
>>>> +    caused the device interrupt to be asserted. This is only used
>>>> +    when MSI is not enabled.
>>>>        The following events are possible:
>>>>        \begin{description}
>>>>          \item[Used Buffer Notification] - bit 0 - the interrupt was asserted
>>>> @@ -1703,7 +1701,7 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>>>>      \mmioreg{InterruptACK}{Interrupt acknowledge}{0x064}{W}{%
>>>>        Writing a value with bits set as defined in \field{InterruptStatus}
>>>>        to this register notifies the device that events causing
>>>> -    the interrupt have been handled.
>>>> +    the interrupt have been handled. This is only used when MSI is not enabled.
>>>>      }
>>>>      \hline
>>>>      \mmioreg{Status}{Device status}{0x070}{RW}{%
>>>> @@ -1762,6 +1760,31 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
>>>>        \field{SHMSel} is unused) results in a base address of
>>>>        0xffffffffffffffff.
>>>>      }
>>>> +  \hline
>>>> +  \mmioreg{MsiStatus}{MSI status}{0x0c0}{R}{%
>>>> +    Reading from this register returns the global MSI enable/disable status and maximum
>>>> +    number of virtqueues that device supports.
>>>> +    \lstinputlisting{msi-status.c}
>>>> +  }
>>> Why is it necessary to combine the number of virtqueues and global
>>> MSI enable/disable into a single 16-bit field?
>> Originally, we want this 16-bit Read-Only, so we put some RO things together
>> and separate
>>
>> enable setting command to next register.
>>
>>> virtio-mmio uses 32-bit registers.  It doesn't try hard to save register
>>> space so it's strange to do it here (11-bit number of virtqueue field
>>> but 32-bit QueueSel field).
>> In order to improve performance/save register space,  we combine some data
>> together.
>>
>> For example, combine MSI cmd operator (e.g. enable/disable, vector setup)
>> with argument (e.g. 1/0,  queue index).
>>
>> But it seems we miss the consistency with QueueSel.  So do you think if the
>> max queue number should be 32-bit,
>>
>> which means it must be the same with QueueSel? If so, I guess we need some
>> re-organization. :)
> I suggest following the 32-bit register size convention unless there is
> a specific reason why using other register sizes is absolutely necessary.

Yes, let's keep consistency with QueueSel and re-organize other registers.

I feel concern why Available Buffer Notifcations (section describing 
VIRTIO_F_NOTIFICATION_DATA) makes vq index as 16bit?

>>>> +  \hline
>>>> +  \mmioreg{MsiCmd}{MSI command}{0x0c2}{W}{%
>>>> +    The driver writes to this register with appropriate operators and arguments to
>>>> +    execute MSI command to device.
>>>> +    Operators supported is setting global MSI enable/disable status
>>>> +    and updating MSI configuration to device.
>>> If the global MSI enable/disable state is written in this register,
>>> consider making this register readable too so the global MSI
>>> enable/disable state can be read from it.
>> Read enable/disable state is in 0x0c0.
> The read-only 0xc0 register combines two pieces of information:
> 1. Global MSI enable/disable state
> 2. Number of virtqueues (or is "number of MSI vectors" a more accurate
>     term?)
>
> A simpler way of organizing the registers is:
> MsiMaxVectors R
> MsiState RW
>
> This is simpler because drivers can read the MsiMaxVectors field and
> treat the value as an integer (no masking required).  And a person
> reading the specification instantly knows how to fetch the MSI
> enable/disable state when they read the MsiState register description.
> They don't need to look through all the other registers to find the one
> that allows them to read the state.

That's a good idea to separate the MsiMaxVectors(32bit) and this also 
meets the request from Jason/MST's comments.

Thanks!

Jing

>
> Stefan

[-- Attachment #2: Type: text/html, Size: 12262 bytes --]