[Qemu-devel] [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Qemu-devel] [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Alex Williamson]

PCI hotplug currently doesn't work after a migration because
we don't migrate the enable bits of the GPE state.  Pull hotplug
structs into vmstate.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/acpi_piix4.c |   29 ++++++++++++++++++++++++++++-
 1 files changed, 28 insertions(+), 1 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index a87286b..8d1a628 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -283,9 +283,33 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static const VMStateDescription vmstate_gpe = {
+    .name = "gpe",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT16(sts, struct gpe_regs),
+        VMSTATE_UINT16(en, struct gpe_regs),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_pci_status = {
+    .name = "pci_status",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_UINT32(up, struct pci_status),
+        VMSTATE_UINT32(down, struct pci_status),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_acpi = {
     .name = "piix4_pm",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .minimum_version_id_old = 1,
     .post_load = vmstate_acpi_post_load,
@@ -297,6 +321,9 @@ static const VMStateDescription vmstate_acpi = {
         VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
         VMSTATE_TIMER(tmr_timer, PIIX4PMState),
         VMSTATE_INT64(tmr_overflow_time, PIIX4PMState),
+        VMSTATE_STRUCT(gpe, PIIX4PMState, 2, vmstate_gpe, struct gpe_regs),
+        VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status,
+                       struct pci_status),
         VMSTATE_END_OF_LIST()
     }
 };

Re: [Qemu-devel] [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Anthony Liguori]

On 06/14/2010 03:28 PM, Alex Williamson wrote:
> PCI hotplug currently doesn't work after a migration because
> we don't migrate the enable bits of the GPE state.  Pull hotplug
> structs into vmstate.
>
> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>
>   hw/acpi_piix4.c |   29 ++++++++++++++++++++++++++++-
>   1 files changed, 28 insertions(+), 1 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index a87286b..8d1a628 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -283,9 +283,33 @@ static int vmstate_acpi_post_load(void *opaque, int version_id)
>       return 0;
>   }
>
> +static const VMStateDescription vmstate_gpe = {
> +    .name = "gpe",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINT16(sts, struct gpe_regs),
> +        VMSTATE_UINT16(en, struct gpe_regs),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_pci_status = {
> +    .name = "pci_status",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField []) {
> +        VMSTATE_UINT32(up, struct pci_status),
> +        VMSTATE_UINT32(down, struct pci_status),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static const VMStateDescription vmstate_acpi = {
>       .name = "piix4_pm",
> -    .version_id = 1,
> +    .version_id = 2,
>       .minimum_version_id = 1,
>       .minimum_version_id_old = 1,
>       .post_load = vmstate_acpi_post_load,
> @@ -297,6 +321,9 @@ static const VMStateDescription vmstate_acpi = {
>           VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
>           VMSTATE_TIMER(tmr_timer, PIIX4PMState),
>           VMSTATE_INT64(tmr_overflow_time, PIIX4PMState),
> +        VMSTATE_STRUCT(gpe, PIIX4PMState, 2, vmstate_gpe, struct gpe_regs),
> +        VMSTATE_STRUCT(pci0_status, PIIX4PMState, 2, vmstate_pci_status,
> +                       struct pci_status),
>           VMSTATE_END_OF_LIST()
>       }
>   };
>
>
>
>    

Re: [Qemu-devel] [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Alexander Graf]

On 14.06.2010, at 22:58, Anthony Liguori wrote:

> On 06/14/2010 03:28 PM, Alex Williamson wrote:
>> PCI hotplug currently doesn't work after a migration because
>> we don't migrate the enable bits of the GPE state.  Pull hotplug
>> structs into vmstate.
>> 
>> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
>>   
> 
> Applied.  Thanks.

Is this stable material?

Alex

[Qemu-devel] Re: [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Juan Quintela]

Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 06/14/2010 03:28 PM, Alex Williamson wrote:
>> PCI hotplug currently doesn't work after a migration because
>> we don't migrate the enable bits of the GPE state.  Pull hotplug
>> structs into vmstate.
>>
>> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
>>    
>
> Applied.  Thanks.
>
> Regards,
>
> Anthony Liguori

I think this is better implemented as a subsection.  We didin't need
this until hotplug arrived, I think that checking if any up/down are
!= 0 and then send it as subsections is a best way to do it.

This way it could also be backported to stable.

Later, Juan.

[Qemu-devel] Re: [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Alex Williamson]

On Wed, 2010-06-16 at 17:47 +0200, Juan Quintela wrote:
> Anthony Liguori <anthony@codemonkey.ws> wrote:
> > On 06/14/2010 03:28 PM, Alex Williamson wrote:
> >> PCI hotplug currently doesn't work after a migration because
> >> we don't migrate the enable bits of the GPE state.  Pull hotplug
> >> structs into vmstate.
> >>
> >> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> >>    
> >
> > Applied.  Thanks.
> >
> > Regards,
> >
> > Anthony Liguori
> 
> I think this is better implemented as a subsection.  We didin't need
> this until hotplug arrived, I think that checking if any up/down are
> != 0 and then send it as subsections is a best way to do it.
> 
> This way it could also be backported to stable.

The slots aren't really the issue, they were mostly for completeness.
The key is gpe.en, which is likely always going to be all 1s for an ACPI
aware OS.  So if we test != 0, we're going to need that subsection in
99% of the cases.  Maybe we can assume gpe.en is all set on the target,
but I don't really look forward to finding out the ways that might
break.  Thanks,

Alex

[Qemu-devel] Re: [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Juan Quintela]

Alex Williamson <alex.williamson@redhat.com> wrote:
> On Wed, 2010-06-16 at 17:47 +0200, Juan Quintela wrote:
>> Anthony Liguori <anthony@codemonkey.ws> wrote:
>> > On 06/14/2010 03:28 PM, Alex Williamson wrote:
>> >> PCI hotplug currently doesn't work after a migration because
>> >> we don't migrate the enable bits of the GPE state.  Pull hotplug
>> >> structs into vmstate.
>> >>
>> >> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
>> >>    
>> >
>> > Applied.  Thanks.
>> >
>> > Regards,
>> >
>> > Anthony Liguori
>> 
>> I think this is better implemented as a subsection.  We didin't need
>> this until hotplug arrived, I think that checking if any up/down are
>> != 0 and then send it as subsections is a best way to do it.
>> 
>> This way it could also be backported to stable.
>
> The slots aren't really the issue, they were mostly for completeness.
> The key is gpe.en, which is likely always going to be all 1s for an ACPI
> aware OS.  So if we test != 0, we're going to need that subsection in
> 99% of the cases.  Maybe we can assume gpe.en is all set on the target,
> but I don't really look forward to finding out the ways that might
> break.  Thanks,

We have never sent it before.  That means that the default value (for
whatever value is it) should be working quite well.

But it is not my call.

Later, Juan.

[Qemu-devel] Re: [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Anthony Liguori]

On 06/16/2010 01:43 PM, Juan Quintela wrote:
> Alex Williamson<alex.williamson@redhat.com>  wrote:
>    
>> On Wed, 2010-06-16 at 17:47 +0200, Juan Quintela wrote:
>>      
>>> Anthony Liguori<anthony@codemonkey.ws>  wrote:
>>>        
>>>> On 06/14/2010 03:28 PM, Alex Williamson wrote:
>>>>          
>>>>> PCI hotplug currently doesn't work after a migration because
>>>>> we don't migrate the enable bits of the GPE state.  Pull hotplug
>>>>> structs into vmstate.
>>>>>
>>>>> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
>>>>>
>>>>>            
>>>> Applied.  Thanks.
>>>>
>>>> Regards,
>>>>
>>>> Anthony Liguori
>>>>          
>>> I think this is better implemented as a subsection.  We didin't need
>>> this until hotplug arrived, I think that checking if any up/down are
>>> != 0 and then send it as subsections is a best way to do it.
>>>
>>> This way it could also be backported to stable.
>>>        
>> The slots aren't really the issue, they were mostly for completeness.
>> The key is gpe.en, which is likely always going to be all 1s for an ACPI
>> aware OS.  So if we test != 0, we're going to need that subsection in
>> 99% of the cases.  Maybe we can assume gpe.en is all set on the target,
>> but I don't really look forward to finding out the ways that might
>> break.  Thanks,
>>      
> We have never sent it before.  That means that the default value (for
> whatever value is it) should be working quite well.
>
> But it is not my call.
>    

This sounds like a case where we're sending broken state so we need to 
bump the version number.  Subsections don't eliminate the need to bump 
version numbers when we fundamentally screw up.

Regards,

Anthony Liguori

> Later, Juan.
>    

[Qemu-devel] Re: [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Alex Williamson]

On Wed, 2010-06-16 at 20:43 +0200, Juan Quintela wrote:
> Alex Williamson <alex.williamson@redhat.com> wrote:
> > On Wed, 2010-06-16 at 17:47 +0200, Juan Quintela wrote:
> >> Anthony Liguori <anthony@codemonkey.ws> wrote:
> >> > On 06/14/2010 03:28 PM, Alex Williamson wrote:
> >> >> PCI hotplug currently doesn't work after a migration because
> >> >> we don't migrate the enable bits of the GPE state.  Pull hotplug
> >> >> structs into vmstate.
> >> >>
> >> >> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
> >> >>    
> >> >
> >> > Applied.  Thanks.
> >> >
> >> > Regards,
> >> >
> >> > Anthony Liguori
> >> 
> >> I think this is better implemented as a subsection.  We didin't need
> >> this until hotplug arrived, I think that checking if any up/down are
> >> != 0 and then send it as subsections is a best way to do it.
> >> 
> >> This way it could also be backported to stable.
> >
> > The slots aren't really the issue, they were mostly for completeness.
> > The key is gpe.en, which is likely always going to be all 1s for an ACPI
> > aware OS.  So if we test != 0, we're going to need that subsection in
> > 99% of the cases.  Maybe we can assume gpe.en is all set on the target,
> > but I don't really look forward to finding out the ways that might
> > break.  Thanks,
> 
> We have never sent it before.  That means that the default value (for
> whatever value is it) should be working quite well.

The gpe.en bits back an ioport range that's poked by the guest via ACPI.
So the guest is the one telling ACPI "I enable you to send me this gpe".
When a hotplug event happens, we figure out which gpe line it would
toggle, set the matching gpe.sts bit, then if the OS has told us to
enable that bit, we send an interrupt.  The OS then masks gpe.en, checks
gpe.sts, preforms actions and clears the respective gpe.sts bits, then
re-enables gpe.en.  So, qemu never sets the bits, but if we decide to
clear (or set) them, we're interfering with how the OS expects them to
work.  I think we're screwed, we need to bump the rev.

Alex

[Qemu-devel] Re: [RESEND PATCH] acpi_piix4: save gpe and pci hotplug slot status

[Juan Quintela]

Alex Williamson <alex.williamson@redhat.com> wrote:
> On Wed, 2010-06-16 at 20:43 +0200, Juan Quintela wrote:
>> Alex Williamson <alex.williamson@redhat.com> wrote:
>> > On Wed, 2010-06-16 at 17:47 +0200, Juan Quintela wrote:
>> >> Anthony Liguori <anthony@codemonkey.ws> wrote:
>> >> > On 06/14/2010 03:28 PM, Alex Williamson wrote:
>> >> >> PCI hotplug currently doesn't work after a migration because
>> >> >> we don't migrate the enable bits of the GPE state.  Pull hotplug
>> >> >> structs into vmstate.
>> >> >>
>> >> >> Signed-off-by: Alex Williamson<alex.williamson@redhat.com>
>> >> >>    
>> >> >
>> >> > Applied.  Thanks.
>> >> >
>> >> > Regards,
>> >> >
>> >> > Anthony Liguori
>> >> 
>> >> I think this is better implemented as a subsection.  We didin't need
>> >> this until hotplug arrived, I think that checking if any up/down are
>> >> != 0 and then send it as subsections is a best way to do it.
>> >> 
>> >> This way it could also be backported to stable.
>> >
>> > The slots aren't really the issue, they were mostly for completeness.
>> > The key is gpe.en, which is likely always going to be all 1s for an ACPI
>> > aware OS.  So if we test != 0, we're going to need that subsection in
>> > 99% of the cases.  Maybe we can assume gpe.en is all set on the target,
>> > but I don't really look forward to finding out the ways that might
>> > break.  Thanks,
>> 
>> We have never sent it before.  That means that the default value (for
>> whatever value is it) should be working quite well.
>
> The gpe.en bits back an ioport range that's poked by the guest via ACPI.
> So the guest is the one telling ACPI "I enable you to send me this gpe".
> When a hotplug event happens, we figure out which gpe line it would
> toggle, set the matching gpe.sts bit, then if the OS has told us to
> enable that bit, we send an interrupt.  The OS then masks gpe.en, checks
> gpe.sts, preforms actions and clears the respective gpe.sts bits, then
> re-enables gpe.en.  So, qemu never sets the bits, but if we decide to
> clear (or set) them, we're interfering with how the OS expects them to
> work.  I think we're screwed, we need to bump the rev.

Sniff.  I tried.  Cross version migration is such a nice thing :(

Later, Juan.