[Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Alexey Kardashevskiy]

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(+)

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

Re: [Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Mike Day]

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
>
>

Re: [Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[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?


> 
>> ---
>>  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
>>
>>
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Andreas Färber]

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

-- 
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] usb-ohci: add vmstate descriptor

[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...

thanks
-- PMM

Re: [Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Andreas Färber]

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

-- 
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] usb-ohci: add vmstate descriptor

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Alexey Kardashevskiy]

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.


-- 
Alexey

Re: [Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Alexey Kardashevskiy]

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.




-- 
Alexey

Re: [Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] usb-ohci: add vmstate descriptor

[Gerd Hoffmann]

  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