Re: [virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support
From: Liu, Jing2
Date: Mon Feb 10 2020 - 04:21:20 EST
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