diff mbox

[qemu-kvm] Fix: SCI isn't sent on cpu hotplug

Message ID 1326058804-561-1-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Jan. 8, 2012, 9:40 p.m. UTC
Change introduced by e71f08bb4a
    "Fix cpu/pci hotplug to generate level triggered interrupt."
 was lost somewhre along the way. And as result SCI is not sent in
 case of cpu hotplug event.
 Restoring hunk 1 of e71f08bb4a fixes issue.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi_piix4.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

Comments

Gleb Natapov Jan. 9, 2012, 8:47 a.m. UTC | #1
On Sun, Jan 08, 2012 at 10:40:04PM +0100, Igor Mammedov wrote:
>  Change introduced by e71f08bb4a
>     "Fix cpu/pci hotplug to generate level triggered interrupt."
>  was lost somewhre along the way. And as result SCI is not sent in
>  case of cpu hotplug event.
>  Restoring hunk 1 of e71f08bb4a fixes issue.
> 
Hmm, I sent similar patch [1] last time someone complained about cpu hotplug
here. Which remind me that in that thread more problem were found in cpu
hotplug. IIRC Jan collected all the patches. Jan, what happened to
them?

[1] http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/76399

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi_piix4.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 1b35707..741a8cc 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -94,7 +94,8 @@ static void pm_update_sci(PIIX4PMState *s)
>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>                     ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> -        (((s->gpe.sts[0] & s->gpe.en[0]) & PIIX4_PCI_HOTPLUG_STATUS) != 0);
> +        (((s->gpe.sts[0] & s->gpe.en[0]) &
> +          (PIIX4_CPU_HOTPLUG_STATUS | PIIX4_PCI_HOTPLUG_STATUS)) != 0);
>  
>      qemu_set_irq(s->irq, sci_level);
>      /* schedule a timer interruption if needed */
> -- 
> 1.7.7.5

--
			Gleb.
Jan Kiszka Jan. 9, 2012, 10:03 a.m. UTC | #2
On 2012-01-09 09:47, Gleb Natapov wrote:
> On Sun, Jan 08, 2012 at 10:40:04PM +0100, Igor Mammedov wrote:
>>  Change introduced by e71f08bb4a
>>     "Fix cpu/pci hotplug to generate level triggered interrupt."
>>  was lost somewhre along the way. And as result SCI is not sent in
>>  case of cpu hotplug event.
>>  Restoring hunk 1 of e71f08bb4a fixes issue.
>>
> Hmm, I sent similar patch [1] last time someone complained about cpu hotplug
> here. Which remind me that in that thread more problem were found in cpu
> hotplug. IIRC Jan collected all the patches. Jan, what happened to
> them?

My patches should have been superseded by the work of Ping Fan on the
ICC bus.

Jan

> 
> [1] http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/76399
> 
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>>  hw/acpi_piix4.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>
>> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
>> index 1b35707..741a8cc 100644
>> --- a/hw/acpi_piix4.c
>> +++ b/hw/acpi_piix4.c
>> @@ -94,7 +94,8 @@ static void pm_update_sci(PIIX4PMState *s)
>>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
>>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
>>                     ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
>> -        (((s->gpe.sts[0] & s->gpe.en[0]) & PIIX4_PCI_HOTPLUG_STATUS) != 0);
>> +        (((s->gpe.sts[0] & s->gpe.en[0]) &
>> +          (PIIX4_CPU_HOTPLUG_STATUS | PIIX4_PCI_HOTPLUG_STATUS)) != 0);
>>  
>>      qemu_set_irq(s->irq, sci_level);
>>      /* schedule a timer interruption if needed */
>> -- 
>> 1.7.7.5
> 
> --
> 			Gleb.
Gleb Natapov Jan. 9, 2012, 10:07 a.m. UTC | #3
On Mon, Jan 09, 2012 at 11:03:10AM +0100, Jan Kiszka wrote:
> On 2012-01-09 09:47, Gleb Natapov wrote:
> > On Sun, Jan 08, 2012 at 10:40:04PM +0100, Igor Mammedov wrote:
> >>  Change introduced by e71f08bb4a
> >>     "Fix cpu/pci hotplug to generate level triggered interrupt."
> >>  was lost somewhre along the way. And as result SCI is not sent in
> >>  case of cpu hotplug event.
> >>  Restoring hunk 1 of e71f08bb4a fixes issue.
> >>
> > Hmm, I sent similar patch [1] last time someone complained about cpu hotplug
> > here. Which remind me that in that thread more problem were found in cpu
> > hotplug. IIRC Jan collected all the patches. Jan, what happened to
> > them?
> 
> My patches should have been superseded by the work of Ping Fan on the
> ICC bus.
> 
Didn't they fix some problems with bringing new cpu online, not just
making cpu hotplugable in qdev? May be I misremember. BTW do you know
what happened with ICC bus patches?

> Jan
> 
> > 
> > [1] http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/76399
> > 
> >> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >> ---
> >>  hw/acpi_piix4.c |    3 ++-
> >>  1 files changed, 2 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> >> index 1b35707..741a8cc 100644
> >> --- a/hw/acpi_piix4.c
> >> +++ b/hw/acpi_piix4.c
> >> @@ -94,7 +94,8 @@ static void pm_update_sci(PIIX4PMState *s)
> >>                     ACPI_BITMASK_POWER_BUTTON_ENABLE |
> >>                     ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
> >>                     ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
> >> -        (((s->gpe.sts[0] & s->gpe.en[0]) & PIIX4_PCI_HOTPLUG_STATUS) != 0);
> >> +        (((s->gpe.sts[0] & s->gpe.en[0]) &
> >> +          (PIIX4_CPU_HOTPLUG_STATUS | PIIX4_PCI_HOTPLUG_STATUS)) != 0);
> >>  
> >>      qemu_set_irq(s->irq, sci_level);
> >>      /* schedule a timer interruption if needed */
> >> -- 
> >> 1.7.7.5
> > 
> > --
> > 			Gleb.
> 
> -- 
> Siemens AG, Corporate Technology, CT T DE IT 1
> Corporate Competence Center Embedded Linux

--
			Gleb.
Jan Kiszka Jan. 9, 2012, 10:19 a.m. UTC | #4
On 2012-01-09 11:07, Gleb Natapov wrote:
> On Mon, Jan 09, 2012 at 11:03:10AM +0100, Jan Kiszka wrote:
>> On 2012-01-09 09:47, Gleb Natapov wrote:
>>> On Sun, Jan 08, 2012 at 10:40:04PM +0100, Igor Mammedov wrote:
>>>>  Change introduced by e71f08bb4a
>>>>     "Fix cpu/pci hotplug to generate level triggered interrupt."
>>>>  was lost somewhre along the way. And as result SCI is not sent in
>>>>  case of cpu hotplug event.
>>>>  Restoring hunk 1 of e71f08bb4a fixes issue.
>>>>
>>> Hmm, I sent similar patch [1] last time someone complained about cpu hotplug
>>> here. Which remind me that in that thread more problem were found in cpu
>>> hotplug. IIRC Jan collected all the patches. Jan, what happened to
>>> them?
>>
>> My patches should have been superseded by the work of Ping Fan on the
>> ICC bus.
>>
> Didn't they fix some problems with bringing new cpu online, not just
> making cpu hotplugable in qdev? May be I misremember.

Let me check... Hmm, yes, there were also some bits required to bring up
the new VCPU thread properly and sync its initial state to the kernel.
That was not part of Ping Fan's ICC patches, also not of the VCPU life
cycle patches.

> BTW do you know
> what happened with ICC bus patches?

Were forgotten to merge? And/or they lacked some acked-by or
reviewed-by. :-/

Jan
Igor Mammedov Jan. 9, 2012, 11:07 a.m. UTC | #5
On 01/09/2012 11:19 AM, Jan Kiszka wrote:
> On 2012-01-09 11:07, Gleb Natapov wrote:
>> On Mon, Jan 09, 2012 at 11:03:10AM +0100, Jan Kiszka wrote:
>>> On 2012-01-09 09:47, Gleb Natapov wrote:
>>>> On Sun, Jan 08, 2012 at 10:40:04PM +0100, Igor Mammedov wrote:
>>>>>   Change introduced by e71f08bb4a
>>>>>      "Fix cpu/pci hotplug to generate level triggered interrupt."
>>>>>   was lost somewhre along the way. And as result SCI is not sent in
>>>>>   case of cpu hotplug event.
>>>>>   Restoring hunk 1 of e71f08bb4a fixes issue.
>>>>>
>>>> Hmm, I sent similar patch [1] last time someone complained about cpu hotplug
>>>> here. Which remind me that in that thread more problem were found in cpu
>>>> hotplug. IIRC Jan collected all the patches. Jan, what happened to
>>>> them?
>>>
>>> My patches should have been superseded by the work of Ping Fan on the
>>> ICC bus.
>>>
>> Didn't they fix some problems with bringing new cpu online, not just
>> making cpu hotplugable in qdev? May be I misremember.
>
> Let me check... Hmm, yes, there were also some bits required to bring up
> the new VCPU thread properly and sync its initial state to the kernel.

CPU bring-up seems to be broken now, guest sees a new cpu but I can't
online it inside guest with error: "CPUx: Not responding". But it
works with old qemu-kvm [3925c857a574].
Jan could you point me to this patches?

> That was not part of Ping Fan's ICC patches, also not of the VCPU life
> cycle patches.

I'm trying to re-base cpu-hotplug to qemu.git, and with ICC patches it's
working like the current qemu-kvm head. But without above mentioned fixes
it seams to be useless.

>
>> BTW do you know
>> what happened with ICC bus patches?
>
> Were forgotten to merge? And/or they lacked some acked-by or
> reviewed-by. :-/
>
> Jan
>
Jan Kiszka Jan. 9, 2012, 11:15 a.m. UTC | #6
On 2012-01-09 12:07, Igor Mammedov wrote:
> On 01/09/2012 11:19 AM, Jan Kiszka wrote:
>> On 2012-01-09 11:07, Gleb Natapov wrote:
>>> On Mon, Jan 09, 2012 at 11:03:10AM +0100, Jan Kiszka wrote:
>>>> On 2012-01-09 09:47, Gleb Natapov wrote:
>>>>> On Sun, Jan 08, 2012 at 10:40:04PM +0100, Igor Mammedov wrote:
>>>>>>   Change introduced by e71f08bb4a
>>>>>>      "Fix cpu/pci hotplug to generate level triggered interrupt."
>>>>>>   was lost somewhre along the way. And as result SCI is not sent in
>>>>>>   case of cpu hotplug event.
>>>>>>   Restoring hunk 1 of e71f08bb4a fixes issue.
>>>>>>
>>>>> Hmm, I sent similar patch [1] last time someone complained about cpu hotplug
>>>>> here. Which remind me that in that thread more problem were found in cpu
>>>>> hotplug. IIRC Jan collected all the patches. Jan, what happened to
>>>>> them?
>>>>
>>>> My patches should have been superseded by the work of Ping Fan on the
>>>> ICC bus.
>>>>
>>> Didn't they fix some problems with bringing new cpu online, not just
>>> making cpu hotplugable in qdev? May be I misremember.
>>
>> Let me check... Hmm, yes, there were also some bits required to bring up
>> the new VCPU thread properly and sync its initial state to the kernel.
> 
> CPU bring-up seems to be broken now, guest sees a new cpu but I can't
> online it inside guest with error: "CPUx: Not responding". But it
> works with old qemu-kvm [3925c857a574].
> Jan could you point me to this patches?

See
http://git.kiszka.org/?p=qemu-kvm.git;a=commitdiff;h=be8f21c6b54eac82f7add7ee9d4ecf9cb8ebb320

Its dependencies have been merged by now. The key is that you need to
synchronize VCPU initialization with the other QEMU threads (so that no
semi-initialized VCPU is visible at any time) and that you perform a
cpu_synchronize_post_init on that VCPU. The hotplug hack is replaced by
the ICC approach.

> 
>> That was not part of Ping Fan's ICC patches, also not of the VCPU life
>> cycle patches.
> 
> I'm trying to re-base cpu-hotplug to qemu.git, and with ICC patches it's
> working like the current qemu-kvm head. But without above mentioned fixes
> it seams to be useless.

Make sure to work against qemu first. Getting things rolling with
qemu-kvm is usually trivial. Working the other way around may create new
forks again.

Jan
diff mbox

Patch

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 1b35707..741a8cc 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -94,7 +94,8 @@  static void pm_update_sci(PIIX4PMState *s)
                    ACPI_BITMASK_POWER_BUTTON_ENABLE |
                    ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                    ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
-        (((s->gpe.sts[0] & s->gpe.en[0]) & PIIX4_PCI_HOTPLUG_STATUS) != 0);
+        (((s->gpe.sts[0] & s->gpe.en[0]) &
+          (PIIX4_CPU_HOTPLUG_STATUS | PIIX4_PCI_HOTPLUG_STATUS)) != 0);
 
     qemu_set_irq(s->irq, sci_level);
     /* schedule a timer interruption if needed */