Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) -
vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
Paolo,
The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Thanks.
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) -
vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
Paolo,
The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
Store it earlier then, using plain put/get.
You can still add a section conditionally to cause
a cleaner failure in broken cross-version scenarios.
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Thanks.
One starts doubting whether all this hackery is worth it. virtio 1.0
should be out real soon now, it makes everything LE so the problem goes
away. It's not like PPC LE is so popular that we must support old drivers
at all costs. Won't time be better spent backporting virtio 1.0 drivers?
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, 12 Jun 2014 10:54:48 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) -
vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
Paolo,
The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
Store it earlier then, using plain put/get.
Not sure I follow... this will break compatibility, no ?
You can still add a section conditionally to cause
a cleaner failure in broken cross-version scenarios.
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Thanks.
One starts doubting whether all this hackery is worth it. virtio 1.0
should be out real soon now, it makes everything LE so the problem goes
away. It's not like PPC LE is so popular that we must support old drivers
at all costs. Won't time be better spent backporting virtio 1.0 drivers?
Hmmm... AFAIC some QEMU maintainers expressed interest in supporting legacy
virtio in the case we have endian-changing targets. Patches to run a ppc64le
guests have
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On 12.06.14 09:54, Michael S. Tsirkin wrote:
On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) -
vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
Paolo,
The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
Store it earlier then, using plain put/get.
You can still add a section conditionally to cause
a cleaner failure in broken cross-version scenarios.
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Thanks.
One starts doubting whether all this hackery is worth it. virtio 1.0
should be out real soon now, it makes everything LE so the problem goes
away. It's not like PPC LE is so popular that we must support old drivers
at all costs. Won't time be better spent backporting virtio 1.0 drivers?
There are already released and working Linux distributions (Ubuntu,
openSUSE, maybe others) that don't have virtio 1.0 drivers. Putting our
heads into the sand is not an option ;).
Alex
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Il 12/06/2014 09:43, Greg Kurz ha scritto:
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
Yes, I guess it is acceptable. So first you should fix the code to
always serialize fields as big-endian, and then apply this little hack
and virtio_tswap on top of the previous change.
Paolo
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On 12.06.14 10:55, Paolo Bonzini wrote:
Il 12/06/2014 09:43, Greg Kurz ha scritto:
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
Yes, I guess it is acceptable. So first you should fix the code to
always serialize fields as big-endian, and then apply this little hack
and virtio_tswap on top of the previous change.
Why don't we just ignore those config fields? They won't change during
the lifetime of the guest anyway - and configuration needs to be the
same on source and dest to work.
Alex
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, Jun 12, 2014 at 10:47:17AM +0200, Greg Kurz wrote:
On Thu, 12 Jun 2014 10:54:48 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) -
vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with
descriptor numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int
version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness.
:-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
Paolo,
The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
Store it earlier then, using plain put/get.
Not sure I follow... this will break compatibility, no ?
Do it only on the ambialent targets :)
You can still add a section conditionally to cause
a cleaner failure in broken cross-version scenarios.
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions
and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Thanks.
One starts doubting whether all this hackery is worth it. virtio 1.0
should be out real soon now, it makes everything LE so the problem goes
away. It's not like PPC LE is so popular that we must support old
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On 12.06.14 10:47, Greg Kurz wrote:
On Thu, 12 Jun 2014 10:54:48 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) -
vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
Paolo,
The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
Store it earlier then, using plain put/get.
Not sure I follow... this will break compatibility, no ?
You can still add a section conditionally to cause
a cleaner failure in broken cross-version scenarios.
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Thanks.
One starts doubting whether all this hackery is worth it. virtio 1.0
should be out real soon now, it makes everything LE so the problem goes
away. It's not like PPC LE is so popular that we must support old drivers
at all costs. Won't time be better spent backporting virtio 1.0 drivers?
Hmmm... AFAIC some QEMU maintainers expressed interest in supporting legacy
virtio in the case we have endian-changing targets. Patches to run a ppc64le
guests have been accepted in KVM, Linux and QEMU... the only missing block
is virtio. I don't especially care in supporting old drivers at all cost: this
request was expressed on the list. I just want people to be able to run a
ppc64le
ubuntu-14.04 (and soon other distros) guest on a ppc64 box and be able to
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, 12 Jun 2014 10:55:42 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 12/06/2014 09:43, Greg Kurz ha scritto:
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
Yes, I guess it is acceptable. So first you should fix the code to
always serialize fields as big-endian, and then apply this little hack
and virtio_tswap on top of the previous change.
Paolo
BTW, can someone explain why we stream the device config ?
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, Jun 12, 2014 at 10:55:40AM +0200, Alexander Graf wrote:
On 12.06.14 09:54, Michael S. Tsirkin wrote:
On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) -
vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
Paolo,
The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
Store it earlier then, using plain put/get.
You can still add a section conditionally to cause
a cleaner failure in broken cross-version scenarios.
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Thanks.
One starts doubting whether all this hackery is worth it. virtio 1.0
should be out real soon now, it makes everything LE so the problem goes
away. It's not like PPC LE is so popular that we must support old drivers
at all costs. Won't time be better spent backporting virtio 1.0 drivers?
There are already released and working Linux distributions (Ubuntu,
openSUSE, maybe others) that don't have virtio 1.0 drivers. Putting our
heads into the sand is not an option ;).
Alex
I don't get it. Does virtio work there at the moment?
--
MST
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On 12.06.14 11:07, Michael S. Tsirkin wrote:
On Thu, Jun 12, 2014 at 10:55:40AM +0200, Alexander Graf wrote:
On 12.06.14 09:54, Michael S. Tsirkin wrote:
On Thu, Jun 12, 2014 at 09:43:51AM +0200, Greg Kurz wrote:
On Thu, 29 May 2014 12:16:26 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) -
vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
Paolo,
The patch set to support endian changing targets adds a device_endian
field to the VirtIODevice structure to be used instead of the default
target endianness as it happens with tswap() macros. It also introduces
virtio_tswap() helpers for this purpose, but they can only be used when
the device_endian field has been restored... in a subsection after the
device descriptor... :-\
Store it earlier then, using plain put/get.
You can still add a section conditionally to cause
a cleaner failure in broken cross-version scenarios.
If the scenario is ppc64le-on-ppc64: tswap() macros don't do anything
and we cannot convert back to LE...
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
Since cols and rows are not involved in the protocol, we can safely
defer the conversion to post load.
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Thanks.
One starts doubting whether all this hackery is worth it. virtio 1.0
should be out real soon now, it makes everything LE so the problem goes
away. It's not like PPC LE is so popular that we must support old drivers
at all costs. Won't time be better spent backporting virtio 1.0 drivers?
There are already released and working Linux distributions (Ubuntu,
openSUSE, maybe others) that don't have virtio 1.0 drivers. Putting our
heads into the sand is not an option ;).
Alex
I don't get it. Does virtio work there at the moment?
With the LE enable patch, yes, sure. Imagine the same would happen for
Windows and e1000 - would you still argue the same way?
Alex
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Il 12/06/2014 11:06, Greg Kurz ha scritto:
On Thu, 12 Jun 2014 10:55:42 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 12/06/2014 09:43, Greg Kurz ha scritto:
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
Yes, I guess it is acceptable. So first you should fix the code to
always serialize fields as big-endian, and then apply this little hack
and virtio_tswap on top of the previous change.
BTW, can someone explain why we stream the device config ?
For max_nr_ports it is useless indeed. Let's keep storing it for
backwards compatibility, but you can remove its load.
The cols and rows values should be defined by the host and thus could
even change on migration, there is no need to store them. As of now,
however, they are unused in QEMU and should always be zero, because
VIRTIO_CONSOLE_F_SIZE is not supported.
Paolo
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, Jun 12, 2014 at 11:19:47AM +0200, Paolo Bonzini wrote:
Il 12/06/2014 11:06, Greg Kurz ha scritto:
On Thu, 12 Jun 2014 10:55:42 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 12/06/2014 09:43, Greg Kurz ha scritto:
Since we know that 0 max_nr_ports 32, is it acceptable to guess
the correct endianness with a heuristic ?
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
max_nr_ports = bswap32(max_nr_ports);
}
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
return -EINVAL;
}
Yes, I guess it is acceptable. So first you should fix the code to
always serialize fields as big-endian, and then apply this little hack
and virtio_tswap on top of the previous change.
BTW, can someone explain why we stream the device config ?
For max_nr_ports it is useless indeed. Let's keep storing it for backwards
compatibility, but you can remove its load.
The cols and rows values should be defined by the host and thus could even
change on migration, there is no need to store them. As of now, however,
they are unused in QEMU and should always be zero, because
VIRTIO_CONSOLE_F_SIZE is not supported.
Paolo
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
--
MST
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto: Maybe just drop unnecessary stuff for new machine types? Then we won't need hacks to migrate it. For any machine type it's trivial to migrate it. All machine types including old ones can disregard the migrated values. Paolo
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With
this we should make virtio-serial config space completely independent of
live migration.
Also since QEMU versions that do read these swapped values during
migration are not bi-endian aware, we can never get into a case where a
cross-endian save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration. */
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, 12 Jun 2014 11:43:20 +0200
Alexander Graf ag...@suse.de wrote:
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With
this we should make virtio-serial config space completely independent of
live migration.
Also since QEMU versions that do read these swapped values during
migration are not bi-endian aware, we can never get into a case where a
cross-endian save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration. */
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
For the moment, we have 0 max_nr_ports 32 so the source
machine only stores a single 32 bit value... If this limit
gets raised, we can end up sending more than that... and
only the source machine max_nr_ports value can give the
information...
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Am 12.06.2014 um 12:14 schrieb Greg Kurz gk...@linux.vnet.ibm.com:
On Thu, 12 Jun 2014 11:43:20 +0200
Alexander Graf ag...@suse.de wrote:
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With
this we should make virtio-serial config space completely independent of
live migration.
Also since QEMU versions that do read these swapped values during
migration are not bi-endian aware, we can never get into a case where a
cross-endian save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration. */
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
For the moment, we have 0 max_nr_ports 32 so the source
machine only stores a single 32 bit value... If this limit
gets raised, we can end up sending more than that... and
only the source machine max_nr_ports value can give the
information...
Why? This value only ever gets set in realize, so it will not change during the
lifetime of the device - which means we don't need to migrate it.
Alex
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, 12 Jun 2014 12:39:27 +0200
Alexander Graf ag...@suse.de wrote:
Am 12.06.2014 um 12:14 schrieb Greg Kurz gk...@linux.vnet.ibm.com:
On Thu, 12 Jun 2014 11:43:20 +0200
Alexander Graf ag...@suse.de wrote:
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With
this we should make virtio-serial config space completely independent of
live migration.
Also since QEMU versions that do read these swapped values during
migration are not bi-endian aware, we can never get into a case where a
cross-endian save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration. */
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
For the moment, we have 0 max_nr_ports 32 so the source
machine only stores a single 32 bit value... If this limit
gets raised, we can end up sending more than that... and
only the source machine max_nr_ports value can give the
information...
Why? This value only ever gets set in realize, so it will not change during
the lifetime of the device - which means we don't need to migrate it.
I agree with the fact that the value does not change and should not be migrated
in the first place.
I am just worried about the size of the active ports bitmap that is streamed in
the for loop... it
is only 32 bit as of today, because we are limited to 32 ports. How would this
work if the limit is
raised ? How can the destination machine know how many bits have to be read ?
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, Jun 12, 2014 at 11:39:42AM +0200, Paolo Bonzini wrote: Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto: Maybe just drop unnecessary stuff for new machine types? Then we won't need hacks to migrate it. For any machine type it's trivial to migrate it. All machine types including old ones can disregard the migrated values. Paolo Ah, good idea, so simply disregard the values in load. -- MST
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, Jun 12, 2014 at 11:43:20AM +0200, Alexander Graf wrote:
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With this
we should make virtio-serial config space completely independent of live
migration.
Also since QEMU versions that do read these swapped values during migration
are not bi-endian aware, we can never get into a case where a cross-endian
save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void *opaque,
int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration. */
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
Exactly.
Reviewed-by: Michael S. Tsirkin m...@redhat.com
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, Jun 12, 2014 at 12:14:40PM +0200, Greg Kurz wrote:
On Thu, 12 Jun 2014 11:43:20 +0200
Alexander Graf ag...@suse.de wrote:
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With
this we should make virtio-serial config space completely independent of
live migration.
Also since QEMU versions that do read these swapped values during
migration are not bi-endian aware, we can never get into a case where a
cross-endian save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration. */
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
For the moment, we have 0 max_nr_ports 32 so the source
machine only stores a single 32 bit value... If this limit
gets raised, we can end up sending more than that... and
only the source machine max_nr_ports value can give the
information...
I don't think we need to worry.
We won't be able to change max_nr_ports in compat machine types.
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Am 12.06.2014 um 12:50 schrieb Greg Kurz gk...@linux.vnet.ibm.com:
On Thu, 12 Jun 2014 12:39:27 +0200
Alexander Graf ag...@suse.de wrote:
Am 12.06.2014 um 12:14 schrieb Greg Kurz gk...@linux.vnet.ibm.com:
On Thu, 12 Jun 2014 11:43:20 +0200
Alexander Graf ag...@suse.de wrote:
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With
this we should make virtio-serial config space completely independent of
live migration.
Also since QEMU versions that do read these swapped values during
migration are not bi-endian aware, we can never get into a case where a
cross-endian save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration. */
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
For the moment, we have 0 max_nr_ports 32 so the source
machine only stores a single 32 bit value... If this limit
gets raised, we can end up sending more than that... and
only the source machine max_nr_ports value can give the
information...
Why? This value only ever gets set in realize, so it will not change during
the lifetime of the device - which means we don't need to migrate it.
I agree with the fact that the value does not change and should not be
migrated in the first place.
I am just worried about the size of the active ports bitmap that is streamed
in the for loop... it
is only 32 bit as of today, because we are limited to 32 ports. How would
this work if the limit is
raised ? How can the destination machine know how many bits have to be read ?
The destination has be be executed in compat mode to the older qemu version via
a versioned machine type. This should ensure that limits are kept.
Alex
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, Jun 12, 2014 at 12:50:56PM +0200, Greg Kurz wrote:
On Thu, 12 Jun 2014 12:39:27 +0200
Alexander Graf ag...@suse.de wrote:
Am 12.06.2014 um 12:14 schrieb Greg Kurz gk...@linux.vnet.ibm.com:
On Thu, 12 Jun 2014 11:43:20 +0200
Alexander Graf ag...@suse.de wrote:
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With
this we should make virtio-serial config space completely independent of
live migration.
Also since QEMU versions that do read these swapped values during
migration are not bi-endian aware, we can never get into a case where a
cross-endian save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration. */
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
For the moment, we have 0 max_nr_ports 32 so the source
machine only stores a single 32 bit value... If this limit
gets raised, we can end up sending more than that... and
only the source machine max_nr_ports value can give the
information...
Why? This value only ever gets set in realize, so it will not change during
the lifetime of the device - which means we don't need to migrate it.
I agree with the fact that the value does not change and should not be
migrated in the first place.
I am just worried about the size of the active ports bitmap that is streamed
in the for loop... it
is only 32 bit as of today, because we are limited to 32 ports. How would
this work if the limit is
raised ? How can the destination machine know how many bits have to be read ?
When the destination machine is started with -M 2.1, it
knows that it has to read 32 bit.
If started with -M 3.0 it reads in 42 bits :)
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Thu, 12 Jun 2014 13:59:59 +0300
Michael S. Tsirkin m...@redhat.com wrote:
On Thu, Jun 12, 2014 at 12:50:56PM +0200, Greg Kurz wrote:
On Thu, 12 Jun 2014 12:39:27 +0200
Alexander Graf ag...@suse.de wrote:
Am 12.06.2014 um 12:14 schrieb Greg Kurz gk...@linux.vnet.ibm.com:
On Thu, 12 Jun 2014 11:43:20 +0200
Alexander Graf ag...@suse.de wrote:
On 12.06.14 11:39, Paolo Bonzini wrote:
Il 12/06/2014 11:37, Michael S. Tsirkin ha scritto:
Maybe just drop unnecessary stuff for new machine types?
Then we won't need hacks to migrate it.
For any machine type it's trivial to migrate it. All machine types
including old ones can disregard the migrated values.
How about a patch like this before the actual LE awareness ones? With
this we should make virtio-serial config space completely independent
of
live migration.
Also since QEMU versions that do read these swapped values during
migration are not bi-endian aware, we can never get into a case where
a
cross-endian save needs to be considered ;).
Alex
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 2b647b6..73cb9b7 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -670,6 +670,7 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
uint32_t max_nr_ports, nr_active_ports, ports_map;
unsigned int i;
int ret;
+uint32_t tmp;
if (version_id 3) {
return -EINVAL;
@@ -685,17 +686,12 @@ static int virtio_serial_load(QEMUFile *f, void
*opaque, int version_id)
return 0;
}
-/* The config space */
-qemu_get_be16s(f, s-config.cols);
-qemu_get_be16s(f, s-config.rows);
-
-qemu_get_be32s(f, max_nr_ports);
-tswap32s(max_nr_ports);
-if (max_nr_ports tswap32(s-config.max_nr_ports)) {
-/* Source could have had more ports than us. Fail migration.
*/
-return -EINVAL;
-}
+/* Unused */
+qemu_get_be16s(f, tmp);
+qemu_get_be16s(f, tmp);
+qemu_get_be32s(f, tmp);
+max_nr_ports = tswap32(s-config.max_nr_ports);
for (i = 0; i (max_nr_ports + 31) / 32; i++) {
qemu_get_be32s(f, ports_map);
For the moment, we have 0 max_nr_ports 32 so the source
machine only stores a single 32 bit value... If this limit
gets raised, we can end up sending more than that... and
only the source machine max_nr_ports value can give the
information...
Why? This value only ever gets set in realize, so it will not change
during the lifetime of the device - which means we don't need to migrate
it.
I agree with the fact that the value does not change and should not be
migrated in the first place.
I am just worried about the size of the active ports bitmap that is
streamed in the for loop... it
is only 32 bit as of today, because we are limited to 32 ports. How would
this work if the limit is
raised ? How can the destination machine know how many bits have to be read
?
When the destination machine is started with -M 2.1, it
knows that it has to read 32 bit.
If started with -M 3.0 it reads in 42 bits :)
Okay ! I was completely missing this point... now things make sense at last ! :)
About Alex's patch, will you or Amit or Anthony apply it directly or should
I send it along with my patches for a full review ?
Thanks for your time.
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Tue, 27 May 2014 17:01:38 +0200
Paolo Bonzini pbonz...@redhat.com wrote:
Il 14/05/2014 17:42, Greg Kurz ha scritto:
+{ .name = virtio/is_big_endian,
+ .version_id = 1,
+ .save = virtio_save_device_endian,
+ .load = virtio_load_device_endian,
+},
{ .name = NULL }
I think this is okay, but you need to add support for a needed
function like you have in normal vmstate subsections. You only need the
subsection if
if (target_words_bigendian()) {
return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_BIG;
} else {
return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
}
Paolo
Hi Paolo,
As suggested by Andreas, I have reworked the patch set to use
VMState directly instead of mimicking... and of course, I end
up with a virtio_device_endian_needed() function that does just
that ! :)
I'm on leave now, I'll try to send an update next week.
BTW, I have spotted two locations where the migration code is
affected by the device endianness at load time:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) - vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function. The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
I have questions about virtio serial:
- what is exactly the point at migrating the virtio device config space ?
- what is the scenario that calls for the active ports bitmap checking
at load time ?
- we currently have 0 max_nr_ports VIRTIO_PCI_QUEUE_MAX / 2 hardcoded.
With the current code, that means the ports bitmap is always a single
32-bit fixed size value... are there plans to support virtio serial
with 0 port ? with more than 32 ports ?
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
A solution could be to simply remove all that crap and bump versions,
or at least send zeroes on save and skip them on load.
Another solution I haven't tried yet would be to stream subsections before
the device descriptor...
Any suggestions ?
Cheers.
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Il 29/05/2014 11:12, Greg Kurz ha scritto:
int virtio_load(VirtIODevice *vdev, QEMUFile *f)
{
[...]
nheads = vring_avail_idx(vdev-vq[i]) - vdev-vq[i].last_avail_idx;
^^^
/* Check it isn't doing very strange things with descriptor
numbers. */
if (nheads vdev-vq[i].vring.num) {
[...]
}
and
static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id)
{
[...]
/* The config space */
qemu_get_be16s(f, s-config.cols);
qemu_get_be16s(f, s-config.rows);
qemu_get_be32s(f, max_nr_ports);
tswap32s(max_nr_ports);
^^
if (max_nr_ports tswap32(s-config.max_nr_ports)) {
[...]
}
If we stream subsections after the device descriptor as it is done
in VMState, these two will break because the device endian is stale.
The first one can be easily dealt with: just defer the sanity check
to a post_load function.
Good, we're lucky here.
The second is a bit more tricky: the
virtio serial migrates its config space (target endian) and the
active ports bitmap. The load code assumes max_nr_ports from the
config space tells the size of the ports bitmap... that means the
virtio migration protocol is also contaminated by target endianness. :-\
Ouch.
I guess we could break migration in the case of host endianness !=
target endianness, like this:
/* These three used to be fetched in target endianness and then
* stored as big endian. It ended up as little endian if host and
* target endianness doesn't match.
*
* Starting with qemu 2.1, we always store as big endian. The
* version wasn't bumped to avoid breaking backwards compatibility.
* We check the validity of max_nr_ports, and the incorrect-
* endianness max_nr_ports will be huge, which will abort migration
* anyway.
*/
uint16_t cols = tswap16(s-config.cols);
uint16_t rows = tswap16(s-config.rows);
uint32_t max_nr_ports = tswap32(s-config.max_nr_ports);
qemu_put_be16s(f, cols);
qemu_put_be16s(f, rows);
qemu_put_be32s(f, max_nr_ports);
...
uint16_t cols, rows;
qemu_get_be16s(f, cols);
qemu_get_be16s(f, rows);
qemu_get_be32s(f, max_nr_ports);
/* Convert back to target endianness when storing into the config
* space.
*/
s-config.cols = tswap16(cols);
s-config.rows = tswap16(rows);
if (max_nr_ports tswap32(s-config.max_nr_ports) {
...
}
In the case the answer for above is legacy virtio really sucks then,
is it acceptable to not honor bug-compatibility with older versions and
fix the code ? :)
As long as the common cases don't break, yes. The question is what are
the common cases. Here I think the only non-obscure case that could
break is x86-on-PPC, and it's not that common.
Paolo
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Il 14/05/2014 17:42, Greg Kurz ha scritto:
+{ .name = virtio/is_big_endian,
+ .version_id = 1,
+ .save = virtio_save_device_endian,
+ .load = virtio_load_device_endian,
+},
{ .name = NULL }
I think this is okay, but you need to add support for a needed
function like you have in normal vmstate subsections. You only need the
subsection if
if (target_words_bigendian()) {
return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_BIG;
} else {
return vdev-device_endian != VIRTIO_DEVICE_ENDIAN_LITTLE;
}
Paolo
[Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Some CPU families can dynamically change their endianness. This means we
can have little endian ppc or big endian arm guests for example. This has
an impact on legacy virtio data structures since they are target endian.
We hence introduce a new property to track the endianness of each virtio
device. It is reasonnably assumed that endianness won't change while the
device is in use : we hence capture the device endianness when it gets
reset.
Of course this property must be part of the migration stream as most of
the virtio code will depend on it. It is hence migrated in a subsection
to preserve compatibility with older migration streams. The tricky part
is that the endianness gets known quite late and we must ensure no access
is made to virtio data before that. It is for example invalid to call
vring_avail_idx() as does the actual migration code: the VQ indexes sanity
checks had to be moved from virtio_load() to virtio_load_subsections()
because of that.
We enforce some paranoia by making the endianness a 3-value enum so
that we can temporarily poison it while loading state.
Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
---
exec.c |8 +---
hw/virtio/virtio-pci.c | 11 ++
hw/virtio/virtio.c | 80 +---
include/exec/cpu-common.h |1 +
include/hw/virtio/virtio.h | 13 +++
5 files changed, 87 insertions(+), 26 deletions(-)
diff --git a/exec.c b/exec.c
index 91513c6..4e83588 100644
--- a/exec.c
+++ b/exec.c
@@ -2740,13 +2740,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
#endif
#if !defined(CONFIG_USER_ONLY)
-
-/*
- * A helper function for the _utterly broken_ virtio device model to find out
if
- * it's running on a big endian machine. Don't do this at home kids!
- */
-bool virtio_is_big_endian(void);
-bool virtio_is_big_endian(void)
+bool target_words_bigendian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
return true;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..2ffceb8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests. */
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque,
hwaddr addr,
break;
case 2:
val = virtio_config_readw(vdev, addr);
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap16(val);
}
break;
case 4:
val = virtio_config_readl(vdev, addr);
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap32(val);
}
break;
@@ -443,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr
addr,
virtio_config_writeb(vdev, addr, val);
break;
case 2:
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap16(val);
}
virtio_config_writew(vdev, addr, val);
break;
case 4:
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap32(val);
}
virtio_config_writel(vdev, addr, val);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7fbad29..6578854 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -539,6 +539,15 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
vdev-status = val;
}
+static void virtio_set_endian_target_default(VirtIODevice *vdev)
+{
+if (target_words_bigendian()) {
+vdev-device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
+} else {
+vdev-device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
+}
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
@@ -546,6 +555,7 @@ void virtio_reset(void *opaque)
int i;
virtio_set_status(vdev, 0);
+virtio_set_endian_target_default(vdev);
if (k-reset) {
k-reset(vdev);
@@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
int version_id;
void (*save)(VirtIODevice *vdev, QEMUFile *f);
int (*load)(VirtIODevice *vdev, QEMUFile *f);
-int (*needed)(VirtIODevice *vdev);
+bool (*needed)(VirtIODevice *vdev);
} VirtIOSubsection;
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+qemu_put_byte(f, vdev-device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+vdev-device_endian = qemu_get_byte(f);
+return 0;
+}
+
+static bool virtio_device_endian_needed(VirtIODevice *vdev)
+{
+/* No migration is supposed to occur while we are loading state.
+ */
+
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Mon, 19 May 2014 10:39:09 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:
Some CPU families can dynamically change their endianness. This means we
can have little endian ppc or big endian arm guests for example. This has
an impact on legacy virtio data structures since they are target endian.
We hence introduce a new property to track the endianness of each virtio
device. It is reasonnably assumed that endianness won't change while the
device is in use : we hence capture the device endianness when it gets
reset.
Of course this property must be part of the migration stream as most of
the virtio code will depend on it. It is hence migrated in a subsection
to preserve compatibility with older migration streams. The tricky part
is that the endianness gets known quite late and we must ensure no access
is made to virtio data before that. It is for example invalid to call
vring_avail_idx() as does the actual migration code: the VQ indexes sanity
checks had to be moved from virtio_load() to virtio_load_subsections()
because of that.
We enforce some paranoia by making the endianness a 3-value enum so
that we can temporarily poison it while loading state.
Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
---
exec.c |8 +---
hw/virtio/virtio-pci.c | 11 ++
hw/virtio/virtio.c | 80
+---
include/exec/cpu-common.h |1 +
include/hw/virtio/virtio.h | 13 +++
5 files changed, 87 insertions(+), 26 deletions(-)
diff --git a/exec.c b/exec.c
index 91513c6..4e83588 100644
--- a/exec.c
+++ b/exec.c
@@ -2740,13 +2740,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong
addr,
#endif
#if !defined(CONFIG_USER_ONLY)
-
-/*
- * A helper function for the _utterly broken_ virtio device model to find
out if
- * it's running on a big endian machine. Don't do this at home kids!
- */
-bool virtio_is_big_endian(void);
-bool virtio_is_big_endian(void)
+bool target_words_bigendian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
return true;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..2ffceb8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests.
*/
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque,
hwaddr addr,
break;
case 2:
val = virtio_config_readw(vdev, addr);
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap16(val);
}
break;
case 4:
val = virtio_config_readl(vdev, addr);
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap32(val);
}
break;
@@ -443,13 +440,13 @@ static void virtio_pci_config_write(void *opaque,
hwaddr addr,
virtio_config_writeb(vdev, addr, val);
break;
case 2:
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap16(val);
}
virtio_config_writew(vdev, addr, val);
break;
case 4:
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap32(val);
}
virtio_config_writel(vdev, addr, val);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7fbad29..6578854 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -539,6 +539,15 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val)
vdev-status = val;
}
+static void virtio_set_endian_target_default(VirtIODevice *vdev)
+{
+if (target_words_bigendian()) {
+vdev-device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
+} else {
+vdev-device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
+}
+}
+
void virtio_reset(void *opaque)
{
VirtIODevice *vdev = opaque;
@@ -546,6 +555,7 @@ void virtio_reset(void *opaque)
int i;
virtio_set_status(vdev, 0);
+virtio_set_endian_target_default(vdev);
if (k-reset) {
k-reset(vdev);
@@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
int version_id;
void (*save)(VirtIODevice *vdev, QEMUFile *f);
int (*load)(VirtIODevice *vdev, QEMUFile *f);
-int (*needed)(VirtIODevice *vdev);
+bool (*needed)(VirtIODevice *vdev);
} VirtIOSubsection;
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+qemu_put_byte(f, vdev-device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Am 19.05.2014 15:06, schrieb Greg Kurz:
On Mon, 19 May 2014 10:39:09 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7fbad29..6578854 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
[...]
@@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
int version_id;
void (*save)(VirtIODevice *vdev, QEMUFile *f);
int (*load)(VirtIODevice *vdev, QEMUFile *f);
-int (*needed)(VirtIODevice *vdev);
+bool (*needed)(VirtIODevice *vdev);
} VirtIOSubsection;
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+qemu_put_byte(f, vdev-device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+vdev-device_endian = qemu_get_byte(f);
+return 0;
+}
+
+static bool virtio_device_endian_needed(VirtIODevice *vdev)
+{
+/* No migration is supposed to occur while we are loading state.
+ */
+assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+if (target_words_bigendian()) {
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
+} else {
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
+}
+
static const VirtIOSubsection virtio_subsection[] = {
+{ .name = virtio/device_endian,
Can anyone comment the subsection name ? Is there a chance the
VMState port would come up with the same ?
+ .version_id = 1,
+ .save = virtio_save_device_endian,
+ .load = virtio_load_device_endian,
+ .needed = virtio_device_endian_needed,
+},
{ .name = NULL }
};
Different question: With converting VirtIO to VMState in mind, why are
you not using a regular VMStateSubsection and loading/saving that as
part of the old-style load/save functions? Is an API for that missing?
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
On Mon, 19 May 2014 19:06:39 +0200
Andreas Färber afaer...@suse.de wrote:
Am 19.05.2014 15:06, schrieb Greg Kurz:
On Mon, 19 May 2014 10:39:09 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7fbad29..6578854 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
[...]
@@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
int version_id;
void (*save)(VirtIODevice *vdev, QEMUFile *f);
int (*load)(VirtIODevice *vdev, QEMUFile *f);
-int (*needed)(VirtIODevice *vdev);
+bool (*needed)(VirtIODevice *vdev);
} VirtIOSubsection;
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+qemu_put_byte(f, vdev-device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+vdev-device_endian = qemu_get_byte(f);
+return 0;
+}
+
+static bool virtio_device_endian_needed(VirtIODevice *vdev)
+{
+/* No migration is supposed to occur while we are loading state.
+ */
+assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+if (target_words_bigendian()) {
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
+} else {
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
+}
+
static const VirtIOSubsection virtio_subsection[] = {
+{ .name = virtio/device_endian,
Can anyone comment the subsection name ? Is there a chance the
VMState port would come up with the same ?
+ .version_id = 1,
+ .save = virtio_save_device_endian,
+ .load = virtio_load_device_endian,
+ .needed = virtio_device_endian_needed,
+},
{ .name = NULL }
};
Different question: With converting VirtIO to VMState in mind, why are
you not using a regular VMStateSubsection and loading/saving that as
part of the old-style load/save functions? Is an API for that missing?
I guess because I haven't tried yet. :)
I'll have a closer look at this.
Regards,
Andreas
Thanks.
--
Gregory Kurz kurzg...@fr.ibm.com
gk...@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys http://www.ibm.com
Tel +33 (0)562 165 496
Anarchy is about taking complete responsibility for yourself.
Alan Moore.
Re: [Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
* Andreas F?rber (afaer...@suse.de) wrote:
Am 19.05.2014 15:06, schrieb Greg Kurz:
On Mon, 19 May 2014 10:39:09 +0200
Greg Kurz gk...@linux.vnet.ibm.com wrote:
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 7fbad29..6578854 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
[...]
@@ -839,10 +849,39 @@ typedef struct VirtIOSubsection {
int version_id;
void (*save)(VirtIODevice *vdev, QEMUFile *f);
int (*load)(VirtIODevice *vdev, QEMUFile *f);
-int (*needed)(VirtIODevice *vdev);
+bool (*needed)(VirtIODevice *vdev);
} VirtIOSubsection;
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+qemu_put_byte(f, vdev-device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+vdev-device_endian = qemu_get_byte(f);
+return 0;
+}
+
+static bool virtio_device_endian_needed(VirtIODevice *vdev)
+{
+/* No migration is supposed to occur while we are loading state.
+ */
+assert(vdev-device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN);
+if (target_words_bigendian()) {
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE;
+} else {
+return vdev-device_endian == VIRTIO_DEVICE_ENDIAN_BIG;
+}
+}
+
static const VirtIOSubsection virtio_subsection[] = {
+{ .name = virtio/device_endian,
Can anyone comment the subsection name ? Is there a chance the
VMState port would come up with the same ?
+ .version_id = 1,
+ .save = virtio_save_device_endian,
+ .load = virtio_load_device_endian,
+ .needed = virtio_device_endian_needed,
+},
{ .name = NULL }
};
Different question: With converting VirtIO to VMState in mind, why are
you not using a regular VMStateSubsection and loading/saving that as
part of the old-style load/save functions? Is an API for that missing?
There are a handful of places that call into vmstate from a non-vmstate
routine but I don't think they're using plain subsections.
hw/pci/pci.c: pci_device_save/load
hw/scsi/spapr_vscsi.c: vscsi_save_request
hw/acpi/piix4.c: acpi_load_old
Dave
Regards,
Andreas
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[Qemu-devel] [PATCH RFC 8/8] virtio: add endian-ambivalent support to VirtIODevice
Some CPU families can dynamically change their endianness. This means we
can have little endian ppc or big endian arm guests for example. This has
an impact on legacy virtio data structures since they are target endian.
We hence introduce a new property to track the endianness of each virtio
device. It is reasonnably assumed that endianness won't change while the
device is in use : we hence capture the device endianness when it gets
reset.
Of course this property must be part of the migration stream as most of
the virtio code will depend on it. It is hence migrated in a subsection
to preserve compatibility with older migration streams. The tricky part
is that the endianness gets known quite late and we must ensure no access
is made to virtio data before that. It is for example invalid to call
vring_avail_idx() as does the actual migration code: the VQ indexes sanity
checks had to be moved from virtio_load() to virtio_load_subsections()
because of that.
In fact, we have two conditions where the endianness property is stale:
- from initialization time (virtio_init) to first reset time (virtio_reset)
- from incoming migration time (virtio_load) to the time the property
is restored (virtio_load_device_endian)
We enforce some paranoia by making the endianness a 3-value enum so
that we can poison it when it should not be used.
Signed-off-by: Greg Kurz gk...@linux.vnet.ibm.com
---
exec.c |8 +-
hw/virtio/virtio-pci.c | 11 +++-
hw/virtio/virtio.c | 64
include/exec/cpu-common.h |1 +
include/hw/virtio/virtio.h | 13 +
5 files changed, 72 insertions(+), 25 deletions(-)
diff --git a/exec.c b/exec.c
index 91513c6..4e83588 100644
--- a/exec.c
+++ b/exec.c
@@ -2740,13 +2740,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
#endif
#if !defined(CONFIG_USER_ONLY)
-
-/*
- * A helper function for the _utterly broken_ virtio device model to find out
if
- * it's running on a big endian machine. Don't do this at home kids!
- */
-bool virtio_is_big_endian(void);
-bool virtio_is_big_endian(void)
+bool target_words_bigendian(void)
{
#if defined(TARGET_WORDS_BIGENDIAN)
return true;
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index ce97514..2ffceb8 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -89,9 +89,6 @@
/* Flags track per-device state like workarounds for quirks in older guests. */
#define VIRTIO_PCI_FLAG_BUS_MASTER_BUG (1 0)
-/* HACK for virtio to determine if it's running a big endian guest */
-bool virtio_is_big_endian(void);
-
static void virtio_pci_bus_new(VirtioBusState *bus, size_t bus_size,
VirtIOPCIProxy *dev);
@@ -409,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque,
hwaddr addr,
break;
case 2:
val = virtio_config_readw(vdev, addr);
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap16(val);
}
break;
case 4:
val = virtio_config_readl(vdev, addr);
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap32(val);
}
break;
@@ -443,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr
addr,
virtio_config_writeb(vdev, addr, val);
break;
case 2:
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap16(val);
}
virtio_config_writew(vdev, addr, val);
break;
case 4:
-if (virtio_is_big_endian()) {
+if (virtio_is_big_endian(vdev)) {
val = bswap32(val);
}
virtio_config_writel(vdev, addr, val);
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f4eaa3f..9018b6c 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -547,6 +547,12 @@ void virtio_reset(void *opaque)
virtio_set_status(vdev, 0);
+if (target_words_bigendian()) {
+vdev-device_endian = VIRTIO_DEVICE_ENDIAN_BIG;
+} else {
+vdev-device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE;
+}
+
if (k-reset) {
k-reset(vdev);
}
@@ -834,12 +840,28 @@ void virtio_notify_config(VirtIODevice *vdev)
virtio_notify_vector(vdev, vdev-config_vector);
}
+static void virtio_save_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+qemu_put_byte(f, vdev-device_endian);
+}
+
+static int virtio_load_device_endian(VirtIODevice *vdev, QEMUFile *f)
+{
+vdev-device_endian = qemu_get_byte(f);
+return 0;
+}
+
static const struct VirtIOSubsectionDescStruct {
const char *name;
int version_id;
void (*save)(VirtIODevice *vdev, QEMUFile *f);
int (*load)(VirtIODevice *vdev, QEMUFile *f);
} virtio_subsection[] = {
+{ .name = virtio/is_big_endian,
+ .version_id = 1,
+ .save = virtio_save_device_endian,
+ .load =
