| Message ID | 1418361995-24091-1-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 12.12.14 06:26, David Gibson wrote: > On a bi-endian target, with a guest in the non-default endian mode, > attempting to migrate twice in a row with a virtio-serial device wil > cause a qemu SEGV on the second outgoing migration. > > The problem is that virtio_serial_save_device() (and other places) expect > VirtIOSerial->config to be in current guest endianness. On a fresh boot, > virtio_serial_device_realize() will initialize VirtIOSerial->config in > default endianness. It's assumed the guest OS will make its true > endianness known before the device is reset and initialized, then > vser_reset adjusts VirtIOSerial->config into the new endianness. > > But on an incoming migration, the device isn't reset (after all the guest > has a running driver as far as it's concerned), which means that > VirtIOSerial->config retains its default endianness value from > virtio_serial_device_realize(). > > On a subsequent outgoing migration, virtio_serial_save_device() attempts > to interpret VirtIOSerial->config.max_nr_ports in current endianness when > its actually in default endianness and then runs off the end of the > ports_map array in the loop immediately afterwards. > > We could fix this by adjusting VirtIOSerial->config into the correct > current endianness after an incoming migration. But a better fix is just > to get rid of VirtIOSerial->config entirely: > * The virtio-serial config space is not settable, it always contains the > values set at initialization > * AFAICT "rows" and "cols" have never actually been used for anything and > are always zero. > * "max_nr_ports" is initialized from > VirtIOSerial->serial.max_virtserial_ports (host endian) > > So instead of maintaining this pointless guest-endian cache of the config > data, we can just construct it directly into the correct current guest > endian in the get_config hook. Current users of ->config can instead use > the sources from which the config values were derived, which means they > don't have to mess about with converting from guest endian at all. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> In general I agree with the patch, but ... > --- > hw/char/virtio-serial-bus.c | 42 +++++++++++++++------------------------ > include/hw/virtio/virtio-serial.h | 2 -- > 2 files changed, 16 insertions(+), 28 deletions(-) > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c > index a7b1b68..dd5d7ec 100644 > --- a/hw/char/virtio-serial-bus.c > +++ b/hw/char/virtio-serial-bus.c > @@ -482,10 +482,14 @@ static uint32_t get_features(VirtIODevice *vdev, uint32_t features) > /* Guest requested config info */ > static void get_config(VirtIODevice *vdev, uint8_t *config_data) > { > - VirtIOSerial *vser; > - > - vser = VIRTIO_SERIAL(vdev); > - memcpy(config_data, &vser->config, sizeof(struct virtio_console_config)); > + VirtIOSerial *vser = VIRTIO_SERIAL(vdev); > + struct virtio_console_config *config = > + (struct virtio_console_config *)config_data; > + > + config->cols = 0; > + config->rows = 0; > + config->max_nr_ports = virtio_tswap32(vdev, > + vser->serial.max_virtserial_ports); > } > > static void guest_reset(VirtIOSerial *vser) > @@ -533,10 +537,6 @@ static void vser_reset(VirtIODevice *vdev) > > vser = VIRTIO_SERIAL(vdev); > guest_reset(vser); > - > - /* In case we have switched endianness */ > - vser->config.max_nr_ports = > - virtio_tswap32(vdev, vser->serial.max_virtserial_ports); > } > > static void virtio_serial_save(QEMUFile *f, void *opaque) > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f) > uint32_t nr_active_ports; > unsigned int i, max_nr_ports; > > - /* The config space */ > - qemu_put_be16s(f, &s->config.cols); > - qemu_put_be16s(f, &s->config.rows); > + max_nr_ports = s->serial.max_virtserial_ports; > > - qemu_put_be32s(f, &s->config.max_nr_ports); > + /* Used to be config space, now redundant */ > + qemu_put_be16(f, 0); > + qemu_put_be16(f, 0); ... are you 100% sure these are always unused and will stay unused? I think we'd be better off just treating them the same way as max_nr_ports and migrate them over - or at least add a comment here what the variables mean so that whenever someone adds support for rows/cols they know where to put them. Alex > + qemu_put_be32(f, virtio_tswap32(vdev, max_nr_ports)); > > /* The ports map */ > - max_nr_ports = virtio_tswap32(vdev, s->config.max_nr_ports); > for (i = 0; i < (max_nr_ports + 31) / 32; i++) { > qemu_put_be32s(f, &s->ports_map[i]); > } > @@ -715,13 +715,7 @@ static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f, > qemu_get_be16s(f, (uint16_t *) &tmp); > qemu_get_be32s(f, &tmp); > > - /* Note: this is the only location where we use tswap32() instead of > - * virtio_tswap32() because: > - * - virtio_tswap32() only makes sense when the device is fully restored > - * - the target endianness that was used to populate s->config is > - * necessarly the default one > - */ > - max_nr_ports = tswap32(s->config.max_nr_ports); > + max_nr_ports = s->serial.max_virtserial_ports; > for (i = 0; i < (max_nr_ports + 31) / 32; i++) { > qemu_get_be32s(f, &ports_map); > > @@ -784,10 +778,9 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent) > /* This function is only used if a port id is not provided by the user */ > static uint32_t find_free_port_id(VirtIOSerial *vser) > { > - VirtIODevice *vdev = VIRTIO_DEVICE(vser); > unsigned int i, max_nr_ports; > > - max_nr_ports = virtio_tswap32(vdev, vser->config.max_nr_ports); > + max_nr_ports = vser->serial.max_virtserial_ports; > for (i = 0; i < (max_nr_ports + 31) / 32; i++) { > uint32_t map, bit; > > @@ -848,7 +841,6 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) > VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev); > VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port); > VirtIOSerialBus *bus = VIRTIO_SERIAL_BUS(qdev_get_parent_bus(dev)); > - VirtIODevice *vdev = VIRTIO_DEVICE(bus->vser); > int max_nr_ports; > bool plugging_port0; > Error *err = NULL; > @@ -890,7 +882,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) > } > } > > - max_nr_ports = virtio_tswap32(vdev, port->vser->config.max_nr_ports); > + max_nr_ports = port->vser->serial.max_virtserial_ports; > if (port->id >= max_nr_ports) { > error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, " > "max. allowed: %u", max_nr_ports - 1); > @@ -995,8 +987,6 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp) > vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output); > } > > - vser->config.max_nr_ports = > - virtio_tswap32(vdev, vser->serial.max_virtserial_ports); > vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32) > * sizeof(vser->ports_map[0])); > /* > diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h > index a679e54..11af978 100644 > --- a/include/hw/virtio/virtio-serial.h > +++ b/include/hw/virtio/virtio-serial.h > @@ -207,8 +207,6 @@ struct VirtIOSerial { > /* bitmap for identifying active ports */ > uint32_t *ports_map; > > - struct virtio_console_config config; > - > struct VirtIOSerialPostLoad *post_load; > > virtio_serial_conf serial; >
On Mon, Dec 15, 2014 at 03:59:56PM +0100, Alexander Graf wrote: > > > On 12.12.14 06:26, David Gibson wrote: > > On a bi-endian target, with a guest in the non-default endian mode, > > attempting to migrate twice in a row with a virtio-serial device wil > > cause a qemu SEGV on the second outgoing migration. > > > > The problem is that virtio_serial_save_device() (and other places) expect > > VirtIOSerial->config to be in current guest endianness. On a fresh boot, > > virtio_serial_device_realize() will initialize VirtIOSerial->config in > > default endianness. It's assumed the guest OS will make its true > > endianness known before the device is reset and initialized, then > > vser_reset adjusts VirtIOSerial->config into the new endianness. > > > > But on an incoming migration, the device isn't reset (after all the guest > > has a running driver as far as it's concerned), which means that > > VirtIOSerial->config retains its default endianness value from > > virtio_serial_device_realize(). > > > > On a subsequent outgoing migration, virtio_serial_save_device() attempts > > to interpret VirtIOSerial->config.max_nr_ports in current endianness when > > its actually in default endianness and then runs off the end of the > > ports_map array in the loop immediately afterwards. > > > > We could fix this by adjusting VirtIOSerial->config into the correct > > current endianness after an incoming migration. But a better fix is just > > to get rid of VirtIOSerial->config entirely: > > * The virtio-serial config space is not settable, it always contains the > > values set at initialization > > * AFAICT "rows" and "cols" have never actually been used for anything and > > are always zero. > > * "max_nr_ports" is initialized from > > VirtIOSerial->serial.max_virtserial_ports (host endian) > > > > So instead of maintaining this pointless guest-endian cache of the config > > data, we can just construct it directly into the correct current guest > > endian in the get_config hook. Current users of ->config can instead use > > the sources from which the config values were derived, which means they > > don't have to mess about with converting from guest endian at all. > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > In general I agree with the patch, but ... [snip] > > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f) > > uint32_t nr_active_ports; > > unsigned int i, max_nr_ports; > > > > - /* The config space */ > > - qemu_put_be16s(f, &s->config.cols); > > - qemu_put_be16s(f, &s->config.rows); > > + max_nr_ports = s->serial.max_virtserial_ports; > > > > - qemu_put_be32s(f, &s->config.max_nr_ports); > > + /* Used to be config space, now redundant */ > > + qemu_put_be16(f, 0); > > + qemu_put_be16(f, 0); > > ... are you 100% sure these are always unused and will stay unused? I > think we'd be better off just treating them the same way as max_nr_ports > and migrate them over - or at least add a comment here what the > variables mean so that whenever someone adds support for rows/cols they > know where to put them. Well, so. Here's what I know: * The rows and cols fields certainly aren't used at present (and we don't advertise the VIRTIO_CONSOLE_F_SIZE feature bit which says they are valid) * I've looked through the git history and I'm confident they've never been used. * The current incoming side migration code will ignore these fields. Prior to e38e943a it would load them, but then not do anything with the cols and rows fields as before. Thinking about it, even if rows/cols are used in future, it doesn't actually make sense to migrate them. They'll change in response to host events, not guest events, and the suitable values might change on migration. So it seems like it would be more sensible for the migration destination to generate correct values for itself and advertise them to the guest with an unconditional config_changed interrupt.
On (Fri) 12 Dec 2014 [16:26:35], David Gibson wrote: > On a bi-endian target, with a guest in the non-default endian mode, > attempting to migrate twice in a row with a virtio-serial device wil > cause a qemu SEGV on the second outgoing migration. > > The problem is that virtio_serial_save_device() (and other places) expect > VirtIOSerial->config to be in current guest endianness. On a fresh boot, > virtio_serial_device_realize() will initialize VirtIOSerial->config in > default endianness. It's assumed the guest OS will make its true > endianness known before the device is reset and initialized, then > vser_reset adjusts VirtIOSerial->config into the new endianness. > > But on an incoming migration, the device isn't reset (after all the guest > has a running driver as far as it's concerned), which means that > VirtIOSerial->config retains its default endianness value from > virtio_serial_device_realize(). > > On a subsequent outgoing migration, virtio_serial_save_device() attempts > to interpret VirtIOSerial->config.max_nr_ports in current endianness when > its actually in default endianness and then runs off the end of the > ports_map array in the loop immediately afterwards. > > We could fix this by adjusting VirtIOSerial->config into the correct > current endianness after an incoming migration. But a better fix is just > to get rid of VirtIOSerial->config entirely: > * The virtio-serial config space is not settable, it always contains the > values set at initialization > * AFAICT "rows" and "cols" have never actually been used for anything and > are always zero. There were patches on the list a few years back to add resizing support. Also, ppc and s390 people were using this feature (why else would it have been implemented?) -- since you're saying they're not in use, I suppose ppc doesn't use it. CC'ing s390 people for comment. > * "max_nr_ports" is initialized from > VirtIOSerial->serial.max_virtserial_ports (host endian) > > So instead of maintaining this pointless guest-endian cache of the config > data, we can just construct it directly into the correct current guest > endian in the get_config hook. Current users of ->config can instead use > the sources from which the config values were derived, which means they > don't have to mess about with converting from guest endian at all. I'd agree with this approach when I have confirmation no one actually uses the {rows,cols}. Since qemu doesn't use the rows and cols, it doesn't matter what settings the dest host has; otherwise the dest host would have had to adjust the guest to use the dest's settings for rows and cols after a migration. Also, for "new" guests, they should use the control vq command to adjust the rows and cols -- and they're not migrated since they're not guest state. > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/char/virtio-serial-bus.c | 42 +++++++++++++++------------------------ > include/hw/virtio/virtio-serial.h | 2 -- > 2 files changed, 16 insertions(+), 28 deletions(-) > > diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c > index a7b1b68..dd5d7ec 100644 > --- a/hw/char/virtio-serial-bus.c > +++ b/hw/char/virtio-serial-bus.c > @@ -482,10 +482,14 @@ static uint32_t get_features(VirtIODevice *vdev, uint32_t features) > /* Guest requested config info */ > static void get_config(VirtIODevice *vdev, uint8_t *config_data) > { > - VirtIOSerial *vser; > - > - vser = VIRTIO_SERIAL(vdev); > - memcpy(config_data, &vser->config, sizeof(struct virtio_console_config)); > + VirtIOSerial *vser = VIRTIO_SERIAL(vdev); > + struct virtio_console_config *config = > + (struct virtio_console_config *)config_data; > + > + config->cols = 0; > + config->rows = 0; > + config->max_nr_ports = virtio_tswap32(vdev, > + vser->serial.max_virtserial_ports); > } > > static void guest_reset(VirtIOSerial *vser) > @@ -533,10 +537,6 @@ static void vser_reset(VirtIODevice *vdev) > > vser = VIRTIO_SERIAL(vdev); > guest_reset(vser); > - > - /* In case we have switched endianness */ > - vser->config.max_nr_ports = > - virtio_tswap32(vdev, vser->serial.max_virtserial_ports); > } > > static void virtio_serial_save(QEMUFile *f, void *opaque) > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f) > uint32_t nr_active_ports; > unsigned int i, max_nr_ports; > > - /* The config space */ > - qemu_put_be16s(f, &s->config.cols); > - qemu_put_be16s(f, &s->config.rows); > + max_nr_ports = s->serial.max_virtserial_ports; > > - qemu_put_be32s(f, &s->config.max_nr_ports); > + /* Used to be config space, now redundant */ > + qemu_put_be16(f, 0); > + qemu_put_be16(f, 0); > + qemu_put_be32(f, virtio_tswap32(vdev, max_nr_ports)); Can you split this patch so the config change and the max_nr_ports change are separate? The max_nr_ports could similarly be ignored by dest, right? Thanks, Amit
On Tue, Dec 16, 2014 at 09:43:30AM +0530, Amit Shah wrote: > On (Fri) 12 Dec 2014 [16:26:35], David Gibson wrote: > > On a bi-endian target, with a guest in the non-default endian mode, > > attempting to migrate twice in a row with a virtio-serial device wil > > cause a qemu SEGV on the second outgoing migration. > > > > The problem is that virtio_serial_save_device() (and other places) expect > > VirtIOSerial->config to be in current guest endianness. On a fresh boot, > > virtio_serial_device_realize() will initialize VirtIOSerial->config in > > default endianness. It's assumed the guest OS will make its true > > endianness known before the device is reset and initialized, then > > vser_reset adjusts VirtIOSerial->config into the new endianness. > > > > But on an incoming migration, the device isn't reset (after all the guest > > has a running driver as far as it's concerned), which means that > > VirtIOSerial->config retains its default endianness value from > > virtio_serial_device_realize(). > > > > On a subsequent outgoing migration, virtio_serial_save_device() attempts > > to interpret VirtIOSerial->config.max_nr_ports in current endianness when > > its actually in default endianness and then runs off the end of the > > ports_map array in the loop immediately afterwards. > > > > We could fix this by adjusting VirtIOSerial->config into the correct > > current endianness after an incoming migration. But a better fix is just > > to get rid of VirtIOSerial->config entirely: > > * The virtio-serial config space is not settable, it always contains the > > values set at initialization > > * AFAICT "rows" and "cols" have never actually been used for anything and > > are always zero. > > There were patches on the list a few years back to add resizing > support. Well, apparently they were never merged, because I can't see a thing in the git history. > Also, ppc and s390 people were using this feature (why else would it > have been implemented?) -- since you're saying they're not in use, I > suppose ppc doesn't use it. CC'ing s390 people for comment. > > > * "max_nr_ports" is initialized from > > VirtIOSerial->serial.max_virtserial_ports (host endian) > > > > So instead of maintaining this pointless guest-endian cache of the config > > data, we can just construct it directly into the correct current guest > > endian in the get_config hook. Current users of ->config can instead use > > the sources from which the config values were derived, which means they > > don't have to mess about with converting from guest endian at all. > > I'd agree with this approach when I have confirmation no one actually > uses the {rows,cols}. I've grepped the tree and searched through git history. I'm pretty sure they're not used. And even if we do want to use them, this approach would still be sound - it would make more sense to hold the rows and cols info in host endian (which is well defined and static, at least) and just byteswap it on get_config() where necessary. > Since qemu doesn't use the rows and cols, it doesn't matter what > settings the dest host has; otherwise the dest host would have had to > adjust the guest to use the dest's settings for rows and cols after a > migration. Also, for "new" guests, they should use the control vq > command to adjust the rows and cols -- and they're not migrated since > they're not guest state. Right, I came to the same conclusion (see my reply to agraf). [snip] > > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f) > > uint32_t nr_active_ports; > > unsigned int i, max_nr_ports; > > > > - /* The config space */ > > - qemu_put_be16s(f, &s->config.cols); > > - qemu_put_be16s(f, &s->config.rows); > > + max_nr_ports = s->serial.max_virtserial_ports; > > > > - qemu_put_be32s(f, &s->config.max_nr_ports); > > + /* Used to be config space, now redundant */ > > + qemu_put_be16(f, 0); > > + qemu_put_be16(f, 0); > > + qemu_put_be32(f, virtio_tswap32(vdev, max_nr_ports)); > > Can you split this patch so the config change and the max_nr_ports > change are separate? The max_nr_ports could similarly be ignored by > dest, right? Um.. I'm not exactly sure where you're drawing the distinction between the two parts. Are you thinking patch 1) make config.max_nr_ports unused, by using max_virtserial_ports instead patch 2) eliminate the config field entirely
On (Tue) 16 Dec 2014 [15:33:54], David Gibson wrote: > On Tue, Dec 16, 2014 at 09:43:30AM +0530, Amit Shah wrote: > > On (Fri) 12 Dec 2014 [16:26:35], David Gibson wrote: > > > On a bi-endian target, with a guest in the non-default endian mode, > > > attempting to migrate twice in a row with a virtio-serial device wil > > > cause a qemu SEGV on the second outgoing migration. > > > > > > The problem is that virtio_serial_save_device() (and other places) expect > > > VirtIOSerial->config to be in current guest endianness. On a fresh boot, > > > virtio_serial_device_realize() will initialize VirtIOSerial->config in > > > default endianness. It's assumed the guest OS will make its true > > > endianness known before the device is reset and initialized, then > > > vser_reset adjusts VirtIOSerial->config into the new endianness. > > > > > > But on an incoming migration, the device isn't reset (after all the guest > > > has a running driver as far as it's concerned), which means that > > > VirtIOSerial->config retains its default endianness value from > > > virtio_serial_device_realize(). > > > > > > On a subsequent outgoing migration, virtio_serial_save_device() attempts > > > to interpret VirtIOSerial->config.max_nr_ports in current endianness when > > > its actually in default endianness and then runs off the end of the > > > ports_map array in the loop immediately afterwards. > > > > > > We could fix this by adjusting VirtIOSerial->config into the correct > > > current endianness after an incoming migration. But a better fix is just > > > to get rid of VirtIOSerial->config entirely: > > > * The virtio-serial config space is not settable, it always contains the > > > values set at initialization > > > * AFAICT "rows" and "cols" have never actually been used for anything and > > > are always zero. > > > > There were patches on the list a few years back to add resizing > > support. > > Well, apparently they were never merged, because I can't see a thing > in the git history. Yes, the submitter didn't follow up after review. > > Also, ppc and s390 people were using this feature (why else would it > > have been implemented?) -- since you're saying they're not in use, I > > suppose ppc doesn't use it. CC'ing s390 people for comment. > > > > > * "max_nr_ports" is initialized from > > > VirtIOSerial->serial.max_virtserial_ports (host endian) > > > > > > So instead of maintaining this pointless guest-endian cache of the config > > > data, we can just construct it directly into the correct current guest > > > endian in the get_config hook. Current users of ->config can instead use > > > the sources from which the config values were derived, which means they > > > don't have to mess about with converting from guest endian at all. > > > > I'd agree with this approach when I have confirmation no one actually > > uses the {rows,cols}. > > I've grepped the tree and searched through git history. I'm pretty > sure they're not used. > > And even if we do want to use them, this approach would still be sound > - it would make more sense to hold the rows and cols info in host > endian (which is well defined and static, at least) and just byteswap > it on get_config() where necessary. > > > Since qemu doesn't use the rows and cols, it doesn't matter what > > settings the dest host has; otherwise the dest host would have had to > > adjust the guest to use the dest's settings for rows and cols after a > > migration. Also, for "new" guests, they should use the control vq > > command to adjust the rows and cols -- and they're not migrated since > > they're not guest state. > > Right, I came to the same conclusion (see my reply to agraf). > > [snip] > > > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f) > > > uint32_t nr_active_ports; > > > unsigned int i, max_nr_ports; > > > > > > - /* The config space */ > > > - qemu_put_be16s(f, &s->config.cols); > > > - qemu_put_be16s(f, &s->config.rows); > > > + max_nr_ports = s->serial.max_virtserial_ports; > > > > > > - qemu_put_be32s(f, &s->config.max_nr_ports); > > > + /* Used to be config space, now redundant */ > > > + qemu_put_be16(f, 0); > > > + qemu_put_be16(f, 0); > > > + qemu_put_be32(f, virtio_tswap32(vdev, max_nr_ports)); > > > > Can you split this patch so the config change and the max_nr_ports > > change are separate? The max_nr_ports could similarly be ignored by > > dest, right? > > Um.. I'm not exactly sure where you're drawing the distinction between > the two parts. Are you thinking > patch 1) make config.max_nr_ports unused, by using > max_virtserial_ports instead > patch 2) eliminate the config field entirely For this patch, I'm just suggesting to only touch cols and rows, and lines that touch the max_nr_ports can be put in 2/2. Eliminating config entirely may not be desirable, but if you want to do that, and mark all fields as 'unused' in the savevm/loadvm functions, go for it :-) Amit
On Tue, Dec 16, 2014 at 10:08:49AM +0530, Amit Shah wrote: > On (Tue) 16 Dec 2014 [15:33:54], David Gibson wrote: > > On Tue, Dec 16, 2014 at 09:43:30AM +0530, Amit Shah wrote: > > > On (Fri) 12 Dec 2014 [16:26:35], David Gibson wrote: > > > > On a bi-endian target, with a guest in the non-default endian mode, > > > > attempting to migrate twice in a row with a virtio-serial device wil > > > > cause a qemu SEGV on the second outgoing migration. > > > > > > > > The problem is that virtio_serial_save_device() (and other places) expect > > > > VirtIOSerial->config to be in current guest endianness. On a fresh boot, > > > > virtio_serial_device_realize() will initialize VirtIOSerial->config in > > > > default endianness. It's assumed the guest OS will make its true > > > > endianness known before the device is reset and initialized, then > > > > vser_reset adjusts VirtIOSerial->config into the new endianness. > > > > > > > > But on an incoming migration, the device isn't reset (after all the guest > > > > has a running driver as far as it's concerned), which means that > > > > VirtIOSerial->config retains its default endianness value from > > > > virtio_serial_device_realize(). > > > > > > > > On a subsequent outgoing migration, virtio_serial_save_device() attempts > > > > to interpret VirtIOSerial->config.max_nr_ports in current endianness when > > > > its actually in default endianness and then runs off the end of the > > > > ports_map array in the loop immediately afterwards. > > > > > > > > We could fix this by adjusting VirtIOSerial->config into the correct > > > > current endianness after an incoming migration. But a better fix is just > > > > to get rid of VirtIOSerial->config entirely: > > > > * The virtio-serial config space is not settable, it always contains the > > > > values set at initialization > > > > * AFAICT "rows" and "cols" have never actually been used for anything and > > > > are always zero. > > > > > > There were patches on the list a few years back to add resizing > > > support. > > > > Well, apparently they were never merged, because I can't see a thing > > in the git history. > > Yes, the submitter didn't follow up after review. > > > > Also, ppc and s390 people were using this feature (why else would it > > > have been implemented?) -- since you're saying they're not in use, I > > > suppose ppc doesn't use it. CC'ing s390 people for comment. > > > > > > > * "max_nr_ports" is initialized from > > > > VirtIOSerial->serial.max_virtserial_ports (host endian) > > > > > > > > So instead of maintaining this pointless guest-endian cache of the config > > > > data, we can just construct it directly into the correct current guest > > > > endian in the get_config hook. Current users of ->config can instead use > > > > the sources from which the config values were derived, which means they > > > > don't have to mess about with converting from guest endian at all. > > > > > > I'd agree with this approach when I have confirmation no one actually > > > uses the {rows,cols}. > > > > I've grepped the tree and searched through git history. I'm pretty > > sure they're not used. > > > > And even if we do want to use them, this approach would still be sound > > - it would make more sense to hold the rows and cols info in host > > endian (which is well defined and static, at least) and just byteswap > > it on get_config() where necessary. > > > > > Since qemu doesn't use the rows and cols, it doesn't matter what > > > settings the dest host has; otherwise the dest host would have had to > > > adjust the guest to use the dest's settings for rows and cols after a > > > migration. Also, for "new" guests, they should use the control vq > > > command to adjust the rows and cols -- and they're not migrated since > > > they're not guest state. > > > > Right, I came to the same conclusion (see my reply to agraf). > > > > [snip] > > > > @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f) > > > > uint32_t nr_active_ports; > > > > unsigned int i, max_nr_ports; > > > > > > > > - /* The config space */ > > > > - qemu_put_be16s(f, &s->config.cols); > > > > - qemu_put_be16s(f, &s->config.rows); > > > > + max_nr_ports = s->serial.max_virtserial_ports; > > > > > > > > - qemu_put_be32s(f, &s->config.max_nr_ports); > > > > + /* Used to be config space, now redundant */ > > > > + qemu_put_be16(f, 0); > > > > + qemu_put_be16(f, 0); > > > > + qemu_put_be32(f, virtio_tswap32(vdev, max_nr_ports)); > > > > > > Can you split this patch so the config change and the max_nr_ports > > > change are separate? The max_nr_ports could similarly be ignored by > > > dest, right? > > > > Um.. I'm not exactly sure where you're drawing the distinction between > > the two parts. Are you thinking > > patch 1) make config.max_nr_ports unused, by using > > max_virtserial_ports instead > > patch 2) eliminate the config field entirely > > For this patch, I'm just suggesting to only touch cols and rows, and > lines that touch the max_nr_ports can be put in 2/2. Eliminating > config entirely may not be desirable, but if you want to do that, and > mark all fields as 'unused' in the savevm/loadvm functions, go for > it :-) That division really doesn't make sense to me. If rows and cols are removed, but max_nr_ports stays, then get_config has to become a weird hybrid where it copies some stuff directly from ->config and other bits from elsewhere. I really don't see any reason keeping config would be a desirable thing: it's a cache of information that doesn't need to be cached, and just adds complexity because of the endian issues.
On (Tue) 16 Dec 2014 [16:38:15], David Gibson wrote: > On Tue, Dec 16, 2014 at 10:08:49AM +0530, Amit Shah wrote: > > On (Tue) 16 Dec 2014 [15:33:54], David Gibson wrote: > > > On Tue, Dec 16, 2014 at 09:43:30AM +0530, Amit Shah wrote: > > > > Can you split this patch so the config change and the max_nr_ports > > > > change are separate? The max_nr_ports could similarly be ignored by > > > > dest, right? > > > > > > Um.. I'm not exactly sure where you're drawing the distinction between > > > the two parts. Are you thinking > > > patch 1) make config.max_nr_ports unused, by using > > > max_virtserial_ports instead > > > patch 2) eliminate the config field entirely > > > > For this patch, I'm just suggesting to only touch cols and rows, and > > lines that touch the max_nr_ports can be put in 2/2. Eliminating > > config entirely may not be desirable, but if you want to do that, and > > mark all fields as 'unused' in the savevm/loadvm functions, go for > > it :-) > > That division really doesn't make sense to me. If rows and cols are > removed, but max_nr_ports stays, then get_config has to become a > weird hybrid where it copies some stuff directly from ->config and > other bits from elsewhere. > > I really don't see any reason keeping config would be a desirable > thing: it's a cache of information that doesn't need to be cached, and > just adds complexity because of the endian issues. Oh please go ahead and do it -- my only point here is to split the patchset into logical units, not wrt the total intent of the series. Amit
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c index a7b1b68..dd5d7ec 100644 --- a/hw/char/virtio-serial-bus.c +++ b/hw/char/virtio-serial-bus.c @@ -482,10 +482,14 @@ static uint32_t get_features(VirtIODevice *vdev, uint32_t features) /* Guest requested config info */ static void get_config(VirtIODevice *vdev, uint8_t *config_data) { - VirtIOSerial *vser; - - vser = VIRTIO_SERIAL(vdev); - memcpy(config_data, &vser->config, sizeof(struct virtio_console_config)); + VirtIOSerial *vser = VIRTIO_SERIAL(vdev); + struct virtio_console_config *config = + (struct virtio_console_config *)config_data; + + config->cols = 0; + config->rows = 0; + config->max_nr_ports = virtio_tswap32(vdev, + vser->serial.max_virtserial_ports); } static void guest_reset(VirtIOSerial *vser) @@ -533,10 +537,6 @@ static void vser_reset(VirtIODevice *vdev) vser = VIRTIO_SERIAL(vdev); guest_reset(vser); - - /* In case we have switched endianness */ - vser->config.max_nr_ports = - virtio_tswap32(vdev, vser->serial.max_virtserial_ports); } static void virtio_serial_save(QEMUFile *f, void *opaque) @@ -552,14 +552,14 @@ static void virtio_serial_save_device(VirtIODevice *vdev, QEMUFile *f) uint32_t nr_active_ports; unsigned int i, max_nr_ports; - /* The config space */ - qemu_put_be16s(f, &s->config.cols); - qemu_put_be16s(f, &s->config.rows); + max_nr_ports = s->serial.max_virtserial_ports; - qemu_put_be32s(f, &s->config.max_nr_ports); + /* Used to be config space, now redundant */ + qemu_put_be16(f, 0); + qemu_put_be16(f, 0); + qemu_put_be32(f, virtio_tswap32(vdev, max_nr_ports)); /* The ports map */ - max_nr_ports = virtio_tswap32(vdev, s->config.max_nr_ports); for (i = 0; i < (max_nr_ports + 31) / 32; i++) { qemu_put_be32s(f, &s->ports_map[i]); } @@ -715,13 +715,7 @@ static int virtio_serial_load_device(VirtIODevice *vdev, QEMUFile *f, qemu_get_be16s(f, (uint16_t *) &tmp); qemu_get_be32s(f, &tmp); - /* Note: this is the only location where we use tswap32() instead of - * virtio_tswap32() because: - * - virtio_tswap32() only makes sense when the device is fully restored - * - the target endianness that was used to populate s->config is - * necessarly the default one - */ - max_nr_ports = tswap32(s->config.max_nr_ports); + max_nr_ports = s->serial.max_virtserial_ports; for (i = 0; i < (max_nr_ports + 31) / 32; i++) { qemu_get_be32s(f, &ports_map); @@ -784,10 +778,9 @@ static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent) /* This function is only used if a port id is not provided by the user */ static uint32_t find_free_port_id(VirtIOSerial *vser) { - VirtIODevice *vdev = VIRTIO_DEVICE(vser); unsigned int i, max_nr_ports; - max_nr_ports = virtio_tswap32(vdev, vser->config.max_nr_ports); + max_nr_ports = vser->serial.max_virtserial_ports; for (i = 0; i < (max_nr_ports + 31) / 32; i++) { uint32_t map, bit; @@ -848,7 +841,6 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev); VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port); VirtIOSerialBus *bus = VIRTIO_SERIAL_BUS(qdev_get_parent_bus(dev)); - VirtIODevice *vdev = VIRTIO_DEVICE(bus->vser); int max_nr_ports; bool plugging_port0; Error *err = NULL; @@ -890,7 +882,7 @@ static void virtser_port_device_realize(DeviceState *dev, Error **errp) } } - max_nr_ports = virtio_tswap32(vdev, port->vser->config.max_nr_ports); + max_nr_ports = port->vser->serial.max_virtserial_ports; if (port->id >= max_nr_ports) { error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, " "max. allowed: %u", max_nr_ports - 1); @@ -995,8 +987,6 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp) vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output); } - vser->config.max_nr_ports = - virtio_tswap32(vdev, vser->serial.max_virtserial_ports); vser->ports_map = g_malloc0(((vser->serial.max_virtserial_ports + 31) / 32) * sizeof(vser->ports_map[0])); /* diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h index a679e54..11af978 100644 --- a/include/hw/virtio/virtio-serial.h +++ b/include/hw/virtio/virtio-serial.h @@ -207,8 +207,6 @@ struct VirtIOSerial { /* bitmap for identifying active ports */ uint32_t *ports_map; - struct virtio_console_config config; - struct VirtIOSerialPostLoad *post_load; virtio_serial_conf serial;
On a bi-endian target, with a guest in the non-default endian mode, attempting to migrate twice in a row with a virtio-serial device wil cause a qemu SEGV on the second outgoing migration. The problem is that virtio_serial_save_device() (and other places) expect VirtIOSerial->config to be in current guest endianness. On a fresh boot, virtio_serial_device_realize() will initialize VirtIOSerial->config in default endianness. It's assumed the guest OS will make its true endianness known before the device is reset and initialized, then vser_reset adjusts VirtIOSerial->config into the new endianness. But on an incoming migration, the device isn't reset (after all the guest has a running driver as far as it's concerned), which means that VirtIOSerial->config retains its default endianness value from virtio_serial_device_realize(). On a subsequent outgoing migration, virtio_serial_save_device() attempts to interpret VirtIOSerial->config.max_nr_ports in current endianness when its actually in default endianness and then runs off the end of the ports_map array in the loop immediately afterwards. We could fix this by adjusting VirtIOSerial->config into the correct current endianness after an incoming migration. But a better fix is just to get rid of VirtIOSerial->config entirely: * The virtio-serial config space is not settable, it always contains the values set at initialization * AFAICT "rows" and "cols" have never actually been used for anything and are always zero. * "max_nr_ports" is initialized from VirtIOSerial->serial.max_virtserial_ports (host endian) So instead of maintaining this pointless guest-endian cache of the config data, we can just construct it directly into the correct current guest endian in the get_config hook. Current users of ->config can instead use the sources from which the config values were derived, which means they don't have to mess about with converting from guest endian at all. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/char/virtio-serial-bus.c | 42 +++++++++++++++------------------------ include/hw/virtio/virtio-serial.h | 2 -- 2 files changed, 16 insertions(+), 28 deletions(-)