[RFC PATCH] virtio: change config to guest endian.

Rusty Russell

author

2008-04-22 03:57 UTC #0 lore raw

RFC PATCH 1/1 virtio: change config to guest endian.

[Christian, Hollis, how much is this ABI breakage going to hurt you?]

A recent proposed feature addition to the virtio block driver revealed some flaws in the API, in particular how easy it is to break big endian machines.

The virtio config space was originally chosen to be little-endian, because we thought the config might be part of the PCI config space for virtio_pci. It's actually a separate mmio region, so that argument holds little water; as only x86 is currently using the virtio mechanism, we can change this (but must do so now, before the impending s390 and ppc merges).

API changes:
- __virtio_config_val() just becomes a striaght vdev->config_get() call.

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/block/virtio_blk.c      |    4 +--
 drivers/virtio/virtio_balloon.c |    6 ++---
 include/linux/virtio_config.h   |   47 +++++++++++++---------------------------
 3 files changed, 21 insertions(+), 36 deletions(-)

drivers/block/virtio_blk.c +2 -2

diff -r a098f19a6da5 drivers/block/virtio_blk.c
@@ -246,8 +246,8 @@ static int virtblk_probe(struct virtio_d
 		blk_queue_ordered(vblk->disk->queue, QUEUE_ORDERED_TAG, NULL);
 
 	/* Host must always specify the capacity. */
-	__virtio_config_val(vdev, offsetof(struct virtio_blk_config, capacity),
-			    &cap);
+	vdev->config->get(vdev, offsetof(struct virtio_blk_config, capacity),
+			  &cap, sizeof(cap));
 
 	/* If capacity is too big, truncate with warning. */
 	if ((sector_t)cap != cap) {

drivers/virtio/virtio_balloon.c +3 -3

diff -r a098f19a6da5 drivers/virtio/virtio_balloon.c
@@ -155,9 +155,9 @@ static inline s64 towards_target(struct 
 static inline s64 towards_target(struct virtio_balloon *vb)
 {
 	u32 v;
-	__virtio_config_val(vb->vdev,
-			    offsetof(struct virtio_balloon_config, num_pages),
-			    &v);
+	vb->vdev->config->get(vb->vdev,
+			      offsetof(struct virtio_balloon_config, num_pages),
+			      &v);
 	return v - vb->num_pages;
 }

include/linux/virtio_config.h +16 ~~-31~~

76 lines not on this page · raw message

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Christian Borntraeger

Acked-by on v1 1/1

2008-04-22 07:44 UTC #1 lore raw

Am Dienstag, 22. April 2008 schrieb Rusty Russell:

Quoted 1 line

[Christian, Hollis, how much is this ABI breakage going to hurt you?]

It is ok for s390 at the moment. We are still working on making userspace ready and I plan to change the guest<->host for s390 anyway. I try to make these changes for drivers/s390/kvm/kvm_virtio.c before 2.6.26. The main reason is, that we are currently limited to around 80 devices. I am not sure, if I should change the allocation of the virtqueues and descriptors to guest memory as well.

Back to your patch:
I have still some ideas about virtio between little endian and big endian systems, but it requires more and different marshalling anyway - even on driver level. No idea yet how to solve that properly.

Consider your change

Acked-by Christian Bornraeger <[email protected]>

given that you fix the issue below:

[...]

drivers/virtio/virtio_balloon.c static inline s64 towards_target(struct · line 160

Quoted 12 lines

drivers/virtio/virtio_balloon.c +3 -3

@@ -155,9 +155,9 @@ static inline s64 towards_target(struct 
 static inline s64 towards_target(struct virtio_balloon *vb)
 {
 	u32 v;
-	__virtio_config_val(vb->vdev,
-			    offsetof(struct virtio_balloon_config, num_pages),
-			    &v);
+	vb->vdev->config->get(vb->vdev,
+			      offsetof(struct virtio_balloon_config, num_pages),
+			      &v);

this is missing a sizeof(v), no?

Christian

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Rusty Russell

author

2008-04-22 14:40 UTC #2 lore raw

Quoted 9 lines · On Tuesday 22 April 2008 17:44:08 Christian Borntraeger wrote:

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
Quoted 1 line
[Christian, Hollis, how much is this ABI breakage going to hurt you?]
It is ok for s390 at the moment. We are still working on making userspace ready and I plan to change the guest<->host for s390 anyway. I try to make these changes for drivers/s390/kvm/kvm_virtio.c before 2.6.26. The main reason is, that we are currently limited to around 80 devices. I am not sure, if I should change the allocation of the virtqueues and descriptors to guest memory as well.

Large rings require contiguous memory, which makes guest allocation problematic. 512 elems at 4k pages == 5 pages.

Quoted 4 lines

Back to your patch:
I have still some ideas about virtio between little endian and big endian systems, but it requires more and different marshalling anyway - even on driver level. No idea yet how to solve that properly.

So far we've pushed such considerations onto the host. This does mean that you can't virtio connect two guests directly without understanding the contents of the buffers so you can endian correct (eg. direct inter-guest networking). inter-guest virtio is currently a party trick anyway, so I'm not sure it's a real issue.

Quoted 5 lines

drivers/virtio/virtio_balloon.c static inline s64 towards_target(struct · line 160
Quoted 3 lines
drivers/virtio/virtio_balloon.c +3 -0
+	vb->vdev->config->get(vb->vdev,
+			      offsetof(struct virtio_balloon_config, num_pages),
+			      &v);
this is missing a sizeof(v), no?

Ah... sure enough, I fixed that in a followon patch. Well-spotted, thanks!

Cheers,
Rusty.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Rusty Russell

author

2008-04-22 14:40 UTC #3 lore raw

In reply to Christian Borntraeger

Quoted 9 lines · On Tuesday 22 April 2008 17:44:08 Christian Borntraeger wrote:

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
Quoted 1 line
[Christian, Hollis, how much is this ABI breakage going to hurt you?]
It is ok for s390 at the moment. We are still working on making userspace ready and I plan to change the guest<->host for s390 anyway. I try to make these changes for drivers/s390/kvm/kvm_virtio.c before 2.6.26. The main reason is, that we are currently limited to around 80 devices. I am not sure, if I should change the allocation of the virtqueues and descriptors to guest memory as well.

Large rings require contiguous memory, which makes guest allocation problematic. 512 elems at 4k pages == 5 pages.

Quoted 4 lines

Back to your patch:
I have still some ideas about virtio between little endian and big endian systems, but it requires more and different marshalling anyway - even on driver level. No idea yet how to solve that properly.

So far we've pushed such considerations onto the host. This does mean that you can't virtio connect two guests directly without understanding the contents of the buffers so you can endian correct (eg. direct inter-guest networking). inter-guest virtio is currently a party trick anyway, so I'm not sure it's a real issue.

Quoted 5 lines

drivers/virtio/virtio_balloon.c static inline s64 towards_target(struct · line 160
Quoted 3 lines
drivers/virtio/virtio_balloon.c +3 -0
+	vb->vdev->config->get(vb->vdev,
+			      offsetof(struct virtio_balloon_config, num_pages),
+			      &v);
this is missing a sizeof(v), no?

Ah... sure enough, I fixed that in a followon patch. Well-spotted, thanks!

Cheers,
Rusty.

Christian Borntraeger

Acked-by on v1 1/1

2008-04-22 07:44 UTC #4 lore raw

In reply to Rusty Russell

Am Dienstag, 22. April 2008 schrieb Rusty Russell:

Quoted 1 line

[Christian, Hollis, how much is this ABI breakage going to hurt you?]

It is ok for s390 at the moment. We are still working on making userspace ready and I plan to change the guest<->host for s390 anyway. I try to make these changes for drivers/s390/kvm/kvm_virtio.c before 2.6.26. The main reason is, that we are currently limited to around 80 devices. I am not sure, if I should change the allocation of the virtqueues and descriptors to guest memory as well.

Back to your patch:
I have still some ideas about virtio between little endian and big endian systems, but it requires more and different marshalling anyway - even on driver level. No idea yet how to solve that properly.

Consider your change

Acked-by Christian Bornraeger <[email protected]>

given that you fix the issue below:

[...]

drivers/virtio/virtio_balloon.c static inline s64 towards_target(struct · line 160

Quoted 12 lines

drivers/virtio/virtio_balloon.c +3 -3

@@ -155,9 +155,9 @@ static inline s64 towards_target(struct 
 static inline s64 towards_target(struct virtio_balloon *vb)
 {
 	u32 v;
-	__virtio_config_val(vb->vdev,
-			    offsetof(struct virtio_balloon_config, num_pages),
-			    &v);
+	vb->vdev->config->get(vb->vdev,
+			      offsetof(struct virtio_balloon_config, num_pages),
+			      &v);

this is missing a sizeof(v), no?

Christian

Avi Kivity

2008-04-22 11:22 UTC #5 lore raw

In reply to Rusty Russell

Quoted 6 lines · Rusty Russell

[Christian, Hollis, how much is this ABI breakage going to hurt you?]
A recent proposed feature addition to the virtio block driver revealed some flaws in the API, in particular how easy it is to break big endian machines.

Quoted 8 lines

The virtio config space was originally chosen to be little-endian, because we thought the config might be part of the PCI config space for virtio_pci. It's actually a separate mmio region, so that argument holds little water; as only x86 is currently using the virtio mechanism, we can change this (but must do so now, before the impending s390 and ppc merges).

This will probably annoy Hollis which has guests that can go both ways.

Signature · 8 lines

error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Rusty Russell

author

2008-04-22 14:31 UTC #6 lore raw

Quoted 8 lines · On Tuesday 22 April 2008 21:22:48 Avi Kivity wrote:

Quoted 6 lines
The virtio config space was originally chosen to be little-endian, because we thought the config might be part of the PCI config space for virtio_pci. It's actually a separate mmio region, so that argument holds little water; as only x86 is currently using the virtio mechanism, we can change this (but must do so now, before the impending s390 and ppc merges).
This will probably annoy Hollis which has guests that can go both ways.

Yes, I discussed this with Hollis. But the virtio rings themselves already have this issue: we don't do any endian conversion on them and assume they're "our" endian in the guest.

We may still regret not doing *everything* little-endian, but this doesn't make it worse.

Thanks,
Rusty.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Hollis Blanchard

2008-04-22 20:29 UTC #7 lore raw

Quoted 8 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:

Quoted 7 lines · On Tuesday 22 April 2008 21:22:48 Avi Kivity wrote:
Quoted 6 lines
The virtio config space was originally chosen to be little-endian, because we thought the config might be part of the PCI config space for virtio_pci. It's actually a separate mmio region, so that argument holds little water; as only x86 is currently using the virtio mechanism, we can change this (but must do so now, before the impending s390 and ppc merges).

Quoted 8 lines

Quoted 1 line
This will probably annoy Hollis which has guests that can go both ways.
Yes, I discussed this with Hollis. But the virtio rings themselves already have this issue: we don't do any endian conversion on them and assume they're "our" endian in the guest.
We may still regret not doing *everything* little-endian, but this doesn't make it worse.

Hmm, why *don't* we just do everything LE, including the ring?

Signature · 2 lines

Hollis Blanchard
IBM Linux Technology Center

Rusty Russell

author

2008-04-22 21:05 UTC #8 lore raw

Quoted 5 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:

Quoted 2 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.
Hmm, why *don't* we just do everything LE, including the ring?

Mainly because when requirements are in doubt, simplicity wins, I think.

Cheers,
Rusty.

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Hollis Blanchard

2008-04-22 22:08 UTC #9 lore raw

Quoted 8 lines · On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:

Quoted 5 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
Quoted 2 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.
Hmm, why *don't* we just do everything LE, including the ring?
Mainly because when requirements are in doubt, simplicity wins, I think.

Well, I think the definition of simplicity is up for debate in this case... "LE everywhere" is much simpler than "it depends", IMHO.

Signature · 8 lines

Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Anthony Liguori

2008-04-22 22:13 UTC #10 lore raw

Quoted 9 lines · Hollis Blanchard

Quoted 7 lines · On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
Quoted 5 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
Quoted 3 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Hmm, why *don't* we just do everything LE, including the ring?
Mainly because when requirements are in doubt, simplicity wins, I think.
Well, I think the definition of simplicity is up for debate in this case... "LE everywhere" is much simpler than "it depends", IMHO.

You couldn't use the vringfd direct ring mapping optimization in KVM for PPC without teaching the kernel to access a vring in LE format. I'm pretty sure the later would get rejected on LKML anyway for vringfd as a generic mechanism.

Regards,

Anthony Liguori

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Hollis Blanchard

2008-04-22 22:33 UTC #11 lore raw

Quoted 15 lines · On Tuesday 22 April 2008 17:13:01 Anthony Liguori wrote:

Quoted 14 lines · Hollis Blanchard
Quoted 11 lines · On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
Quoted 7 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
Quoted 3 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.
Hmm, why *don't* we just do everything LE, including the ring?
Mainly because when requirements are in doubt, simplicity wins, I think.

Quoted 7 lines

Quoted 2 lines
Well, I think the definition of simplicity is up for debate in this case... "LE everywhere" is much simpler than "it depends", IMHO.
You couldn't use the vringfd direct ring mapping optimization in KVM for PPC without teaching the kernel to access a vring in LE format. I'm pretty sure the later would get rejected on LKML anyway for vringfd as a generic mechanism.

You mean vringfd for use cases other than virtual IO drivers? I have a poor imagination; can you give some examples?

Even then, it should be possible to have VIO drivers use a different set of accessors, just like there are swapping and non-swapping accessors for real IO, so I still don't see the problem.

Signature · 8 lines

Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Hollis Blanchard

2008-04-22 22:33 UTC #12 lore raw

In reply to Anthony Liguori

Quoted 15 lines · On Tuesday 22 April 2008 17:13:01 Anthony Liguori wrote:

Quoted 14 lines · Hollis Blanchard
Quoted 11 lines · On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
Quoted 7 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
Quoted 3 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.
Hmm, why *don't* we just do everything LE, including the ring?
Mainly because when requirements are in doubt, simplicity wins, I think.

Quoted 7 lines

Quoted 2 lines
Well, I think the definition of simplicity is up for debate in this case... "LE everywhere" is much simpler than "it depends", IMHO.
You couldn't use the vringfd direct ring mapping optimization in KVM for PPC without teaching the kernel to access a vring in LE format. I'm pretty sure the later would get rejected on LKML anyway for vringfd as a generic mechanism.

You mean vringfd for use cases other than virtual IO drivers? I have a poor imagination; can you give some examples?

Even then, it should be possible to have VIO drivers use a different set of accessors, just like there are swapping and non-swapping accessors for real IO, so I still don't see the problem.

Signature · 2 lines

Hollis Blanchard
IBM Linux Technology Center

Hollis Blanchard

2008-04-22 23:53 UTC #13 lore raw

In reply to Anthony Liguori

Quoted 15 lines · On Tuesday 22 April 2008 17:13:01 Anthony Liguori wrote:

Quoted 14 lines · Hollis Blanchard
Quoted 11 lines · On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
Quoted 7 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
Quoted 3 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.
Hmm, why *don't* we just do everything LE, including the ring?
Mainly because when requirements are in doubt, simplicity wins, I think.

Quoted 8 lines

Quoted 3 lines
Well, I think the definition of simplicity is up for debate in this case... "LE everywhere" is much simpler than "it depends", IMHO.
You couldn't use the vringfd direct ring mapping optimization in KVM for PPC without teaching the kernel to access a vring in LE format. I'm pretty sure the later would get rejected on LKML anyway for vringfd as a generic mechanism.

(Since the IA64 guys have already implemented BE guests on LE hosts, they should be aware of this discussion too, which is why I've CCed them.)

After a short but torturous whiteboard session, followed by a much longer but less painful discussion, I'm fine with the virtio device config space being BE for PowerPC and LE for x86.

In the future, we can use a feature bit to indicate that PCI config space contains an explicit endianness flag. (This will be set to BE or LE, *not* to "opposite of normal", because "normal" is also too vague.)

Signature · 8 lines

Hollis Blanchard
IBM Linux Technology Center

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Hollis Blanchard

2008-04-22 23:53 UTC #14 lore raw

In reply to Anthony Liguori

Quoted 15 lines · On Tuesday 22 April 2008 17:13:01 Anthony Liguori wrote:

Quoted 14 lines · Hollis Blanchard
Quoted 11 lines · On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
Quoted 7 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
Quoted 3 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.
Hmm, why *don't* we just do everything LE, including the ring?
Mainly because when requirements are in doubt, simplicity wins, I think.

Quoted 8 lines

Quoted 3 lines
Well, I think the definition of simplicity is up for debate in this case... "LE everywhere" is much simpler than "it depends", IMHO.
You couldn't use the vringfd direct ring mapping optimization in KVM for PPC without teaching the kernel to access a vring in LE format. I'm pretty sure the later would get rejected on LKML anyway for vringfd as a generic mechanism.

(Since the IA64 guys have already implemented BE guests on LE hosts, they should be aware of this discussion too, which is why I've CCed them.)

After a short but torturous whiteboard session, followed by a much longer but less painful discussion, I'm fine with the virtio device config space being BE for PowerPC and LE for x86.

In the future, we can use a feature bit to indicate that PCI config space contains an explicit endianness flag. (This will be set to BE or LE, *not* to "opposite of normal", because "normal" is also too vague.)

Signature · 2 lines

Hollis Blanchard
IBM Linux Technology Center

Anthony Liguori

2008-04-22 22:13 UTC #15 lore raw

In reply to Hollis Blanchard

Quoted 9 lines · Hollis Blanchard

Quoted 7 lines · On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
Quoted 5 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
Quoted 3 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Hmm, why *don't* we just do everything LE, including the ring?
Mainly because when requirements are in doubt, simplicity wins, I think.
Well, I think the definition of simplicity is up for debate in this case... "LE everywhere" is much simpler than "it depends", IMHO.

You couldn't use the vringfd direct ring mapping optimization in KVM for PPC without teaching the kernel to access a vring in LE format. I'm pretty sure the later would get rejected on LKML anyway for vringfd as a generic mechanism.

Regards,

Anthony Liguori

Hollis Blanchard

2008-04-22 22:08 UTC #16 lore raw

In reply to Rusty Russell

Quoted 8 lines · On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:

Quoted 5 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
Quoted 2 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.
Hmm, why *don't* we just do everything LE, including the ring?
Mainly because when requirements are in doubt, simplicity wins, I think.

Well, I think the definition of simplicity is up for debate in this case... "LE everywhere" is much simpler than "it depends", IMHO.

Signature · 2 lines

Hollis Blanchard
IBM Linux Technology Center

Rusty Russell

author

2008-04-22 21:05 UTC #17 lore raw

In reply to Hollis Blanchard

Quoted 5 lines · On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:

Quoted 2 lines · On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
We may still regret not doing *everything* little-endian, but this doesn't make it worse.
Hmm, why *don't* we just do everything LE, including the ring?

Mainly because when requirements are in doubt, simplicity wins, I think.

Cheers,
Rusty.

Rusty Russell

author

2008-04-22 14:31 UTC #18 lore raw

In reply to Avi Kivity

Quoted 8 lines · On Tuesday 22 April 2008 21:22:48 Avi Kivity wrote:

Quoted 6 lines
The virtio config space was originally chosen to be little-endian, because we thought the config might be part of the PCI config space for virtio_pci. It's actually a separate mmio region, so that argument holds little water; as only x86 is currently using the virtio mechanism, we can change this (but must do so now, before the impending s390 and ppc merges).
This will probably annoy Hollis which has guests that can go both ways.

Yes, I discussed this with Hollis. But the virtio rings themselves already have this issue: we don't do any endian conversion on them and assume they're "our" endian in the guest.

We may still regret not doing *everything* little-endian, but this doesn't make it worse.

Thanks,
Rusty.

Hollis Blanchard

2008-04-22 16:21 UTC #19 lore raw

In reply to Avi Kivity

Quoted 7 lines · On Tuesday 22 April 2008 06:22:48 Avi Kivity wrote:

Quoted 6 lines · Rusty Russell
[Christian, Hollis, how much is this ABI breakage going to hurt you?]
A recent proposed feature addition to the virtio block driver revealed some flaws in the API, in particular how easy it is to break big endian machines.

Quoted 8 lines

Quoted 6 lines
The virtio config space was originally chosen to be little-endian, because we thought the config might be part of the PCI config space for virtio_pci. It's actually a separate mmio region, so that argument holds little water; as only x86 is currently using the virtio mechanism, we can change this (but must do so now, before the impending s390 and ppc merges).
This will probably annoy Hollis which has guests that can go both ways.

Rusty and I have discussed it. Ultimately, this just takes us from a cross-architecture endianness definition to a per-architecture definition. Anyways, we've already fallen into this situation with the virtio ring data itself, so we're really saying "same endianness as the ring".

Signature · 2 lines

Hollis Blanchard
IBM Linux Technology Center

Avi Kivity

2008-04-22 11:22 UTC #20 lore raw

In reply to Rusty Russell

Quoted 6 lines · Rusty Russell

[Christian, Hollis, how much is this ABI breakage going to hurt you?]
A recent proposed feature addition to the virtio block driver revealed some flaws in the API, in particular how easy it is to break big endian machines.

Quoted 8 lines

The virtio config space was originally chosen to be little-endian, because we thought the config might be part of the PCI config space for virtio_pci. It's actually a separate mmio region, so that argument holds little water; as only x86 is currently using the virtio mechanism, we can change this (but must do so now, before the impending s390 and ppc merges).

This will probably annoy Hollis which has guests that can go both ways.

Signature · 1 line

error compiling committee.c: too many arguments to function

Christian Borntraeger

2008-04-23 10:55 UTC #21 lore raw

In reply to Rusty Russell

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
[...]

include/linux/virtio_config.h line 19

Quoted 9 lines

diff -r a098f19a6da5 include/linux/virtio_config.h
include/linux/virtio_config.h +1 -1
@@ -16,7 +16,7 @@
 #define VIRTIO_CONFIG_S_FAILED		0x80
 
 #ifdef __KERNEL__
-struct virtio_device;
+#include <linux/virtio.h>

I just realized, that this breaks make headers_check as we dont export virtio.h (and we dont want to export it as it relies on scatterlist.h).

Christian

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Avi Kivity

2008-04-23 12:38 UTC #22 lore raw

Quoted 9 lines · Christian Borntraeger

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
[...]
Quoted 6 lines
diff -r a098f19a6da5 include/linux/virtio_config.h
include/linux/virtio_config.h +0 -0
@@ -16,7 +16,7 @@
 #define VIRTIO_CONFIG_S_FAILED		0x80
 

Quoted 9 lines

include/linux/virtio_config.h line 20
Quoted 4 lines
include/linux/virtio_config.h +1 -1
 #ifdef __KERNEL__
-struct virtio_device;
+#include <linux/virtio.h>
 
I just realized, that this breaks make headers_check as we dont export virtio.h (and we dont want to export it as it relies on scatterlist.h).

It's guarded by a #ifdef __KERNEL__, so it should be alright.

Signature · 8 lines

Do not meddle in the internals of kernels, for they are subtle and quick to panic.


-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Christian Borntraeger

2008-04-23 12:47 UTC #23 lore raw

Am Mittwoch, 23. April 2008 schrieb Avi Kivity:

Quoted 12 lines

Quoted 11 lines
include/linux/virtio_config.h line 19
Quoted 6 lines
#define VIRTIO_CONFIG_S_FAILED 0x80
#ifdef __KERNEL__
include/linux/virtio_config.h +1 -1
-struct virtio_device;
+#include <linux/virtio.h>
I just realized, that this breaks make headers_check as we dont export virtio.h (and we dont want to export it as it relies on scatterlist.h).

Quoted 1 line

It's guarded by a #ifdef __KERNEL__, so it should be alright.

Yes, you are right. Thanks

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone

Christian Borntraeger

2008-04-23 12:47 UTC #24 lore raw

In reply to Avi Kivity

Am Mittwoch, 23. April 2008 schrieb Avi Kivity:

Quoted 12 lines

Quoted 11 lines
include/linux/virtio_config.h line 19
Quoted 6 lines
#define VIRTIO_CONFIG_S_FAILED 0x80
#ifdef __KERNEL__
include/linux/virtio_config.h +1 -1
-struct virtio_device;
+#include <linux/virtio.h>
I just realized, that this breaks make headers_check as we dont export virtio.h (and we dont want to export it as it relies on scatterlist.h).

Quoted 1 line

It's guarded by a #ifdef __KERNEL__, so it should be alright.

Yes, you are right. Thanks

Avi Kivity

2008-04-23 12:38 UTC #25 lore raw

In reply to Christian Borntraeger

Quoted 9 lines · Christian Borntraeger

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
[...]
Quoted 6 lines
diff -r a098f19a6da5 include/linux/virtio_config.h
include/linux/virtio_config.h +0 -0
@@ -16,7 +16,7 @@
 #define VIRTIO_CONFIG_S_FAILED		0x80
 

Quoted 9 lines

include/linux/virtio_config.h line 20
Quoted 4 lines
include/linux/virtio_config.h +1 -1
 #ifdef __KERNEL__
-struct virtio_device;
+#include <linux/virtio.h>
 
I just realized, that this breaks make headers_check as we dont export virtio.h (and we dont want to export it as it relies on scatterlist.h).

It's guarded by a #ifdef __KERNEL__, so it should be alright.

Signature · 1 line

Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Rusty Russell

author

2008-04-23 15:53 UTC #26 lore raw

In reply to Christian Borntraeger

Quoted 9 lines · On Wednesday 23 April 2008 20:55:50 Christian Borntraeger wrote:

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
[...]
Quoted 6 lines
diff -r a098f19a6da5 include/linux/virtio_config.h
include/linux/virtio_config.h +0 -0
@@ -16,7 +16,7 @@
 #define VIRTIO_CONFIG_S_FAILED		0x80
 

Quoted 6 lines

include/linux/virtio_config.h line 19
Quoted 3 lines
include/linux/virtio_config.h +1 -1
 #ifdef __KERNEL__
-struct virtio_device;
+#include <linux/virtio.h>
I just realized, that this breaks make headers_check as we dont export virtio.h (and we dont want to export it as it relies on scatterlist.h).

It's under #ifdef __KERNEL__ though?

Cheers,
Rusty.

Christian Borntraeger

2008-04-23 10:55 UTC #27 lore raw

In reply to Rusty Russell

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
[...]

include/linux/virtio_config.h line 19

Quoted 9 lines

diff -r a098f19a6da5 include/linux/virtio_config.h
include/linux/virtio_config.h +1 -1
@@ -16,7 +16,7 @@
 #define VIRTIO_CONFIG_S_FAILED		0x80
 
 #ifdef __KERNEL__
-struct virtio_device;
+#include <linux/virtio.h>

I just realized, that this breaks make headers_check as we dont export virtio.h (and we dont want to export it as it relies on scatterlist.h).

Christian