All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rusty Russell <rusty@rustcorp.com.au>
To: Sasha Levin <levinsasha928@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>
Cc: Krishna Kumar <krkumar2@in.ibm.com>,
	kvm@vger.kernel.org, Pawel Moll <pawel.moll@arm.com>,
	Wang Sheng-Hui <shhuiw@gmail.com>,
	Alexey Kardashevskiy <aik@ozlabs.ru>,
	lkml - Kernel Mailing List <linux-kernel@vger.kernel.org>,
	virtualization@lists.linux-foundation.org,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Amit Shah <amit.shah@redhat.com>
Subject: Re: [PATCHv3 RFC] virtio-pci: flexible configuration layout
Date: Thu, 24 Nov 2011 11:37:26 +1030	[thread overview]
Message-ID: <87r50yxr8x.fsf@rustcorp.com.au> (raw)
In-Reply-To: <1322041120.3581.6.camel@lappy>

On Wed, 23 Nov 2011 11:38:40 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Wed, 2011-11-23 at 10:49 +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 23, 2011 at 01:02:22PM +1030, Rusty Russell wrote:
> > > +/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
> > > +struct virtio_pci_common_cfg {
> > > +	/* About the whole device. */
> > > +	__u64 device_features;	/* read-only */
> > > +	__u64 guest_features;	/* read-write */
> > 
> > We currently require atomic accesses to common fields.
> > Some architectures might not have such for 64 bit,
> > so these need to be split I think ...
> 
> We can consider stealing the feature implementation from virtio-mmio:
> Use the first 32 bits as a selector and the last as the features
> themselves.
> 
> It's more complex to work with, but it provides 2**32 32 feature bits
> (which should be enough for a while) and it solves the atomic access
> issue.

That works.  I don't expect we'll need more than 64 features given that
virtio_net hasn't seen a new one in over a year, but it's gone from 5 to
18 in 4 years, so another 32 bits at that rate only gets us another decade.

Unfortunately, it doesn't solve the queue_address problem.

We currently multiply the (32-bit) address by 4096 (the alignment).  If
drivers are going to reduce alignment, that makes it trickier, hence the
change here to a 64.  Simplicity.

We could cheat and assert that a 32-bit write there implies 0 in the
upper bits, and hope that all 64 bit platforms can do a 64-bit atomic
write.  Or insist the value not be intepreted until the guest writes its
(first) feature bit.

Perhaps we really need a "I'm done configuring!" trigger, now we can't
use the feature bit field for that.

Thoughts welcome...
Rusty.

WARNING: multiple messages have this Message-ID (diff)
From: Rusty Russell <rusty@rustcorp.com.au>
To: Sasha Levin <levinsasha928@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>
Cc: lkml - Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Alexey Kardashevskiy <aik@ozlabs.ru>,
	Amit Shah <amit.shah@redhat.com>,
	Christian Borntraeger <borntraeger@de.ibm.com>,
	Krishna Kumar <krkumar2@in.ibm.com>,
	Pawel Moll <pawel.moll@arm.com>,
	Wang Sheng-Hui <shhuiw@gmail.com>,
	virtualization@lists.linux-foundation.org, kvm@vger.kernel.org
Subject: Re: [PATCHv3 RFC] virtio-pci: flexible configuration layout
Date: Thu, 24 Nov 2011 11:37:26 +1030	[thread overview]
Message-ID: <87r50yxr8x.fsf@rustcorp.com.au> (raw)
In-Reply-To: <1322041120.3581.6.camel@lappy>

On Wed, 23 Nov 2011 11:38:40 +0200, Sasha Levin <levinsasha928@gmail.com> wrote:
> On Wed, 2011-11-23 at 10:49 +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 23, 2011 at 01:02:22PM +1030, Rusty Russell wrote:
> > > +/* Fields in VIRTIO_PCI_CAP_COMMON_CFG: */
> > > +struct virtio_pci_common_cfg {
> > > +	/* About the whole device. */
> > > +	__u64 device_features;	/* read-only */
> > > +	__u64 guest_features;	/* read-write */
> > 
> > We currently require atomic accesses to common fields.
> > Some architectures might not have such for 64 bit,
> > so these need to be split I think ...
> 
> We can consider stealing the feature implementation from virtio-mmio:
> Use the first 32 bits as a selector and the last as the features
> themselves.
> 
> It's more complex to work with, but it provides 2**32 32 feature bits
> (which should be enough for a while) and it solves the atomic access
> issue.

That works.  I don't expect we'll need more than 64 features given that
virtio_net hasn't seen a new one in over a year, but it's gone from 5 to
18 in 4 years, so another 32 bits at that rate only gets us another decade.

Unfortunately, it doesn't solve the queue_address problem.

We currently multiply the (32-bit) address by 4096 (the alignment).  If
drivers are going to reduce alignment, that makes it trickier, hence the
change here to a 64.  Simplicity.

We could cheat and assert that a 32-bit write there implies 0 in the
upper bits, and hope that all 64 bit platforms can do a 64-bit atomic
write.  Or insist the value not be intepreted until the guest writes its
(first) feature bit.

Perhaps we really need a "I'm done configuring!" trigger, now we can't
use the feature bit field for that.

Thoughts welcome...
Rusty.

  reply	other threads:[~2011-11-24  1:07 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-11-22 18:36 [PATCHv3 RFC] virtio-pci: flexible configuration layout Michael S. Tsirkin
2011-11-22 18:36 ` Michael S. Tsirkin
2011-11-23  2:32 ` Rusty Russell
2011-11-23  2:32   ` Rusty Russell
2011-11-23  8:46   ` Michael S. Tsirkin
2011-11-23  8:46     ` Michael S. Tsirkin
2011-11-23 15:34     ` Michael S. Tsirkin
2011-11-23 15:34       ` Michael S. Tsirkin
2011-11-24  0:36     ` Rusty Russell
2011-11-24  0:36       ` Rusty Russell
2011-11-24  6:24       ` Michael S. Tsirkin
2011-11-24  6:24         ` Michael S. Tsirkin
2011-11-24  7:11       ` Michael S. Tsirkin
2011-11-24  7:11         ` Michael S. Tsirkin
2011-11-28  0:55         ` Rusty Russell
2011-11-28  0:55           ` Rusty Russell
2011-11-28  8:41           ` Michael S. Tsirkin
2011-11-28  8:41             ` Michael S. Tsirkin
2011-11-29 23:28             ` Rusty Russell
2011-11-29 23:28               ` Rusty Russell
2011-11-30  7:18               ` Michael S. Tsirkin
2011-11-30  7:18                 ` Michael S. Tsirkin
2011-11-28  9:15           ` Sasha Levin
2011-11-28  9:15             ` Sasha Levin
2011-11-29 23:40             ` Rusty Russell
2011-11-29 23:40               ` Rusty Russell
2011-11-30  8:14               ` Michael S. Tsirkin
2011-11-30  8:14                 ` Michael S. Tsirkin
2011-11-30 13:12               ` Sasha Levin
2011-11-30 13:12                 ` Sasha Levin
2011-12-01  2:42                 ` Rusty Russell
2011-12-01  2:42                   ` Rusty Russell
2011-11-23  8:49   ` Michael S. Tsirkin
2011-11-23  8:49     ` Michael S. Tsirkin
2011-11-23  9:38     ` Sasha Levin
2011-11-23  9:38       ` Sasha Levin
2011-11-24  1:07       ` Rusty Russell [this message]
2011-11-24  1:07         ` Rusty Russell
2011-11-23  9:44   ` Sasha Levin
2011-11-23  9:44     ` Sasha Levin

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=87r50yxr8x.fsf@rustcorp.com.au \
    --to=rusty@rustcorp.com.au \
    --cc=aik@ozlabs.ru \
    --cc=amit.shah@redhat.com \
    --cc=borntraeger@de.ibm.com \
    --cc=krkumar2@in.ibm.com \
    --cc=kvm@vger.kernel.org \
    --cc=levinsasha928@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mst@redhat.com \
    --cc=pawel.moll@arm.com \
    --cc=shhuiw@gmail.com \
    --cc=virtualization@lists.linux-foundation.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.