| Message ID | m3k4tl4ovy.fsf@trasno.mitica |
|---|---|
| State | New |
| Headers | show |
On (Tue) Mar 09 2010 [14:15:45], Juan Quintela wrote: > > Hi Amit Hey Juan, > Checking migration, I just found this problem: > > I don't know what to put there. a return -EINVAL or continue? > Looking more at the code, I am not sure what checks: > > a- that bus->max_nr_ports is the same in both sides (or at least bigger > on migration destination) Yes, we should check for this. > b- We sent the value of config.nr_ports, but ... we assign it back on > destination, instead of checking that they are the same. This is done to accomodate for hot-plug/unplug. nr_ports will go up / down after those operations. (Current implementation only increases this value, on hotplug operations. On hot-unplug, this value is not decremented.) > c- port->id is taken from nr_ports again, and nothing checks that ports > appear in the same order in source and destination. Actually, this has me thinking about how would this work: - start vm with 3 ports - hotplug 2 more ports - migrate Would the destination have 5 ports, or would it have 3? I thought qdev would take care of this scenario (hotplug). I don't think I've tested this, so I'll do this soon, but in case anyone knows the answer, please let me know. [snipped patch that's necessary in case qdev doesn't handle this kind of hotplug] Amit
On (Tue) Mar 09 2010 [20:59:58], Amit Shah wrote: > On (Tue) Mar 09 2010 [14:15:45], Juan Quintela wrote: > > > > Hi Amit > > Hey Juan, > > > Checking migration, I just found this problem: > > > > I don't know what to put there. a return -EINVAL or continue? > > Looking more at the code, I am not sure what checks: > > > > a- that bus->max_nr_ports is the same in both sides (or at least bigger > > on migration destination) > > Yes, we should check for this. I've added this check in my local tree. > > b- We sent the value of config.nr_ports, but ... we assign it back on > > destination, instead of checking that they are the same. > > This is done to accomodate for hot-plug/unplug. nr_ports will go up / > down after those operations. (Current implementation only increases this > value, on hotplug operations. On hot-unplug, this value is not > decremented.) For now I've added a check that tests nr_ports == s->config.nr_ports. If that's not true, then return with -EINVAL. These two were small changes, no problem. > > c- port->id is taken from nr_ports again, and nothing checks that ports > > appear in the same order in source and destination. > > Actually, this has me thinking about how would this work: > - start vm with 3 ports > - hotplug 2 more ports > - migrate > > Would the destination have 5 ports, or would it have 3? I thought qdev > would take care of this scenario (hotplug). I don't think I've tested > this, so I'll do this soon, but in case anyone knows the answer, please > let me know. I spoke with Dan and he confirmed libvirt starts qemu instances on the destination computer with the appropriate devices, taking into consideration the hotplug/unplug status. However, the only problem here is virtio-serial ports are allocated an 'id', ie., a port number, and this is auto-assigned when a new port is found by adding 1 to the previous id. To make this whole thing independent of the order in which ports are added / removed, we'll have to accept the port id on the command line. This also means that we'll have to let the kernel know of the id because the control messages that the kernel and qemu exchange contain the port id. So basically some design changes will have to be incorporated both, in the kernel as well as in qemu to accomodate this. If this sounds right, I'll get to this right away. If there's an easier solution, please let me know. Amit
--- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -429,6 +429,10 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) id = qemu_get_be32(f); port = find_port_by_id(s, id); + if (port == NULL) { + return -EINVAL; + } + port->guest_connected = qemu_get_byte(f); }