All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Pawel Moll <Pawel.Moll@arm.com>,
	"virtualization@lists.linux-foundation.org"
	<virtualization@lists.linux-foundation.org>
Subject: Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests
Date: Wed, 13 Feb 2013 18:53:56 +0200	[thread overview]
Message-ID: <20130213165356.GA23193@redhat.com> (raw)
In-Reply-To: <511BB134.30003@arm.com>

On Wed, Feb 13, 2013 at 03:28:52PM +0000, Marc Zyngier wrote:
> On 13/02/13 15:08, Pawel Moll wrote:
> > On Wed, 2013-02-13 at 14:25 +0000, Marc Zyngier wrote:
> >> Using readl() to read the magic value and then memcmp() to check it
> >> fails on BE, as bytes will be the other way around (by virtue of
> >> the registers to follow the endianess of the guest).
> > 
> > Hm. Interesting. I missed the fact that readl() as a "PCI operation"
> > will always assume LE values...
> > 
> >> Fix it by encoding the magic as an integer instead of a string.
> >> So I'm not completely sure this is the right fix, 
> > 
> > It seems right, however...
> > 
> >> - Using __raw_readl() instead. Is that a generic enough API?
> >>
> > ... this implies that either the spec is wrong (as it should say: the
> > device registers are always LE, in the PCI spirit) or all readl()s & co.
> > should be replaced with __raw equivalents.
> 
> Well, the spec clearly says that the registers reflect the endianess of
> the guest, and it makes sense: when performing the MMIO access, KVM
> needs to convert between host and guest endianess.
> 
> > Having said that, does the change make everything else work with a BE
> > guest? (I assume we're talking about the guest being BE, right? ;-) If
> > so it means that the host is not following the current spec and it
> > treats all the registers as LE.
> 
> Yes, I only care about a BE guest. And no, not much is actually working
> (kvmtool is not happy about the guest addresses it finds in the
> virtio-ring). Need to dive into it and understand what needs to be fixed...

Does it work for qemu? I know people were using virtio on BE there.

> >> - Reading the MAGIC register byte by byte. Is that allowed? The spec
> >>   only says it is 32bit wide.
> > 
> > And the spirit of the spec was: _exactly 32bit wide_. It's just simpler
> > to implement one access width on the host side.
> 
> I guessed as much...
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

WARNING: multiple messages have this Message-ID (diff)
From: "Michael S. Tsirkin" <mst@redhat.com>
To: Marc Zyngier <marc.zyngier@arm.com>
Cc: Pawel Moll <Pawel.Moll@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"virtualization@lists.linux-foundation.org" 
	<virtualization@lists.linux-foundation.org>,
	Rusty Russell <rusty@rustcorp.com.au>
Subject: Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests
Date: Wed, 13 Feb 2013 18:53:56 +0200	[thread overview]
Message-ID: <20130213165356.GA23193@redhat.com> (raw)
In-Reply-To: <511BB134.30003@arm.com>

On Wed, Feb 13, 2013 at 03:28:52PM +0000, Marc Zyngier wrote:
> On 13/02/13 15:08, Pawel Moll wrote:
> > On Wed, 2013-02-13 at 14:25 +0000, Marc Zyngier wrote:
> >> Using readl() to read the magic value and then memcmp() to check it
> >> fails on BE, as bytes will be the other way around (by virtue of
> >> the registers to follow the endianess of the guest).
> > 
> > Hm. Interesting. I missed the fact that readl() as a "PCI operation"
> > will always assume LE values...
> > 
> >> Fix it by encoding the magic as an integer instead of a string.
> >> So I'm not completely sure this is the right fix, 
> > 
> > It seems right, however...
> > 
> >> - Using __raw_readl() instead. Is that a generic enough API?
> >>
> > ... this implies that either the spec is wrong (as it should say: the
> > device registers are always LE, in the PCI spirit) or all readl()s & co.
> > should be replaced with __raw equivalents.
> 
> Well, the spec clearly says that the registers reflect the endianess of
> the guest, and it makes sense: when performing the MMIO access, KVM
> needs to convert between host and guest endianess.
> 
> > Having said that, does the change make everything else work with a BE
> > guest? (I assume we're talking about the guest being BE, right? ;-) If
> > so it means that the host is not following the current spec and it
> > treats all the registers as LE.
> 
> Yes, I only care about a BE guest. And no, not much is actually working
> (kvmtool is not happy about the guest addresses it finds in the
> virtio-ring). Need to dive into it and understand what needs to be fixed...

Does it work for qemu? I know people were using virtio on BE there.

> >> - Reading the MAGIC register byte by byte. Is that allowed? The spec
> >>   only says it is 32bit wide.
> > 
> > And the spirit of the spec was: _exactly 32bit wide_. It's just simpler
> > to implement one access width on the host side.
> 
> I guessed as much...
> 
> 	M.
> -- 
> Jazz is not dead. It just smells funny...

  parent reply	other threads:[~2013-02-13 16:53 UTC|newest]

Thread overview: 38+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-02-13 14:25 [RFC PATCH] virt_mmio: fix signature checking for BE guests Marc Zyngier
2013-02-13 14:25 ` Marc Zyngier
2013-02-13 15:08 ` Pawel Moll
2013-02-13 15:08   ` Pawel Moll
2013-02-13 15:28   ` Marc Zyngier
2013-02-13 15:28     ` Marc Zyngier
2013-02-13 15:46     ` Pawel Moll
2013-02-13 15:46       ` Pawel Moll
2013-02-13 16:40       ` Marc Zyngier
2013-02-13 16:40         ` Marc Zyngier
2013-02-13 16:53     ` Michael S. Tsirkin [this message]
2013-02-13 16:53       ` Michael S. Tsirkin
2013-02-13 17:04       ` Marc Zyngier
2013-02-13 17:04         ` Marc Zyngier
2013-02-14  0:22       ` Rusty Russell
2013-02-14  0:22         ` Rusty Russell
2013-02-14 10:54         ` [PATCH] virtio-spec: Define virtio-mmio registers as LE Pawel Moll
2013-02-14 10:54           ` Pawel Moll
2013-02-15  3:57           ` Rusty Russell
2013-02-15  3:57             ` Rusty Russell
2013-03-01 10:41           ` Marc Zyngier
2013-03-01 10:41             ` Marc Zyngier
2013-03-01 10:50             ` Pawel Moll
2013-03-01 10:50               ` Pawel Moll
2013-03-01 11:21               ` Marc Zyngier
2013-03-01 11:21                 ` Marc Zyngier
2013-03-01 12:37                 ` Pawel Moll
2013-03-01 12:37                   ` Pawel Moll
2013-03-01 13:16                   ` Marc Zyngier
2013-03-01 13:16                     ` Marc Zyngier
2013-03-05  0:11                   ` Rusty Russell
2013-03-05  0:11                     ` Rusty Russell
2013-03-06 15:10                     ` Pawel Moll
2013-03-06 15:10                       ` Pawel Moll
2013-03-07  6:36                       ` Rusty Russell
2013-03-07  6:36                         ` Rusty Russell
2013-02-15  0:07     ` [RFC PATCH] virt_mmio: fix signature checking for BE guests Benjamin Herrenschmidt
2013-02-15  0:07     ` Benjamin Herrenschmidt

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=20130213165356.GA23193@redhat.com \
    --to=mst@redhat.com \
    --cc=Pawel.Moll@arm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marc.zyngier@arm.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.