| Message ID | 20140218123849.9849.77875.stgit@bahia.lab.toulouse-stg.fr.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 02/18/2014 01:38 PM, Greg Kurz wrote: > From: Rusty Russell <rusty@rustcorp.com.au> > > virtio data structures are defined as "target endian", which assumes > that's a fixed value. In fact, that actually means it's > platform-specific. > > The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for > little endian ppc (and potentially ARM). This is called at device > reset time (which is done before any driver is loaded) since it > may involve a system call to get the status when running under kvm. > > [ fixed checkpatch.pl error with the virtio_byteswap initialisation, > ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > --- > hw/virtio/virtio.c | 6 ++ > include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ > include/hw/virtio/virtio.h | 2 + > stubs/Makefile.objs | 1 > stubs/virtio_get_byteswap.c | 6 ++ > 5 files changed, 147 insertions(+) > create mode 100644 include/hw/virtio/virtio-access.h > create mode 100644 stubs/virtio_get_byteswap.c > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index aeabf3a..4fd6ac2 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -19,6 +19,9 @@ > #include "hw/virtio/virtio.h" > #include "qemu/atomic.h" > #include "hw/virtio/virtio-bus.h" > +#include "hw/virtio/virtio-access.h" > + > +bool virtio_byteswap; Could this be a virtio object property rather than a global? Imagine an AMP guest system with a BE and an LE system running in parallel accessing two separate virtio devices. With a single global that would break. Alex
On 02/18/2014 04:03 PM, Michael S. Tsirkin wrote: > On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: >> On 02/18/2014 01:38 PM, Greg Kurz wrote: >>> From: Rusty Russell <rusty@rustcorp.com.au> >>> >>> virtio data structures are defined as "target endian", which assumes >>> that's a fixed value. In fact, that actually means it's >>> platform-specific. >>> >>> The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for >>> little endian ppc (and potentially ARM). This is called at device >>> reset time (which is done before any driver is loaded) since it >>> may involve a system call to get the status when running under kvm. >>> >>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation, >>> ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] >>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> >>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >>> --- >>> hw/virtio/virtio.c | 6 ++ >>> include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ >>> include/hw/virtio/virtio.h | 2 + >>> stubs/Makefile.objs | 1 >>> stubs/virtio_get_byteswap.c | 6 ++ >>> 5 files changed, 147 insertions(+) >>> create mode 100644 include/hw/virtio/virtio-access.h >>> create mode 100644 stubs/virtio_get_byteswap.c >>> >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index aeabf3a..4fd6ac2 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -19,6 +19,9 @@ >>> #include "hw/virtio/virtio.h" >>> #include "qemu/atomic.h" >>> #include "hw/virtio/virtio-bus.h" >>> +#include "hw/virtio/virtio-access.h" >>> + >>> +bool virtio_byteswap; >> Could this be a virtio object property rather than a global? Imagine >> an AMP guest system with a BE and an LE system running in parallel >> accessing two separate virtio devices. With a single global that >> would break. >> >> >> Alex > Well, how does a device know which CPU uses it? > I suspect we are better off waiting for 1.0 with this one. Good point. It just feels like a heavy layering violation to have a global variable for all virtio devices. What if we start mixing legacy and 1.0 devices? We could reuse the same flag, but it would be initialized differently per device. Alex
On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: > On 02/18/2014 01:38 PM, Greg Kurz wrote: > >From: Rusty Russell <rusty@rustcorp.com.au> > > > >virtio data structures are defined as "target endian", which assumes > >that's a fixed value. In fact, that actually means it's > >platform-specific. > > > >The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for > >little endian ppc (and potentially ARM). This is called at device > >reset time (which is done before any driver is loaded) since it > >may involve a system call to get the status when running under kvm. > > > >[ fixed checkpatch.pl error with the virtio_byteswap initialisation, > > ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] > >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > >Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > >--- > > hw/virtio/virtio.c | 6 ++ > > include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ > > include/hw/virtio/virtio.h | 2 + > > stubs/Makefile.objs | 1 > > stubs/virtio_get_byteswap.c | 6 ++ > > 5 files changed, 147 insertions(+) > > create mode 100644 include/hw/virtio/virtio-access.h > > create mode 100644 stubs/virtio_get_byteswap.c > > > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >index aeabf3a..4fd6ac2 100644 > >--- a/hw/virtio/virtio.c > >+++ b/hw/virtio/virtio.c > >@@ -19,6 +19,9 @@ > > #include "hw/virtio/virtio.h" > > #include "qemu/atomic.h" > > #include "hw/virtio/virtio-bus.h" > >+#include "hw/virtio/virtio-access.h" > >+ > >+bool virtio_byteswap; > > Could this be a virtio object property rather than a global? Imagine > an AMP guest system with a BE and an LE system running in parallel > accessing two separate virtio devices. With a single global that > would break. > > > Alex Well, how does a device know which CPU uses it? I suspect we are better off waiting for 1.0 with this one.
On Tue, Feb 18, 2014 at 05:03:27PM +0200, Michael S. Tsirkin wrote: > On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: > > On 02/18/2014 01:38 PM, Greg Kurz wrote: > > >From: Rusty Russell <rusty@rustcorp.com.au> > > > > > >virtio data structures are defined as "target endian", which assumes > > >that's a fixed value. In fact, that actually means it's > > >platform-specific. > > > > > >The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for > > >little endian ppc (and potentially ARM). This is called at device > > >reset time (which is done before any driver is loaded) since it > > >may involve a system call to get the status when running under kvm. > > > > > >[ fixed checkpatch.pl error with the virtio_byteswap initialisation, > > > ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] > > >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > >Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > >--- > > > hw/virtio/virtio.c | 6 ++ > > > include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ > > > include/hw/virtio/virtio.h | 2 + > > > stubs/Makefile.objs | 1 > > > stubs/virtio_get_byteswap.c | 6 ++ > > > 5 files changed, 147 insertions(+) > > > create mode 100644 include/hw/virtio/virtio-access.h > > > create mode 100644 stubs/virtio_get_byteswap.c > > > > > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > >index aeabf3a..4fd6ac2 100644 > > >--- a/hw/virtio/virtio.c > > >+++ b/hw/virtio/virtio.c > > >@@ -19,6 +19,9 @@ > > > #include "hw/virtio/virtio.h" > > > #include "qemu/atomic.h" > > > #include "hw/virtio/virtio-bus.h" > > >+#include "hw/virtio/virtio-access.h" > > >+ > > >+bool virtio_byteswap; > > > > Could this be a virtio object property rather than a global? Imagine > > an AMP guest system with a BE and an LE system running in parallel > > accessing two separate virtio devices. With a single global that > > would break. > > > > > > Alex > > Well, how does a device know which CPU uses it? > I suspect we are better off waiting for 1.0 with this one. I mean an AMP guest system with a BE and an LE system here. Not this patchset itself. > -- > MST
On 02/18/2014 04:11 PM, Michael S. Tsirkin wrote: > On Tue, Feb 18, 2014 at 04:02:18PM +0100, Alexander Graf wrote: >> On 02/18/2014 04:03 PM, Michael S. Tsirkin wrote: >>> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: >>>> On 02/18/2014 01:38 PM, Greg Kurz wrote: >>>>> From: Rusty Russell <rusty@rustcorp.com.au> >>>>> >>>>> virtio data structures are defined as "target endian", which assumes >>>>> that's a fixed value. In fact, that actually means it's >>>>> platform-specific. >>>>> >>>>> The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for >>>>> little endian ppc (and potentially ARM). This is called at device >>>>> reset time (which is done before any driver is loaded) since it >>>>> may involve a system call to get the status when running under kvm. >>>>> >>>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation, >>>>> ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] >>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> >>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >>>>> --- >>>>> hw/virtio/virtio.c | 6 ++ >>>>> include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ >>>>> include/hw/virtio/virtio.h | 2 + >>>>> stubs/Makefile.objs | 1 >>>>> stubs/virtio_get_byteswap.c | 6 ++ >>>>> 5 files changed, 147 insertions(+) >>>>> create mode 100644 include/hw/virtio/virtio-access.h >>>>> create mode 100644 stubs/virtio_get_byteswap.c >>>>> >>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>>> index aeabf3a..4fd6ac2 100644 >>>>> --- a/hw/virtio/virtio.c >>>>> +++ b/hw/virtio/virtio.c >>>>> @@ -19,6 +19,9 @@ >>>>> #include "hw/virtio/virtio.h" >>>>> #include "qemu/atomic.h" >>>>> #include "hw/virtio/virtio-bus.h" >>>>> +#include "hw/virtio/virtio-access.h" >>>>> + >>>>> +bool virtio_byteswap; >>>> Could this be a virtio object property rather than a global? Imagine >>>> an AMP guest system with a BE and an LE system running in parallel >>>> accessing two separate virtio devices. With a single global that >>>> would break. >>>> >>>> >>>> Alex >>> Well, how does a device know which CPU uses it? >>> I suspect we are better off waiting for 1.0 with this one. >> Good point. It just feels like a heavy layering violation to have a >> global variable for all virtio devices. What if we start mixing >> legacy and 1.0 devices? We could reuse the same flag, but it would >> be initialized differently per device. >> >> >> Alex > 1.0 won't use the flag, it's all LE. At which point we need another flag indicating that the fields are always LE which means we can reuse this flag, no? Alex
On Tue, Feb 18, 2014 at 04:02:18PM +0100, Alexander Graf wrote: > On 02/18/2014 04:03 PM, Michael S. Tsirkin wrote: > >On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: > >>On 02/18/2014 01:38 PM, Greg Kurz wrote: > >>>From: Rusty Russell <rusty@rustcorp.com.au> > >>> > >>>virtio data structures are defined as "target endian", which assumes > >>>that's a fixed value. In fact, that actually means it's > >>>platform-specific. > >>> > >>>The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for > >>>little endian ppc (and potentially ARM). This is called at device > >>>reset time (which is done before any driver is loaded) since it > >>>may involve a system call to get the status when running under kvm. > >>> > >>>[ fixed checkpatch.pl error with the virtio_byteswap initialisation, > >>> ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] > >>>Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > >>>Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > >>>--- > >>> hw/virtio/virtio.c | 6 ++ > >>> include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ > >>> include/hw/virtio/virtio.h | 2 + > >>> stubs/Makefile.objs | 1 > >>> stubs/virtio_get_byteswap.c | 6 ++ > >>> 5 files changed, 147 insertions(+) > >>> create mode 100644 include/hw/virtio/virtio-access.h > >>> create mode 100644 stubs/virtio_get_byteswap.c > >>> > >>>diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >>>index aeabf3a..4fd6ac2 100644 > >>>--- a/hw/virtio/virtio.c > >>>+++ b/hw/virtio/virtio.c > >>>@@ -19,6 +19,9 @@ > >>> #include "hw/virtio/virtio.h" > >>> #include "qemu/atomic.h" > >>> #include "hw/virtio/virtio-bus.h" > >>>+#include "hw/virtio/virtio-access.h" > >>>+ > >>>+bool virtio_byteswap; > >>Could this be a virtio object property rather than a global? Imagine > >>an AMP guest system with a BE and an LE system running in parallel > >>accessing two separate virtio devices. With a single global that > >>would break. > >> > >> > >>Alex > >Well, how does a device know which CPU uses it? > >I suspect we are better off waiting for 1.0 with this one. > > Good point. It just feels like a heavy layering violation to have a > global variable for all virtio devices. What if we start mixing > legacy and 1.0 devices? We could reuse the same flag, but it would > be initialized differently per device. > > > Alex 1.0 won't use the flag, it's all LE.
On Tue, 18 Feb 2014 17:03:27 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: > > On 02/18/2014 01:38 PM, Greg Kurz wrote: > > >From: Rusty Russell <rusty@rustcorp.com.au> > > > > > >virtio data structures are defined as "target endian", which assumes > > >that's a fixed value. In fact, that actually means it's > > >platform-specific. > > > > > >The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for > > >little endian ppc (and potentially ARM). This is called at device > > >reset time (which is done before any driver is loaded) since it > > >may involve a system call to get the status when running under kvm. > > > > > >[ fixed checkpatch.pl error with the virtio_byteswap initialisation, > > > ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] > > >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > >Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > >--- > > > hw/virtio/virtio.c | 6 ++ > > > include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ > > > include/hw/virtio/virtio.h | 2 + > > > stubs/Makefile.objs | 1 > > > stubs/virtio_get_byteswap.c | 6 ++ > > > 5 files changed, 147 insertions(+) > > > create mode 100644 include/hw/virtio/virtio-access.h > > > create mode 100644 stubs/virtio_get_byteswap.c > > > > > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > >index aeabf3a..4fd6ac2 100644 > > >--- a/hw/virtio/virtio.c > > >+++ b/hw/virtio/virtio.c > > >@@ -19,6 +19,9 @@ > > > #include "hw/virtio/virtio.h" > > > #include "qemu/atomic.h" > > > #include "hw/virtio/virtio-bus.h" > > >+#include "hw/virtio/virtio-access.h" > > >+ > > >+bool virtio_byteswap; > > > > Could this be a virtio object property rather than a global? Imagine > > an AMP guest system with a BE and an LE system running in parallel > > accessing two separate virtio devices. With a single global that > > would break. > > > > > > Alex > > Well, how does a device know which CPU uses it? > I suspect we are better off waiting for 1.0 with this one. > 1.0 makes this a bit more complex, no? virtio-endian accessors are defined by the endianness of host and guest (doing a bswap depends on the host/guest combination). This needs to be per qemu instance. (ioctl under kvm? machine option?) For 1.0, we'll have everything le, so a be host will always do a bswap (as will a be guest). But whether a device is 1.0 or legacy is not something that can be decided globally, or we can't have transitional devices with qemu.
On Tue, 18 Feb 2014 16:12:08 +0100 Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > On Tue, 18 Feb 2014 17:03:27 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: > > > On 02/18/2014 01:38 PM, Greg Kurz wrote: > > > >From: Rusty Russell <rusty@rustcorp.com.au> > > > > > > > >virtio data structures are defined as "target endian", which assumes > > > >that's a fixed value. In fact, that actually means it's > > > >platform-specific. > > > > > > > >The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for > > > >little endian ppc (and potentially ARM). This is called at device > > > >reset time (which is done before any driver is loaded) since it > > > >may involve a system call to get the status when running under kvm. > > > > > > > >[ fixed checkpatch.pl error with the virtio_byteswap initialisation, > > > > ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] > > > >Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > > > >Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > > > >--- > > > > hw/virtio/virtio.c | 6 ++ > > > > include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ > > > > include/hw/virtio/virtio.h | 2 + > > > > stubs/Makefile.objs | 1 > > > > stubs/virtio_get_byteswap.c | 6 ++ > > > > 5 files changed, 147 insertions(+) > > > > create mode 100644 include/hw/virtio/virtio-access.h > > > > create mode 100644 stubs/virtio_get_byteswap.c > > > > > > > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > > >index aeabf3a..4fd6ac2 100644 > > > >--- a/hw/virtio/virtio.c > > > >+++ b/hw/virtio/virtio.c > > > >@@ -19,6 +19,9 @@ > > > > #include "hw/virtio/virtio.h" > > > > #include "qemu/atomic.h" > > > > #include "hw/virtio/virtio-bus.h" > > > >+#include "hw/virtio/virtio-access.h" > > > >+ > > > >+bool virtio_byteswap; > > > > > > Could this be a virtio object property rather than a global? Imagine > > > an AMP guest system with a BE and an LE system running in parallel > > > accessing two separate virtio devices. With a single global that > > > would break. > > > > > > > > > Alex > > > > Well, how does a device know which CPU uses it? > > I suspect we are better off waiting for 1.0 with this one. > > > > 1.0 makes this a bit more complex, no? > > virtio-endian accessors are defined by the endianness of host and guest > (doing a bswap depends on the host/guest combination). This needs to be > per qemu instance. (ioctl under kvm? machine option?) > > For 1.0, we'll have everything le, so a be host will always do a bswap > (as will a be guest). But whether a device is 1.0 or legacy is not > something that can be decided globally, or we can't have transitional > devices with qemu. > So here are two stupid tables on who needs to do byteswaps, one for legacy devices, one for 1.0 devices: legacy devices: host be le g be host no host yes u guest no guest no e s le host yes host no t guest no guest no virtio 1.0 devices: host be le g be host yes host no u guest yes guest yes e s le host yes host no t guest no guest no This means byteswaps in qemu always depend on guest-endianness for legacy and on host-endianness for 1.0. If we want to support transitional devices with a mixture of legacy/1.0, we'll need both a per-machine and per-device swap flag: virtio_whatever(device, parameters...) { if (device->legacy) { if (guest_needs_byteswap) { whatever_byteswap(parameters...); } else { whatever(parameters...); } } else { /* 1.0 */ whatever_le(parameters...); } } Comments?
> Am 18.02.2014 um 16:45 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>: > > On Tue, 18 Feb 2014 16:12:08 +0100 > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > >> On Tue, 18 Feb 2014 17:03:27 +0200 >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >>>> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: >>>>> On 02/18/2014 01:38 PM, Greg Kurz wrote: >>>>> From: Rusty Russell <rusty@rustcorp.com.au> >>>>> >>>>> virtio data structures are defined as "target endian", which assumes >>>>> that's a fixed value. In fact, that actually means it's >>>>> platform-specific. >>>>> >>>>> The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for >>>>> little endian ppc (and potentially ARM). This is called at device >>>>> reset time (which is done before any driver is loaded) since it >>>>> may involve a system call to get the status when running under kvm. >>>>> >>>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation, >>>>> ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] >>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> >>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >>>>> --- >>>>> hw/virtio/virtio.c | 6 ++ >>>>> include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ >>>>> include/hw/virtio/virtio.h | 2 + >>>>> stubs/Makefile.objs | 1 >>>>> stubs/virtio_get_byteswap.c | 6 ++ >>>>> 5 files changed, 147 insertions(+) >>>>> create mode 100644 include/hw/virtio/virtio-access.h >>>>> create mode 100644 stubs/virtio_get_byteswap.c >>>>> >>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>>> index aeabf3a..4fd6ac2 100644 >>>>> --- a/hw/virtio/virtio.c >>>>> +++ b/hw/virtio/virtio.c >>>>> @@ -19,6 +19,9 @@ >>>>> #include "hw/virtio/virtio.h" >>>>> #include "qemu/atomic.h" >>>>> #include "hw/virtio/virtio-bus.h" >>>>> +#include "hw/virtio/virtio-access.h" >>>>> + >>>>> +bool virtio_byteswap; >>>> >>>> Could this be a virtio object property rather than a global? Imagine >>>> an AMP guest system with a BE and an LE system running in parallel >>>> accessing two separate virtio devices. With a single global that >>>> would break. >>>> >>>> >>>> Alex >>> >>> Well, how does a device know which CPU uses it? >>> I suspect we are better off waiting for 1.0 with this one. >> >> 1.0 makes this a bit more complex, no? >> >> virtio-endian accessors are defined by the endianness of host and guest >> (doing a bswap depends on the host/guest combination). This needs to be >> per qemu instance. (ioctl under kvm? machine option?) >> >> For 1.0, we'll have everything le, so a be host will always do a bswap >> (as will a be guest). But whether a device is 1.0 or legacy is not >> something that can be decided globally, or we can't have transitional >> devices with qemu. > > So here are two stupid tables on who needs to do byteswaps, one for > legacy devices, one for 1.0 devices: > > legacy devices: > > host > be le > > g be host no host yes > u guest no guest no > e > s le host yes host no > t guest no guest no > > > > virtio 1.0 devices: > > host > be le > > g be host yes host no > u guest yes guest yes > e > s le host yes host no > t guest no guest no > > > This means byteswaps in qemu always depend on guest-endianness for > legacy and on host-endianness for 1.0. If we want to support > transitional devices with a mixture of legacy/1.0, we'll need both a > per-machine and per-device swap flag: > > virtio_whatever(device, parameters...) > { > if (device->legacy) { > if (guest_needs_byteswap) { > whatever_byteswap(parameters...); > } else { > whatever(parameters...); > } > } else { /* 1.0 */ > whatever_le(parameters...); > } > } > > Comments? Yes. My point was that we could move all of the ifery above into the device reset function and from then on only use the swap bool property. Alex >
On Tue, 18 Feb 2014 17:02:18 +0100 Alexander Graf <agraf@suse.de> wrote: > > > > Am 18.02.2014 um 16:45 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>: > > > > On Tue, 18 Feb 2014 16:12:08 +0100 > > Cornelia Huck <cornelia.huck@de.ibm.com> wrote: > > > >> On Tue, 18 Feb 2014 17:03:27 +0200 > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >>>> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: > >>>>> On 02/18/2014 01:38 PM, Greg Kurz wrote: > >>>>> From: Rusty Russell <rusty@rustcorp.com.au> > >>>>> > >>>>> virtio data structures are defined as "target endian", which assumes > >>>>> that's a fixed value. In fact, that actually means it's > >>>>> platform-specific. > >>>>> > >>>>> The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for > >>>>> little endian ppc (and potentially ARM). This is called at device > >>>>> reset time (which is done before any driver is loaded) since it > >>>>> may involve a system call to get the status when running under kvm. > >>>>> > >>>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation, > >>>>> ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] > >>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> > >>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> > >>>>> --- > >>>>> hw/virtio/virtio.c | 6 ++ > >>>>> include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ > >>>>> include/hw/virtio/virtio.h | 2 + > >>>>> stubs/Makefile.objs | 1 > >>>>> stubs/virtio_get_byteswap.c | 6 ++ > >>>>> 5 files changed, 147 insertions(+) > >>>>> create mode 100644 include/hw/virtio/virtio-access.h > >>>>> create mode 100644 stubs/virtio_get_byteswap.c > >>>>> > >>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >>>>> index aeabf3a..4fd6ac2 100644 > >>>>> --- a/hw/virtio/virtio.c > >>>>> +++ b/hw/virtio/virtio.c > >>>>> @@ -19,6 +19,9 @@ > >>>>> #include "hw/virtio/virtio.h" > >>>>> #include "qemu/atomic.h" > >>>>> #include "hw/virtio/virtio-bus.h" > >>>>> +#include "hw/virtio/virtio-access.h" > >>>>> + > >>>>> +bool virtio_byteswap; > >>>> > >>>> Could this be a virtio object property rather than a global? Imagine > >>>> an AMP guest system with a BE and an LE system running in parallel > >>>> accessing two separate virtio devices. With a single global that > >>>> would break. > >>>> > >>>> > >>>> Alex > >>> > >>> Well, how does a device know which CPU uses it? > >>> I suspect we are better off waiting for 1.0 with this one. > >> > >> 1.0 makes this a bit more complex, no? > >> > >> virtio-endian accessors are defined by the endianness of host and guest > >> (doing a bswap depends on the host/guest combination). This needs to be > >> per qemu instance. (ioctl under kvm? machine option?) > >> > >> For 1.0, we'll have everything le, so a be host will always do a bswap > >> (as will a be guest). But whether a device is 1.0 or legacy is not > >> something that can be decided globally, or we can't have transitional > >> devices with qemu. > > > > So here are two stupid tables on who needs to do byteswaps, one for > > legacy devices, one for 1.0 devices: > > > > legacy devices: > > > > host > > be le > > > > g be host no host yes > > u guest no guest no > > e > > s le host yes host no > > t guest no guest no > > > > > > > > virtio 1.0 devices: > > > > host > > be le > > > > g be host yes host no > > u guest yes guest yes > > e > > s le host yes host no > > t guest no guest no > > > > > > This means byteswaps in qemu always depend on guest-endianness for > > legacy and on host-endianness for 1.0. If we want to support > > transitional devices with a mixture of legacy/1.0, we'll need both a > > per-machine and per-device swap flag: > > > > virtio_whatever(device, parameters...) > > { > > if (device->legacy) { > > if (guest_needs_byteswap) { > > whatever_byteswap(parameters...); > > } else { > > whatever(parameters...); > > } > > } else { /* 1.0 */ > > whatever_le(parameters...); > > } > > } > > > > Comments? > > Yes. My point was that we could move all of the ifery above into the device reset function and from then on only use the swap bool property. > > Alex > Hm. So whatever_le for 1.0 devices, and virtio_whatever (checking the byteswap value) for legacy devices? The device implementation will be aware of the virtio version anyway. (Btw., can some of those architectures supporting both le/be run with mixed le/be cpus? That would be a mess for legacy devices.)
On 02/18/2014 05:17 PM, Cornelia Huck wrote: > On Tue, 18 Feb 2014 17:02:18 +0100 > Alexander Graf <agraf@suse.de> wrote: > >> >>> Am 18.02.2014 um 16:45 schrieb Cornelia Huck <cornelia.huck@de.ibm.com>: >>> >>> On Tue, 18 Feb 2014 16:12:08 +0100 >>> Cornelia Huck <cornelia.huck@de.ibm.com> wrote: >>> >>>> On Tue, 18 Feb 2014 17:03:27 +0200 >>>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>>> >>>>>> On Tue, Feb 18, 2014 at 03:48:38PM +0100, Alexander Graf wrote: >>>>>>> On 02/18/2014 01:38 PM, Greg Kurz wrote: >>>>>>> From: Rusty Russell <rusty@rustcorp.com.au> >>>>>>> >>>>>>> virtio data structures are defined as "target endian", which assumes >>>>>>> that's a fixed value. In fact, that actually means it's >>>>>>> platform-specific. >>>>>>> >>>>>>> The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for >>>>>>> little endian ppc (and potentially ARM). This is called at device >>>>>>> reset time (which is done before any driver is loaded) since it >>>>>>> may involve a system call to get the status when running under kvm. >>>>>>> >>>>>>> [ fixed checkpatch.pl error with the virtio_byteswap initialisation, >>>>>>> ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] >>>>>>> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> >>>>>>> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >>>>>>> --- >>>>>>> hw/virtio/virtio.c | 6 ++ >>>>>>> include/hw/virtio/virtio-access.h | 132 +++++++++++++++++++++++++++++++++++++ >>>>>>> include/hw/virtio/virtio.h | 2 + >>>>>>> stubs/Makefile.objs | 1 >>>>>>> stubs/virtio_get_byteswap.c | 6 ++ >>>>>>> 5 files changed, 147 insertions(+) >>>>>>> create mode 100644 include/hw/virtio/virtio-access.h >>>>>>> create mode 100644 stubs/virtio_get_byteswap.c >>>>>>> >>>>>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>>>>>> index aeabf3a..4fd6ac2 100644 >>>>>>> --- a/hw/virtio/virtio.c >>>>>>> +++ b/hw/virtio/virtio.c >>>>>>> @@ -19,6 +19,9 @@ >>>>>>> #include "hw/virtio/virtio.h" >>>>>>> #include "qemu/atomic.h" >>>>>>> #include "hw/virtio/virtio-bus.h" >>>>>>> +#include "hw/virtio/virtio-access.h" >>>>>>> + >>>>>>> +bool virtio_byteswap; >>>>>> Could this be a virtio object property rather than a global? Imagine >>>>>> an AMP guest system with a BE and an LE system running in parallel >>>>>> accessing two separate virtio devices. With a single global that >>>>>> would break. >>>>>> >>>>>> >>>>>> Alex >>>>> Well, how does a device know which CPU uses it? >>>>> I suspect we are better off waiting for 1.0 with this one. >>>> 1.0 makes this a bit more complex, no? >>>> >>>> virtio-endian accessors are defined by the endianness of host and guest >>>> (doing a bswap depends on the host/guest combination). This needs to be >>>> per qemu instance. (ioctl under kvm? machine option?) >>>> >>>> For 1.0, we'll have everything le, so a be host will always do a bswap >>>> (as will a be guest). But whether a device is 1.0 or legacy is not >>>> something that can be decided globally, or we can't have transitional >>>> devices with qemu. >>> So here are two stupid tables on who needs to do byteswaps, one for >>> legacy devices, one for 1.0 devices: >>> >>> legacy devices: >>> >>> host >>> be le >>> >>> g be host no host yes >>> u guest no guest no >>> e >>> s le host yes host no >>> t guest no guest no >>> >>> >>> >>> virtio 1.0 devices: >>> >>> host >>> be le >>> >>> g be host yes host no >>> u guest yes guest yes >>> e >>> s le host yes host no >>> t guest no guest no >>> >>> >>> This means byteswaps in qemu always depend on guest-endianness for >>> legacy and on host-endianness for 1.0. If we want to support >>> transitional devices with a mixture of legacy/1.0, we'll need both a >>> per-machine and per-device swap flag: >>> >>> virtio_whatever(device, parameters...) >>> { >>> if (device->legacy) { >>> if (guest_needs_byteswap) { >>> whatever_byteswap(parameters...); >>> } else { >>> whatever(parameters...); >>> } >>> } else { /* 1.0 */ >>> whatever_le(parameters...); >>> } >>> } >>> >>> Comments? >> Yes. My point was that we could move all of the ifery above into the device reset function and from then on only use the swap bool property. >> >> Alex >> > Hm. So whatever_le for 1.0 devices, and virtio_whatever (checking the > byteswap value) for legacy devices? The device implementation will be > aware of the virtio version anyway. Yeah, but I would hope we want to share as much code as possible here, so that config accessors can be shared between legacy virtio and 1.0 virtio. And in that case we want to have a generic helper to read/write pieces of that config space - which this patch introduces for us :). > (Btw., can some of those architectures supporting both le/be run with > mixed le/be cpus? That would be a mess for legacy devices.) Yes, they can. No, we don't care :). Alex
Am 18.02.2014 13:38, schrieb Greg Kurz: > diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h > new file mode 100644 > index 0000000..2e22a47 > --- /dev/null > +++ b/include/hw/virtio/virtio-access.h > @@ -0,0 +1,132 @@ > +/* > + * Virtio Accessor Support: In case your target can change endian. > + * > + * Copyright IBM, Corp. 2013 > + * > + * Authors: > + * Rusty Russell <rusty@au.ibm.com> > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. [snip] I notice that this has been GPL-2.0 from Rusty's first series on. Is there a reason not to make the new file GPL-2.0+? Cf. http://wiki.qemu.org/Relicensing Thanks, Andreas
Am 18.02.2014 15:48, schrieb Alexander Graf: > On 02/18/2014 01:38 PM, Greg Kurz wrote: >> From: Rusty Russell <rusty@rustcorp.com.au> >> >> virtio data structures are defined as "target endian", which assumes >> that's a fixed value. In fact, that actually means it's >> platform-specific. >> >> The OASIS virtio 1.0 spec will fix this. Meanwhile, create a hook for >> little endian ppc (and potentially ARM). This is called at device >> reset time (which is done before any driver is loaded) since it >> may involve a system call to get the status when running under kvm. >> >> [ fixed checkpatch.pl error with the virtio_byteswap initialisation, >> ldq_phys() API change, Greg Kurz <gkurz@linux.vnet.ibm.com> ] >> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> >> Signed-off-by: Greg Kurz <gkurz@linux.vnet.ibm.com> >> --- >> hw/virtio/virtio.c | 6 ++ >> include/hw/virtio/virtio-access.h | 132 >> +++++++++++++++++++++++++++++++++++++ >> include/hw/virtio/virtio.h | 2 + >> stubs/Makefile.objs | 1 >> stubs/virtio_get_byteswap.c | 6 ++ >> 5 files changed, 147 insertions(+) >> create mode 100644 include/hw/virtio/virtio-access.h >> create mode 100644 stubs/virtio_get_byteswap.c >> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index aeabf3a..4fd6ac2 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -19,6 +19,9 @@ >> #include "hw/virtio/virtio.h" >> #include "qemu/atomic.h" >> #include "hw/virtio/virtio-bus.h" >> +#include "hw/virtio/virtio-access.h" >> + >> +bool virtio_byteswap; > > Could this be a virtio object property rather than a global? Imagine an > AMP guest system with a BE and an LE system running in parallel > accessing two separate virtio devices. With a single global that would > break. Déjà vu... ;) http://lists.nongnu.org/archive/html/qemu-devel/2013-08/msg01043.html Andreas
On Tue, 18 Feb 2014 20:25:15 +0100 Andreas Färber <afaerber@suse.de> wrote: > Am 18.02.2014 13:38, schrieb Greg Kurz: > > diff --git a/include/hw/virtio/virtio-access.h > > b/include/hw/virtio/virtio-access.h new file mode 100644 > > index 0000000..2e22a47 > > --- /dev/null > > +++ b/include/hw/virtio/virtio-access.h > > @@ -0,0 +1,132 @@ > > +/* > > + * Virtio Accessor Support: In case your target can change endian. > > + * > > + * Copyright IBM, Corp. 2013 > > + * > > + * Authors: > > + * Rusty Russell <rusty@au.ibm.com> > > + * > > + * This work is licensed under the terms of the GNU GPL, version 2. > > See > > + * the COPYING file in the top-level directory. > [snip] > > I notice that this has been GPL-2.0 from Rusty's first series on. Is > there a reason not to make the new file GPL-2.0+? > > Cf. http://wiki.qemu.org/Relicensing > > Thanks, > Andreas > Rusty ? It is your call. :)
Greg Kurz <gkurz@linux.vnet.ibm.com> writes: > On Tue, 18 Feb 2014 20:25:15 +0100 > Andreas Färber <afaerber@suse.de> wrote: >> Am 18.02.2014 13:38, schrieb Greg Kurz: >> > diff --git a/include/hw/virtio/virtio-access.h >> > b/include/hw/virtio/virtio-access.h new file mode 100644 >> > index 0000000..2e22a47 >> > --- /dev/null >> > +++ b/include/hw/virtio/virtio-access.h >> > @@ -0,0 +1,132 @@ >> > +/* >> > + * Virtio Accessor Support: In case your target can change endian. >> > + * >> > + * Copyright IBM, Corp. 2013 >> > + * >> > + * Authors: >> > + * Rusty Russell <rusty@au.ibm.com> >> > + * >> > + * This work is licensed under the terms of the GNU GPL, version 2. >> > See >> > + * the COPYING file in the top-level directory. >> [snip] >> >> I notice that this has been GPL-2.0 from Rusty's first series on. Is >> there a reason not to make the new file GPL-2.0+? >> >> Cf. http://wiki.qemu.org/Relicensing >> >> Thanks, >> Andreas >> > > Rusty ? It is your call. :) I cut & paste that header. I always prefer 2+ to 2 anyway. Please fix: * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation, either version 2 of the License, or * (at your option) any later version. Thanks, Rusty.
Alexander Graf <agraf@suse.de> writes: > On 02/18/2014 05:17 PM, Cornelia Huck wrote: >> Hm. So whatever_le for 1.0 devices, and virtio_whatever (checking the >> byteswap value) for legacy devices? The device implementation will be >> aware of the virtio version anyway. > > Yeah, but I would hope we want to share as much code as possible here, > so that config accessors can be shared between legacy virtio and 1.0 > virtio. And in that case we want to have a generic helper to read/write > pieces of that config space - which this patch introduces for us :). > >> (Btw., can some of those architectures supporting both le/be run with >> mixed le/be cpus? That would be a mess for legacy devices.) > > Yes, they can. No, we don't care :). There's per-cpu (what endian) and per-device (legacy or no). To do this Right, we'd need to consult both, but we don't always have that information. So this the minimal viable solution. We'll need a per-device legacy flag eventually (there are other changes than just endian). But getting the wrappers in place is a good first step. Cheers, Rusty.
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index aeabf3a..4fd6ac2 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -19,6 +19,9 @@ #include "hw/virtio/virtio.h" #include "qemu/atomic.h" #include "hw/virtio/virtio-bus.h" +#include "hw/virtio/virtio-access.h" + +bool virtio_byteswap; /* * The alignment to use between consumer and producer parts of vring. @@ -546,6 +549,9 @@ void virtio_reset(void *opaque) virtio_set_status(vdev, 0); + /* We assume all devices are the same endian. */ + virtio_byteswap = virtio_get_byteswap(); + if (k->reset) { k->reset(vdev); } diff --git a/include/hw/virtio/virtio-access.h b/include/hw/virtio/virtio-access.h new file mode 100644 index 0000000..2e22a47 --- /dev/null +++ b/include/hw/virtio/virtio-access.h @@ -0,0 +1,132 @@ +/* + * Virtio Accessor Support: In case your target can change endian. + * + * Copyright IBM, Corp. 2013 + * + * Authors: + * Rusty Russell <rusty@au.ibm.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + */ +#ifndef _QEMU_VIRTIO_ACCESS_H +#define _QEMU_VIRTIO_ACCESS_H +#include "hw/virtio/virtio.h" + +/* Initialized by virtio_get_byteswap() at any virtio device reset. */ +extern bool virtio_byteswap; + +static inline uint16_t virtio_lduw_phys(AddressSpace *as, hwaddr pa) +{ + if (virtio_byteswap) { + return bswap16(lduw_phys(as, pa)); + } + return lduw_phys(as, pa); +} + +static inline uint32_t virtio_ldl_phys(AddressSpace *as, hwaddr pa) +{ + if (virtio_byteswap) { + return bswap32(ldl_phys(as, pa)); + } + return ldl_phys(as, pa); +} + +static inline uint64_t virtio_ldq_phys(AddressSpace *as, hwaddr pa) +{ + if (virtio_byteswap) { + return bswap64(ldq_phys(as, pa)); + } + return ldq_phys(as, pa); +} + +static inline void virtio_stw_phys(AddressSpace *as, hwaddr pa, uint16_t value) +{ + if (virtio_byteswap) { + stw_phys(as, pa, bswap16(value)); + } else { + stw_phys(as, pa, value); + } +} + +static inline void virtio_stl_phys(AddressSpace *as, hwaddr pa, uint32_t value) +{ + if (virtio_byteswap) { + stl_phys(as, pa, bswap32(value)); + } else { + stl_phys(as, pa, value); + } +} + +static inline void virtio_stw_p(void *ptr, uint16_t v) +{ + if (virtio_byteswap) { + stw_p(ptr, bswap16(v)); + } else { + stw_p(ptr, v); + } +} + +static inline void virtio_stl_p(void *ptr, uint32_t v) +{ + if (virtio_byteswap) { + stl_p(ptr, bswap32(v)); + } else { + stl_p(ptr, v); + } +} + +static inline void virtio_stq_p(void *ptr, uint64_t v) +{ + if (virtio_byteswap) { + stq_p(ptr, bswap64(v)); + } else { + stq_p(ptr, v); + } +} + +static inline int virtio_lduw_p(const void *ptr) +{ + if (virtio_byteswap) { + return bswap16(lduw_p(ptr)); + } else { + return lduw_p(ptr); + } +} + +static inline int virtio_ldl_p(const void *ptr) +{ + if (virtio_byteswap) { + return bswap32(ldl_p(ptr)); + } else { + return ldl_p(ptr); + } +} + +static inline uint64_t virtio_ldq_p(const void *ptr) +{ + if (virtio_byteswap) { + return bswap64(ldq_p(ptr)); + } else { + return ldq_p(ptr); + } +} + +static inline uint32_t virtio_tswap32(uint32_t s) +{ + if (virtio_byteswap) { + return bswap32(tswap32(s)); + } else { + return tswap32(s); + } +} + +static inline void virtio_tswap32s(uint32_t *s) +{ + tswap32s(s); + if (virtio_byteswap) { + *s = bswap32(*s); + } +} +#endif /* _QEMU_VIRTIO_ACCESS_H */ diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h index 3e54e90..5009945 100644 --- a/include/hw/virtio/virtio.h +++ b/include/hw/virtio/virtio.h @@ -253,4 +253,6 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign, bool set_handler); void virtio_queue_notify_vq(VirtQueue *vq); void virtio_irq(VirtQueue *vq); + +extern bool virtio_get_byteswap(void); #endif diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs index df92fe5..7e7a9c8 100644 --- a/stubs/Makefile.objs +++ b/stubs/Makefile.objs @@ -27,3 +27,4 @@ stub-obj-y += vm-stop.o stub-obj-y += vmstate.o stub-obj-$(CONFIG_WIN32) += fd-register.o stub-obj-y += cpus.o +stub-obj-y += virtio_get_byteswap.o diff --git a/stubs/virtio_get_byteswap.c b/stubs/virtio_get_byteswap.c new file mode 100644 index 0000000..7cf764d --- /dev/null +++ b/stubs/virtio_get_byteswap.c @@ -0,0 +1,6 @@ +#include "hw/virtio/virtio.h" + +bool virtio_get_byteswap(void) +{ + return false; +}