From: Stefan Hajnoczi <stefanha@redhat.com>
To: "Liu, Jing2" <jing2.liu@linux.intel.com>
Cc: virtio-dev@lists.oasis-open.org, slp@redhat.com,
linux-kernel@vger.kernel.org,
Chao Peng <chao.p.peng@linux.intel.com>,
Zha Bin <zhabin@linux.alibaba.com>,
Liu Jiang <gerry@linux.alibaba.com>
Subject: Re: [virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support
Date: Wed, 22 Jan 2020 16:56:37 +0000 [thread overview]
Message-ID: <20200122165637.GD677983@stefanha-x1.localdomain> (raw)
In-Reply-To: <801cf09a-759f-b6bf-e71b-02dbf0f1d513@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 6642 bytes --]
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.
> > > > > - \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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2020-01-22 16:56 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-20 15:25 [virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support Jing Liu
2019-12-20 15:25 ` [virtio-dev][PATCH v1 2/2] virtio-mmio: Append version 2 interface Jing Liu
2020-01-06 16:18 ` [virtio-dev][PATCH v1 1/2] virtio-mmio: Add MSI and different notification address support Stefan Hajnoczi
2020-01-09 5:13 ` Liu, Jing2
2020-01-10 9:52 ` Stefan Hajnoczi
[not found] ` <801cf09a-759f-b6bf-e71b-02dbf0f1d513@linux.intel.com>
2020-01-22 16:56 ` Stefan Hajnoczi [this message]
2020-02-10 9:21 ` Liu, Jing2
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200122165637.GD677983@stefanha-x1.localdomain \
--to=stefanha@redhat.com \
--cc=chao.p.peng@linux.intel.com \
--cc=gerry@linux.alibaba.com \
--cc=jing2.liu@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=slp@redhat.com \
--cc=virtio-dev@lists.oasis-open.org \
--cc=zhabin@linux.alibaba.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).