| Message ID | 1394088217-4504-1-git-send-email-aik@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > This adds migration support for OHCI. > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> Reviewed-by: Mike Day <ncmike@ncultra.org> > --- > hw/usb/hcd-ohci.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > index e38cdeb..c42e091 100644 > --- a/hw/usb/hcd-ohci.c > +++ b/hw/usb/hcd-ohci.c > @@ -1984,6 +1984,17 @@ static Property ohci_pci_properties[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static const VMStateDescription vmstate_ohci = { > + .name = "ohci", > + .version_id = 1, > + .minimum_version_id = 1, > + .minimum_version_id_old = 1, > + .fields = (VMStateField[]) { > + VMSTATE_PCI_DEVICE(parent_obj, OHCIPCIState), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static void ohci_pci_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > @@ -1997,6 +2008,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data) > set_bit(DEVICE_CATEGORY_USB, dc->categories); > dc->desc = "Apple USB Controller"; > dc->props = ohci_pci_properties; > + dc->vmsd = &vmstate_ohci; > } > > static const TypeInfo ohci_pci_info = { > -- > 1.8.4.rc4 > >
On 03/07/2014 12:57 AM, Mike Day wrote: > > Alexey Kardashevskiy <aik@ozlabs.ru> writes: > >> This adds migration support for OHCI. >> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > Reviewed-by: Mike Day <ncmike@ncultra.org> Thanks! What is next? > >> --- >> hw/usb/hcd-ohci.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c >> index e38cdeb..c42e091 100644 >> --- a/hw/usb/hcd-ohci.c >> +++ b/hw/usb/hcd-ohci.c >> @@ -1984,6 +1984,17 @@ static Property ohci_pci_properties[] = { >> DEFINE_PROP_END_OF_LIST(), >> }; >> >> +static const VMStateDescription vmstate_ohci = { >> + .name = "ohci", >> + .version_id = 1, >> + .minimum_version_id = 1, >> + .minimum_version_id_old = 1, >> + .fields = (VMStateField[]) { >> + VMSTATE_PCI_DEVICE(parent_obj, OHCIPCIState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> static void ohci_pci_class_init(ObjectClass *klass, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(klass); >> @@ -1997,6 +2008,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data) >> set_bit(DEVICE_CATEGORY_USB, dc->categories); >> dc->desc = "Apple USB Controller"; >> dc->props = ohci_pci_properties; >> + dc->vmsd = &vmstate_ohci; >> } >> >> static const TypeInfo ohci_pci_info = { >> -- >> 1.8.4.rc4 >> >> >
Am 14.03.2014 06:09, schrieb Alexey Kardashevskiy: > On 03/07/2014 12:57 AM, Mike Day wrote: >> >> Alexey Kardashevskiy <aik@ozlabs.ru> writes: >> >>> This adds migration support for OHCI. >>> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> >> Reviewed-by: Mike Day <ncmike@ncultra.org> > > Thanks! > > What is next? The USB maintainer (that you've now CC'ed) needs to review and pick it up. It may help to understand what went wrong without it. Because AFAIU migration is possible without VMSD, just not with VMSD that sets .unmigratable = 1. Understanding what this patch is about may also help in determining whether this needs to be fixed in 2.0 or not. That said, the patch does look okay to me on brief sight. Regards, Andreas
On 22 March 2014 20:18, Andreas Färber <afaerber@suse.de> wrote: > Because AFAIU > migration is possible without VMSD, just not with VMSD that sets > .unmigratable = 1. Well, the migration won't fail with an error, but on the destination end you'll end up with a device in its reset state but a guest which may think the device is in some other state. If the device was quiescent and doesn't need complex setup it might cope, but more likely is that that guest driver will fall over in a heap next time you try to use it... thanks -- PMM
Am 22.03.2014 21:54, schrieb Peter Maydell: > On 22 March 2014 20:18, Andreas Färber <afaerber@suse.de> wrote: >> Because AFAIU >> migration is possible without VMSD, just not with VMSD that sets >> .unmigratable = 1. > > Well, the migration won't fail with an error, but on the destination > end you'll end up with a device in its reset state but a guest which > may think the device is in some other state. If the device was > quiescent and doesn't need complex setup it might cope, but > more likely is that that guest driver will fall over in a heap next > time you try to use it... Well, there is no OHCI state being added, only PCI state. So I'd be curious to know what in there is the problem because a general review of PCI devices might be due then - and ideally before we do the release. Cheers, Andreas
On 22 March 2014 21:04, Andreas Färber <afaerber@suse.de> wrote: > Am 22.03.2014 21:54, schrieb Peter Maydell: >> On 22 March 2014 20:18, Andreas Färber <afaerber@suse.de> wrote: >>> Because AFAIU >>> migration is possible without VMSD, just not with VMSD that sets >>> .unmigratable = 1. >> >> Well, the migration won't fail with an error, but on the destination >> end you'll end up with a device in its reset state but a guest which >> may think the device is in some other state. If the device was >> quiescent and doesn't need complex setup it might cope, but >> more likely is that that guest driver will fall over in a heap next >> time you try to use it... > > Well, there is no OHCI state being added, only PCI state. So I'd be > curious to know what in there is the problem because a general review of > PCI devices might be due then - and ideally before we do the release. Oops, I hadn't noticed that; this patch is incorrect, then, because vmstate_ohci needs to include a line for the OHCIState, and we need a second vmstate struct for the OHCIState. thanks -- PMM
On 03/23/2014 08:04 AM, Andreas Färber wrote: > Am 22.03.2014 21:54, schrieb Peter Maydell: >> On 22 March 2014 20:18, Andreas Färber <afaerber@suse.de> wrote: >>> Because AFAIU >>> migration is possible without VMSD, just not with VMSD that sets >>> .unmigratable = 1. >> >> Well, the migration won't fail with an error, but on the destination >> end you'll end up with a device in its reset state but a guest which >> may think the device is in some other state. If the device was >> quiescent and doesn't need complex setup it might cope, but >> more likely is that that guest driver will fall over in a heap next >> time you try to use it... > > Well, there is no OHCI state being added, only PCI state. So I'd be > curious to know what in there is the problem because a general review of > PCI devices might be due then - and ideally before we do the release. Without the patch, if to migrate a machine while it is in SLOF between BARs assigned and USB driver started working, the BAR memory regions were not enabled and BAR accesses were ending up in unassigned_mem_ops.
On 03/23/2014 08:23 AM, Peter Maydell wrote: > On 22 March 2014 21:04, Andreas Färber <afaerber@suse.de> wrote: >> Am 22.03.2014 21:54, schrieb Peter Maydell: >>> On 22 March 2014 20:18, Andreas Färber <afaerber@suse.de> wrote: >>>> Because AFAIU >>>> migration is possible without VMSD, just not with VMSD that sets >>>> .unmigratable = 1. >>> >>> Well, the migration won't fail with an error, but on the destination >>> end you'll end up with a device in its reset state but a guest which >>> may think the device is in some other state. If the device was >>> quiescent and doesn't need complex setup it might cope, but >>> more likely is that that guest driver will fall over in a heap next >>> time you try to use it... >> >> Well, there is no OHCI state being added, only PCI state. So I'd be >> curious to know what in there is the problem because a general review of >> PCI devices might be due then - and ideally before we do the release. > > Oops, I hadn't noticed that; this patch is incorrect, then, because > vmstate_ohci needs to include a line for the OHCIState, and we > need a second vmstate struct for the OHCIState. Sorry but what is incorrect in the patch? I can understand that it is incomplete as it is missing OHCI-specific bits from the OHCIState state and I can do that but I need some hints what is really necessary. So far the USB device was able to recover, only PCI bits were really needed. Thanks.
On 24 March 2014 05:53, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: > On 03/23/2014 08:23 AM, Peter Maydell wrote: >> Oops, I hadn't noticed that; this patch is incorrect, then, because >> vmstate_ohci needs to include a line for the OHCIState, and we >> need a second vmstate struct for the OHCIState. > Sorry but what is incorrect in the patch? I can understand that it is > incomplete as it is missing OHCI-specific bits from the OHCIState state and > I can do that but I need some hints what is really necessary. So far the > USB device was able to recover, only PCI bits were really needed. Thanks. As I say above, you're not saving all the state. You need to include a line for the OHCIState which refers to a second vmstate struct to save and load the information in the OHCIState. Compare the EHCI save/load for an example. You need to save everything which isn't constant (ie a device property or a reference to another part of the model). So for OHCIState you don't need to save irq, mem, as, num_ports or bus (the first 6 struct members), or localmem_base. It looks like you don't need to save USBPacket or USBPort fields, since the other USB controllers don't (though I'm not sure why not -- Gerd, do you know why this is OK?). You do need to save everything else. Use a third vmstate for 'struct OHCIPort' so you can handle the arrays of those structs in OHCIState. The idea is that after migration the device should be in exactly the same state as before, as far as the guest can tell. Your patch relies on the guest OS being able to cope with it being in the wrong state (probably by figuring out it's broken and resetting it, or perhaps you only tested migration when the device was pretty much idle). thanks -- PMM
Hi, > It looks like you don't need to save USBPacket or USBPort > fields, since the other USB controllers don't (though I'm not > sure why not -- Gerd, do you know why this is OK?). Most state info is in guest memory. Any unfinished usb transfers are simply restarted on the target host. This can be as simple doing nothing (ehci for example, and I think ohci is simliar) as the next frame timer run will automatically pick up any unfinished work from the usb host controller transfer descriptors in guest memory. xhci is a bit more complicated as it has to reload state from guest memory data structures, setup timers etc. > The idea is that after migration the device should be in > exactly the same state as before, as far as the guest can > tell. Your patch relies on the guest OS being able to cope > with it being in the wrong state (probably by figuring out it's > broken and resetting it, or perhaps you only tested migration > when the device was pretty much idle). Could also be that the device simply doesn't do anything on the target due to the frame timer not being migrated. Try the following: * configure linux guest with usb keyboard connected via ohci. * boot linux guest to the login prompt, then live-migrate (or save/restore). * check whenever the keyboard still works, check dmesg. I expect you'll see either a dead keyboard or error messages in the kernel log about ohci failure followed by a device reset & usb bus rescan. cheers, Gerd
On 03/24/2014 08:26 PM, Peter Maydell wrote: > On 24 March 2014 05:53, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >> On 03/23/2014 08:23 AM, Peter Maydell wrote: >>> Oops, I hadn't noticed that; this patch is incorrect, then, because >>> vmstate_ohci needs to include a line for the OHCIState, and we >>> need a second vmstate struct for the OHCIState. > >> Sorry but what is incorrect in the patch? I can understand that it is >> incomplete as it is missing OHCI-specific bits from the OHCIState state and >> I can do that but I need some hints what is really necessary. So far the >> USB device was able to recover, only PCI bits were really needed. Thanks. > > As I say above, you're not saving all the state. You need to include > a line for the OHCIState which refers to a second vmstate struct > to save and load the information in the OHCIState. Compare the > EHCI save/load for an example. > > You need to save everything which isn't constant (ie a device > property or a reference to another part of the model). So for > OHCIState you don't need to save irq, mem, as, num_ports > or bus (the first 6 struct members), or localmem_base. > It looks like you don't need to save USBPacket or USBPort > fields, since the other USB controllers don't (though I'm not > sure why not -- Gerd, do you know why this is OK?). > You do need to save everything else. Use a third vmstate for > 'struct OHCIPort' so you can handle the arrays of those structs > in OHCIState. What is a "third" vmstate? I was going to use VMSTATE_STRUCT_ARRAY.
On 04/13/2014 01:09 PM, Alexey Kardashevskiy wrote: > On 03/24/2014 08:26 PM, Peter Maydell wrote: >> On 24 March 2014 05:53, Alexey Kardashevskiy <aik@ozlabs.ru> wrote: >>> On 03/23/2014 08:23 AM, Peter Maydell wrote: >>>> Oops, I hadn't noticed that; this patch is incorrect, then, because >>>> vmstate_ohci needs to include a line for the OHCIState, and we >>>> need a second vmstate struct for the OHCIState. >> >>> Sorry but what is incorrect in the patch? I can understand that it is >>> incomplete as it is missing OHCI-specific bits from the OHCIState state and >>> I can do that but I need some hints what is really necessary. So far the >>> USB device was able to recover, only PCI bits were really needed. Thanks. >> >> As I say above, you're not saving all the state. You need to include >> a line for the OHCIState which refers to a second vmstate struct >> to save and load the information in the OHCIState. Compare the >> EHCI save/load for an example. >> >> You need to save everything which isn't constant (ie a device >> property or a reference to another part of the model). So for >> OHCIState you don't need to save irq, mem, as, num_ports >> or bus (the first 6 struct members), or localmem_base. >> It looks like you don't need to save USBPacket or USBPort >> fields, since the other USB controllers don't (though I'm not >> sure why not -- Gerd, do you know why this is OK?). >> You do need to save everything else. Use a third vmstate for >> 'struct OHCIPort' so you can handle the arrays of those structs >> in OHCIState. > > What is a "third" vmstate? I was going to use VMSTATE_STRUCT_ARRAY. Never mind, got it.
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c index e38cdeb..c42e091 100644 --- a/hw/usb/hcd-ohci.c +++ b/hw/usb/hcd-ohci.c @@ -1984,6 +1984,17 @@ static Property ohci_pci_properties[] = { DEFINE_PROP_END_OF_LIST(), }; +static const VMStateDescription vmstate_ohci = { + .name = "ohci", + .version_id = 1, + .minimum_version_id = 1, + .minimum_version_id_old = 1, + .fields = (VMStateField[]) { + VMSTATE_PCI_DEVICE(parent_obj, OHCIPCIState), + VMSTATE_END_OF_LIST() + } +}; + static void ohci_pci_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -1997,6 +2008,7 @@ static void ohci_pci_class_init(ObjectClass *klass, void *data) set_bit(DEVICE_CATEGORY_USB, dc->categories); dc->desc = "Apple USB Controller"; dc->props = ohci_pci_properties; + dc->vmsd = &vmstate_ohci; } static const TypeInfo ohci_pci_info = {
This adds migration support for OHCI. Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- hw/usb/hcd-ohci.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)