On 1/23/2020 12:56 AM, Stefan Hajnoczi wrote:> On Mon, Jan 13, 2020 at 07:54:06PM +0800, Liu, Jing2 wrote:>>>>>>         \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.> I understand now:>>    Devices that only require a single notify address may set>    notify_base=0x50 and notify_multiplier=0 to use the Queue Notifier>    register itself for notifications.  In this case the driver writes to>    Queue Notifier to notify the device that there are new buffers in a>    virtqueue.>> Perhaps you could include this in the text.Thanks for the guide. Since v2 was sent out, we'll add such text in later version.Jing>>>>>> -    \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 Buﬀer Notifcations (section describing>> VIRTIO_F_NOTIFICATION_DATA) makes vq index as 16bit?> As you mentioned, the valid range of virtqueue numbers is only 16 bits> due to non-MMIO parts of the specification using 16 bits.>> However, I think it makes sense to stick to the MMIO transport 32-bit> register size convention for consistency.  Devices just won't support> values above 0xffff.>> Stefan