Messages in this thread | | | Subject | Re: [virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support | From | "Liu, Jing2" <> | Date | Mon, 10 Feb 2020 17:21:11 +0800 |
| |
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 Buffer 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
| |