[RFC PATCH] virt_mmio: fix signature checking for BE guests

[RFC PATCH] virt_mmio: fix signature checking for BE guests

[Marc Zyngier]

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).

Fix it by encoding the magic as an integer instead of a string.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
So I'm not completely sure this is the right fix, and I can imagine
other ways to cure the problem:
- Reading the MAGIC register byte by byte. Is that allowed? The spec
  only says it is 32bit wide.
- Using __raw_readl() instead. Is that a generic enough API?

 drivers/virtio/virtio_mmio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_mmio.c b/drivers/virtio/virtio_mmio.c
index 31f966f..3811e94 100644
--- a/drivers/virtio/virtio_mmio.c
+++ b/drivers/virtio/virtio_mmio.c
@@ -470,7 +470,7 @@ static int virtio_mmio_probe(struct platform_device *pdev)
 
 	/* Check magic value */
 	magic = readl(vm_dev->base + VIRTIO_MMIO_MAGIC_VALUE);
-	if (memcmp(&magic, "virt", 4) != 0) {
+	if (magic != ('v' | 'i' << 8 | 'r' << 16 | 't' << 24)) {
 		dev_warn(&pdev->dev, "Wrong magic value 0x%08lx!\n", magic);
 		return -ENODEV;
 	}
-- 
1.8.1.2

Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests

[Pawel Moll]

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.

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.

> - 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.

Paweł

Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests

[Marc Zyngier]

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...

>> - 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...

Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests

[Pawel Moll]

On Wed, 2013-02-13 at 15:28 +0000, Marc Zyngier wrote:
> >> 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.

The virtio-mmio spec says so because it seemed like a good idea at the
time ;-) after reading the PCI device spec. But - as I said - I missed
the fact that the readl()-like accessors will always do le32_to_cpu().
Apparently ioread32() does the same (there's a separate ioread32be()).
So I'm not sure that the spec is correct in this aspect any more. Maybe
it should specify the registers as LE always, similarly to PCI? This
problem is already covered by "2.3.1 A Note on Virtqueue Endianness" in
the spec...

> > 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...

Do the other registers like queuenum make sense? Could it be that the
page number of the ring you're getting has wrong endianness?

Paweł

Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests

[Marc Zyngier]

On 13/02/13 15:46, Pawel Moll wrote:
> On Wed, 2013-02-13 at 15:28 +0000, Marc Zyngier wrote:
>>>> 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.
> 
> The virtio-mmio spec says so because it seemed like a good idea at the
> time ;-) after reading the PCI device spec. But - as I said - I missed
> the fact that the readl()-like accessors will always do le32_to_cpu().
> Apparently ioread32() does the same (there's a separate ioread32be()).

Maybe. There's so much byte swapping at every possible level that my
head spins... ;-)

> So I'm not sure that the spec is correct in this aspect any more. Maybe
> it should specify the registers as LE always, similarly to PCI? This
> problem is already covered by "2.3.1 A Note on Virtqueue Endianness" in
> the spec...

This section basically covers shared memory, and there is not much we
can do about it. When it comes to the registers (that actually trap into
the hypervisor), it probably makes sense to declare them as LE indeed.

>>> 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...
> 
> Do the other registers like queuenum make sense? Could it be that the
> page number of the ring you're getting has wrong endianness?

The addresses are definitely wrong. kvmtool is spitting things like:
Warning: unable to translate guest address 0xe8fd828f00000000 to host

which tends to indicate that yes, page numbers are the other way around.
Cross-endianness shared memory fun.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests

[Michael S. Tsirkin]

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...

Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests

[Marc Zyngier]

On 13/02/13 16:53, Michael S. Tsirkin wrote:
> 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.

No idea, haven't tried yet, and my guest "platform" isn't supported by
QEMU. Is someone actually using BE guests on LE hosts? That would be
very interesting to compare things...

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests

[Rusty Russell]

"Michael S. Tsirkin" <mst@redhat.com> writes:
> 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.

It's had no end of problems: the powerpc folk are quite upset with me.

If I were doing virtio again, I'd use LE everywhere: device authors
*expect* to worry about endian, and it's confusing for them when they
don't.

It's tempting to use LE for the PCI config space for the new layout, for
example.

If you want to specify virtio-mmio as LE, feel free.

Cheers,
Rusty.

[PATCH] virtio-spec: Define virtio-mmio registers as LE

[Pawel Moll]

To solve the never-ending confusions between hosts and guests
of different endianess, define all virtio-mmio registers as LE.

This change should be safe at this stage, because no known
working mixed-endian system exists so there is virtually no
risk of breaking compatibility.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 virtio-spec.lyx |   30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 1ba9992..a00b675 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -56,6 +56,7 @@
 \html_math_output 0
 \html_css_as_file 0
 \html_be_strict false
+\author -875410574 "Pawel Moll" 
 \author -608949062 "Rusty Russell,,," 
 \author -385801441 "Cornelia Huck" cornelia.huck@de.ibm.com
 \author 1112500848 "Rusty Russell" rusty@rustcorp.com.au
@@ -1850,6 +1851,17 @@ struct vring {
 
 \begin_layout Subsection
 A Note on Virtqueue Endianness
+\change_inserted -875410574 1360838374
+
+\begin_inset CommandInset label
+LatexCommand label
+name "sub:Virtqueue-Endianness"
+
+\end_inset
+
+
+\change_unchanged
+
 \end_layout
 
 \begin_layout Standard
@@ -9850,8 +9862,24 @@ a
 \end_layout
 
 \begin_layout Standard
+
+\change_deleted -875410574 1360838214
 The endianness of the registers follows the native endianness of the Guest.
- Writing to registers described as 
+\change_inserted -875410574 1360838930
+All register values are organized as Little Endian, similarly to the PCI
+ variant, see also 
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "sub:Virtqueue-Endianness"
+
+\end_inset
+
+.
+ 
+\change_deleted -875410574 1360838262
+ 
+\change_unchanged
+Writing to registers described as 
 \begin_inset Quotes eld
 \end_inset
 
-- 
1.7.10.4

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Rusty Russell]

Pawel Moll <pawel.moll@arm.com> writes:
> To solve the never-ending confusions between hosts and guests
> of different endianess, define all virtio-mmio registers as LE.
>
> This change should be safe at this stage, because no known
> working mixed-endian system exists so there is virtually no
> risk of breaking compatibility.
>
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

Applied.  We could also move device configuration space to LE, if we
wanted.  It's a bigger change to the Linux implementation, since we now
need accessors for each width, but it might be a good idea.  OTOH,
getting such a change into qemu might be harder...

Cheers,
Rusty.

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Marc Zyngier]

On 14/02/13 10:54, Pawel Moll wrote:
> To solve the never-ending confusions between hosts and guests
> of different endianess, define all virtio-mmio registers as LE.
> 
> This change should be safe at this stage, because no known
> working mixed-endian system exists so there is virtually no
> risk of breaking compatibility.
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> ---
>  virtio-spec.lyx |   30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/virtio-spec.lyx b/virtio-spec.lyx
> index 1ba9992..a00b675 100644
> --- a/virtio-spec.lyx
> +++ b/virtio-spec.lyx
> @@ -56,6 +56,7 @@
>  \html_math_output 0
>  \html_css_as_file 0
>  \html_be_strict false
> +\author -875410574 "Pawel Moll" 
>  \author -608949062 "Rusty Russell,,," 
>  \author -385801441 "Cornelia Huck" cornelia.huck@de.ibm.com
>  \author 1112500848 "Rusty Russell" rusty@rustcorp.com.au
> @@ -1850,6 +1851,17 @@ struct vring {
>  
>  \begin_layout Subsection
>  A Note on Virtqueue Endianness
> +\change_inserted -875410574 1360838374
> +
> +\begin_inset CommandInset label
> +LatexCommand label
> +name "sub:Virtqueue-Endianness"
> +
> +\end_inset
> +
> +
> +\change_unchanged
> +
>  \end_layout
>  
>  \begin_layout Standard
> @@ -9850,8 +9862,24 @@ a
>  \end_layout
>  
>  \begin_layout Standard
> +
> +\change_deleted -875410574 1360838214
>  The endianness of the registers follows the native endianness of the Guest.
> - Writing to registers described as 
> +\change_inserted -875410574 1360838930
> +All register values are organized as Little Endian, similarly to the PCI
> + variant, see also 
> +\begin_inset CommandInset ref
> +LatexCommand ref
> +reference "sub:Virtqueue-Endianness"

Shouldn't we exclude the config space? PCI defines it as guest-endian,
and the above tends to indicate that it should be LE with MMIO.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Pawel Moll]

On Fri, 2013-03-01 at 10:41 +0000, Marc Zyngier wrote:
> On 14/02/13 10:54, Pawel Moll wrote:
> > To solve the never-ending confusions between hosts and guests
> > of different endianess, define all virtio-mmio registers as LE.
> > 
> > This change should be safe at this stage, because no known
> > working mixed-endian system exists so there is virtually no
> > risk of breaking compatibility.
> > 
> > Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>
> Shouldn't we exclude the config space? PCI defines it as guest-endian,
> and the above tends to indicate that it should be LE with MMIO.

The spec says: "Device-specific configuration space starts at an offset
0x100 and is accessed with byte alignment. Its meaning and size depends
on the device and the driver."

I would hope that "accessed with byte alignment" is enough of a clue in
this subject, but if you think otherwise I can add a sentence stating
this explicitly.

Having said that, Rusty was contemplating enforcing LE config space in
the new PCI layout...

Paweł

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Marc Zyngier]

On 01/03/13 10:50, Pawel Moll wrote:
> On Fri, 2013-03-01 at 10:41 +0000, Marc Zyngier wrote:
>> On 14/02/13 10:54, Pawel Moll wrote:
>>> To solve the never-ending confusions between hosts and guests
>>> of different endianess, define all virtio-mmio registers as LE.
>>>
>>> This change should be safe at this stage, because no known
>>> working mixed-endian system exists so there is virtually no
>>> risk of breaking compatibility.
>>>
>>> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>>
>> Shouldn't we exclude the config space? PCI defines it as guest-endian,
>> and the above tends to indicate that it should be LE with MMIO.
> 
> The spec says: "Device-specific configuration space starts at an offset
> 0x100 and is accessed with byte alignment. Its meaning and size depends
> on the device and the driver."
> 
> I would hope that "accessed with byte alignment" is enough of a clue in
> this subject, but if you think otherwise I can add a sentence stating
> this explicitly.

Well, it was unclear enough for me to get confused... ;-) It would make
sense to have a wording similar to the one in the PCI section.

> Having said that, Rusty was contemplating enforcing LE config space in
> the new PCI layout...

I wouldn't complain about that, and would like to see a similar thing on
MMIO.

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Pawel Moll]

On Fri, 2013-03-01 at 11:21 +0000, Marc Zyngier wrote:
> > Having said that, Rusty was contemplating enforcing LE config space in
> > the new PCI layout...
> 
> I wouldn't complain about that, and would like to see a similar thing on
> MMIO.

Wherever PCI goes, MMIO follows :-)

> Well, it was unclear enough for me to get confused... ;-) It would make
> sense to have a wording similar to the one in the PCI section.

How about this?

8<---------
>From a80f01f15397395a8fc49ef424a2d47c8be0937a Mon Sep 17 00:00:00 2001
From: Pawel Moll <pawel.moll@arm.com>
Date: Fri, 1 Mar 2013 12:35:06 +0000
Subject: [PATCH] virtio-spec: Clarify virtio-mmio configuration space organisation

To clarify potential confusion regarding the configuration
space organisation (endianness in particular) the spec
should clearly state that it follows the PCI device behaviour.

Signed-off-by: Pawel Moll <pawel.moll@arm.com>
---
 virtio-spec.lyx |   22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index a00b675..2fba0b0 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -696,6 +696,17 @@ rproc serial
 
 \begin_layout Section
 Device Configuration
+\change_inserted -875410574 1362141102
+
+\begin_inset CommandInset label
+LatexCommand label
+name "sec:Device-Configuration"
+
+\end_inset
+
+
+\change_unchanged
+
 \end_layout
 
 \begin_layout Standard
@@ -9865,7 +9876,7 @@ a
 
 \change_deleted -875410574 1360838214
 The endianness of the registers follows the native endianness of the Guest.
-\change_inserted -875410574 1360838930
+\change_inserted -875410574 1362141146
 All register values are organized as Little Endian, similarly to the PCI
  variant, see also 
 \begin_inset CommandInset ref
@@ -9875,6 +9886,15 @@ reference "sub:Virtqueue-Endianness"
 \end_inset
 
 .
+ The device-specific configuration space organisation follows the PCI device
+ specification, see
+\begin_inset CommandInset ref
+LatexCommand ref
+reference "sec:Device-Configuration"
+
+\end_inset
+
+.
  
 \change_deleted -875410574 1360838262
  
-- 
1.7.10.4

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Marc Zyngier]

On 01/03/13 12:37, Pawel Moll wrote:
> On Fri, 2013-03-01 at 11:21 +0000, Marc Zyngier wrote:
>>> Having said that, Rusty was contemplating enforcing LE config space in
>>> the new PCI layout...
>>
>> I wouldn't complain about that, and would like to see a similar thing on
>> MMIO.
> 
> Wherever PCI goes, MMIO follows :-)

Sweet. Makes my life easier ;-).

>> Well, it was unclear enough for me to get confused... ;-) It would make
>> sense to have a wording similar to the one in the PCI section.
> 
> How about this?
> 
> 8<---------
> From a80f01f15397395a8fc49ef424a2d47c8be0937a Mon Sep 17 00:00:00 2001
> From: Pawel Moll <pawel.moll@arm.com>
> Date: Fri, 1 Mar 2013 12:35:06 +0000
> Subject: [PATCH] virtio-spec: Clarify virtio-mmio configuration space organisation
> 
> To clarify potential confusion regarding the configuration
> space organisation (endianness in particular) the spec
> should clearly state that it follows the PCI device behaviour.
> 
> Signed-off-by: Pawel Moll <pawel.moll@arm.com>

Looks good to me!

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Rusty Russell]

Pawel Moll <pawel.moll@arm.com> writes:
> On Fri, 2013-03-01 at 11:21 +0000, Marc Zyngier wrote:
>> > Having said that, Rusty was contemplating enforcing LE config space in
>> > the new PCI layout...
>> 
>> I wouldn't complain about that, and would like to see a similar thing on
>> MMIO.
>
> Wherever PCI goes, MMIO follows :-)

Yes, but if you switch from 'guest-endian' to 'little-endian' how will
you tell?  For PCI, we'd detect it by using the new layout.

I'd rather you specify MMIO as little endian, and we fix the kernel
config accessors to be endian aware (ie. 8, 16, 32, 64-bit accessors).
Since noone BE is using MMIO right now, it's safe...

Thanks,
Rusty.

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Pawel Moll]

On Tue, 2013-03-05 at 00:11 +0000, Rusty Russell wrote:
> Pawel Moll <pawel.moll@arm.com> writes:
> > On Fri, 2013-03-01 at 11:21 +0000, Marc Zyngier wrote:
> >> > Having said that, Rusty was contemplating enforcing LE config space in
> >> > the new PCI layout...
> >> 
> >> I wouldn't complain about that, and would like to see a similar thing on
> >> MMIO.
> >
> > Wherever PCI goes, MMIO follows :-)
> 
> Yes, but if you switch from 'guest-endian' to 'little-endian' how will
> you tell?  For PCI, we'd detect it by using the new layout.

The version register/value. At some point of time there will be a
new(ish) MMIO layout anyway to deal with 64-bit addresses, replacing the
ring page number with two 32-bit hi/lo physical address registers. This
was discussed not long after the driver got merged...

> I'd rather you specify MMIO as little endian, and we fix the kernel
> config accessors to be endian aware (ie. 8, 16, 32, 64-bit accessors).
> Since noone BE is using MMIO right now, it's safe...

That's absolutely fine with me, however I don't see anything I could do
in the virtio_mmio driver and spec - the virtio_config_ops specifies
get/set as void * operations and I simply do byte-by-byte copy. Have I
missed some config/endianess/PCI related discussion?

Paweł

Re: [PATCH] virtio-spec: Define virtio-mmio registers as LE

[Rusty Russell]

Pawel Moll <pawel.moll@arm.com> writes:
> On Tue, 2013-03-05 at 00:11 +0000, Rusty Russell wrote:
>> Pawel Moll <pawel.moll@arm.com> writes:
>> > On Fri, 2013-03-01 at 11:21 +0000, Marc Zyngier wrote:
>> >> > Having said that, Rusty was contemplating enforcing LE config space in
>> >> > the new PCI layout...
>> >> 
>> >> I wouldn't complain about that, and would like to see a similar thing on
>> >> MMIO.
>> >
>> > Wherever PCI goes, MMIO follows :-)
>> 
>> Yes, but if you switch from 'guest-endian' to 'little-endian' how will
>> you tell?  For PCI, we'd detect it by using the new layout.
>
> The version register/value. At some point of time there will be a
> new(ish) MMIO layout anyway to deal with 64-bit addresses, replacing the
> ring page number with two 32-bit hi/lo physical address registers. This
> was discussed not long after the driver got merged...

As long as you have a plan for older guests...

>> I'd rather you specify MMIO as little endian, and we fix the kernel
>> config accessors to be endian aware (ie. 8, 16, 32, 64-bit accessors).
>> Since noone BE is using MMIO right now, it's safe...
>
> That's absolutely fine with me, however I don't see anything I could do
> in the virtio_mmio driver and spec - the virtio_config_ops specifies
> get/set as void * operations and I simply do byte-by-byte copy. Have I
> missed some config/endianess/PCI related discussion?

Yes, that's exactly what I mean, they'd have to be split into
8/16/32/64 accessors.  Which would do byte-by-byte for PCI.

The spec can simply be updated.

Cheers,
Rusty.

Re: [RFC PATCH] virt_mmio: fix signature checking for BE guests

[Benjamin Herrenschmidt]

On Wed, 2013-02-13 at 15:28 +0000, Marc Zyngier wrote:

> 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.

It's actually a horrible idea :-)

What does "guest endianness" means from a qemu perspective if your
emulated CPU can operate in either mode ?

It's actually been causing endless problems, besides linux doesn't have
"Sane" MMIO accessors that say "current endianness". ioreadN/writeN are
LE, realN/writeN are LE, ioreadNbe/iowriteNbe are BE always, the only
"whatever my endianness is" are the __raw ones which also don't have
barriers etc...  

> > 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...
> 
> >> - 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...

Cheers,
Ben.