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

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

[Christian Borntraeger]

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
> [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 <borntraeger@de.ibm.com>
given that you fix the issue below:

[...]
> --- a/drivers/virtio/virtio_balloon.c	Sun Apr 20 14:41:02 2008 +1000
> +++ b/drivers/virtio/virtio_balloon.c	Sun Apr 20 15:07:45 2008 +1000
> @@ -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

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Avi Kivity]

Rusty Russell wrote:
> [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).
>
>   

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

-- 
error compiling committee.c: too many arguments to function

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

[Rusty Russell]

On Tuesday 22 April 2008 17:44:08 Christian Borntraeger wrote:
> Am Dienstag, 22. April 2008 schrieb Rusty Russell:
> > [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.

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

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

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Rusty Russell]

On Tuesday 22 April 2008 21:22:48 Avi Kivity wrote:
> > 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.

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Hollis Blanchard]

On Tuesday 22 April 2008 06:22:48 Avi Kivity wrote:
> Rusty Russell wrote:
> > [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).
> 
> 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".

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Hollis Blanchard]

On Tuesday 22 April 2008 09:31:35 Rusty Russell wrote:
> On Tuesday 22 April 2008 21:22:48 Avi Kivity wrote:
> > > 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.

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

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Rusty Russell]

On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
> 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.

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Hollis Blanchard]

On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
> On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
> > 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.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Anthony Liguori]

Hollis Blanchard wrote:
> On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
>   
>> On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
>>     
>>> 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.
>   

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

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Hollis Blanchard]

On Tuesday 22 April 2008 17:13:01 Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
> >   
> >> On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
> >>     
> >>> 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.
> 
> 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.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Hollis Blanchard]

On Tuesday 22 April 2008 17:13:01 Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > On Tuesday 22 April 2008 16:05:38 Rusty Russell wrote:
> >   
> >> On Wednesday 23 April 2008 06:29:14 Hollis Blanchard wrote:
> >>     
> >>> 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.
> >   
> 
> 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.)

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Christian Borntraeger]

Am Dienstag, 22. April 2008 schrieb Rusty Russell:
[...]
> diff -r a098f19a6da5 include/linux/virtio_config.h
> --- a/include/linux/virtio_config.h	Sun Apr 20 14:41:02 2008 +1000
> +++ b/include/linux/virtio_config.h	Sun Apr 20 15:07:45 2008 +1000
> @@ -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

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Avi Kivity]

Christian Borntraeger wrote:
> Am Dienstag, 22. April 2008 schrieb Rusty Russell:
> [...]
>   
>> diff -r a098f19a6da5 include/linux/virtio_config.h
>> --- a/include/linux/virtio_config.h	Sun Apr 20 14:41:02 2008 +1000
>> +++ b/include/linux/virtio_config.h	Sun Apr 20 15:07:45 2008 +1000
>> @@ -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).
>
>   

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

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

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Christian Borntraeger]

Am Mittwoch, 23. April 2008 schrieb Avi Kivity:
> >>  #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).
> >
> >   
> 
> It's guarded by a #ifdef __KERNEL__, so it should be alright.

Yes, you are right. Thanks

Re: [kvm-devel] [RFC PATCH] virtio: change config to guest endian.

[Rusty Russell]

On Wednesday 23 April 2008 20:55:50 Christian Borntraeger wrote:
> Am Dienstag, 22. April 2008 schrieb Rusty Russell:
> [...]
>
> > diff -r a098f19a6da5 include/linux/virtio_config.h
> > --- a/include/linux/virtio_config.h	Sun Apr 20 14:41:02 2008 +1000
> > +++ b/include/linux/virtio_config.h	Sun Apr 20 15:07:45 2008 +1000
> > @@ -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).

It's under #ifdef __KERNEL__ though?

Cheers,
Rusty.