diff mbox

[16/27] acpi: ich9: allow guest to clear SCI rised by GPE

Message ID 1385001528-12003-17-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Nov. 21, 2013, 2:38 a.m. UTC
it fixes IRQ storm since guest isn't able to lower SCI IRQ
after it has been handled when it clears GPE event.

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

Comments

Michael S. Tsirkin Nov. 21, 2013, 7:14 a.m. UTC | #1
On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> it fixes IRQ storm since guest isn't able to lower SCI IRQ
> after it has been handled when it clears GPE event.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

The storm is only on memory hotplug right?
But memory hotplug support is only added by next
patch.
Maybe just smash with that one?

> ---
>  hw/acpi/ich9.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index e59688b..fb88162 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -61,6 +61,7 @@ static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
>  {
>      ICH9LPCPMRegs *pm = opaque;
>      acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
> +    acpi_update_sci(&pm->acpi_regs, pm->irq, ACPI_MEMORY_HOTPLUG_STATUS);
>  }
>  
>  static const MemoryRegionOps ich9_gpe_ops = {
> -- 
> 1.7.1
Hu Tao Nov. 21, 2013, 8:12 a.m. UTC | #2
On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> > it fixes IRQ storm since guest isn't able to lower SCI IRQ
> > after it has been handled when it clears GPE event.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> The storm is only on memory hotplug right?

IIRC, it happens on cpu hotplug, too.
liguang Nov. 21, 2013, 8:18 a.m. UTC | #3
Hu Tao wrote:
> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
>    
>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
>>      
>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
>>> after it has been handled when it clears GPE event.
>>>
>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>        
>> The storm is only on memory hotplug right?
>>      
> IIRC, it happens on cpu hotplug, too.
>
>
>
>    
:-), that made remember EC implementation,
with EC, SCI will be safer, I think.
Michael S. Tsirkin Nov. 21, 2013, 8:26 a.m. UTC | #4
On Thu, Nov 21, 2013 at 04:12:22PM +0800, Hu Tao wrote:
> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> > > it fixes IRQ storm since guest isn't able to lower SCI IRQ
> > > after it has been handled when it clears GPE event.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > The storm is only on memory hotplug right?
> 
> IIRC, it happens on cpu hotplug, too.

So this is a bugfix then? We should include this in 1.7?
Hu Tao Nov. 21, 2013, 8:28 a.m. UTC | #5
On Thu, Nov 21, 2013 at 10:26:36AM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 21, 2013 at 04:12:22PM +0800, Hu Tao wrote:
> > On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> > > > it fixes IRQ storm since guest isn't able to lower SCI IRQ
> > > > after it has been handled when it clears GPE event.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > The storm is only on memory hotplug right?
> > 
> > IIRC, it happens on cpu hotplug, too.
> 
> So this is a bugfix then? We should include this in 1.7?

Yes. but cpu hotplug support is not added for q35, so the bug doesn't
affect anyone, yet.
Michael S. Tsirkin Nov. 21, 2013, 8:29 a.m. UTC | #6
On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
> Hu Tao wrote:
> >On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> >>On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> >>>it fixes IRQ storm since guest isn't able to lower SCI IRQ
> >>>after it has been handled when it clears GPE event.
> >>>
> >>>Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>The storm is only on memory hotplug right?
> >IIRC, it happens on cpu hotplug, too.
> >
> >
> >
> :-), that made remember EC implementation,
> with EC, SCI will be safer, I think.

Hmm you are saying let's use EC for memory hotplug?
liguang Nov. 21, 2013, 8:32 a.m. UTC | #7
Michael S. Tsirkin wrote:
> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
>    
>> Hu Tao wrote:
>>      
>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
>>>        
>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
>>>>          
>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
>>>>> after it has been handled when it clears GPE event.
>>>>>
>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>>>            
>>>> The storm is only on memory hotplug right?
>>>>          
>>> IIRC, it happens on cpu hotplug, too.
>>>
>>>
>>>
>>>        
>> :-), that made remember EC implementation,
>> with EC, SCI will be safer, I think.
>>      
> Hmm you are saying let's use EC for memory hotplug?
>
>
>    
It can be a bridge between guest and QEMU,
with it, we may don't have to bother ASL writing
and south-bridge hardware related work(or very
little) if we implement EC correctly.
Michael S. Tsirkin Nov. 21, 2013, 9:43 a.m. UTC | #8
On Thu, Nov 21, 2013 at 04:32:27PM +0800, Li Guang wrote:
> Michael S. Tsirkin wrote:
> >On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
> >>Hu Tao wrote:
> >>>On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> >>>>On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> >>>>>it fixes IRQ storm since guest isn't able to lower SCI IRQ
> >>>>>after it has been handled when it clears GPE event.
> >>>>>
> >>>>>Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>>>The storm is only on memory hotplug right?
> >>>IIRC, it happens on cpu hotplug, too.
> >>>
> >>>
> >>>
> >>:-), that made remember EC implementation,
> >>with EC, SCI will be safer, I think.
> >Hmm you are saying let's use EC for memory hotplug?
> >
> >
> It can be a bridge between guest and QEMU,
> with it, we may don't have to bother ASL writing
> and south-bridge hardware related work(or very
> little) if we implement EC correctly.
> 
> 


I'd like to see that. Can you write a document (just text)
for an imaginary EC support for memory hotplug?
Igor Mammedov Nov. 21, 2013, 1:19 p.m. UTC | #9
On Thu, 21 Nov 2013 16:32:27 +0800
Li Guang <lig.fnst@cn.fujitsu.com> wrote:

> Michael S. Tsirkin wrote:
> > On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
> >    
> >> Hu Tao wrote:
> >>      
> >>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> >>>        
> >>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> >>>>          
> >>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
> >>>>> after it has been handled when it clears GPE event.
> >>>>>
> >>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>>>>            
> >>>> The storm is only on memory hotplug right?
> >>>>          
> >>> IIRC, it happens on cpu hotplug, too.
> >>>
> >>>
> >>>
> >>>        
> >> :-), that made remember EC implementation,
> >> with EC, SCI will be safer, I think.
> >>      
> > Hmm you are saying let's use EC for memory hotplug?
> >
> >
> >    
> It can be a bridge between guest and QEMU,
> with it, we may don't have to bother ASL writing
> and south-bridge hardware related work(or very
> little) if we implement EC correctly.
> 
Wouldn't it require guest driver though?
Beauty of ASL/GPE it's that it supported by Windows and Linux
out of box.
Igor Mammedov Nov. 21, 2013, 1:21 p.m. UTC | #10
On Thu, 21 Nov 2013 10:26:36 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Thu, Nov 21, 2013 at 04:12:22PM +0800, Hu Tao wrote:
> > On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> > > > it fixes IRQ storm since guest isn't able to lower SCI IRQ
> > > > after it has been handled when it clears GPE event.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > The storm is only on memory hotplug right?
> > 
> > IIRC, it happens on cpu hotplug, too.
> 
> So this is a bugfix then? We should include this in 1.7?

It's bug fix but, since GPE event's werent' used by by Q35 before
it doesn't affect 1.7.
Igor Mammedov Nov. 21, 2013, 1:31 p.m. UTC | #11
On Thu, 21 Nov 2013 16:28:40 +0800
Hu Tao <hutao@cn.fujitsu.com> wrote:

> On Thu, Nov 21, 2013 at 10:26:36AM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 21, 2013 at 04:12:22PM +0800, Hu Tao wrote:
> > > On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> > > > > it fixes IRQ storm since guest isn't able to lower SCI IRQ
> > > > > after it has been handled when it clears GPE event.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > The storm is only on memory hotplug right?
> > > 
> > > IIRC, it happens on cpu hotplug, too.
> > 
> > So this is a bugfix then? We should include this in 1.7?
> 
> Yes. but cpu hotplug support is not added for q35, so the bug doesn't
> affect anyone, yet.
> 

I was thinking about CPU hotplug for q35 as well, thus patch

1/27 "acpi: factor out common pm_update_sci() into acpi core"

moves common for CPU/Memory hotplug ACPI bits into generic code,
so making CPU hotplug for q35 would require only unifying CPU
hotplug bits and plumbing it in q35 its done for mem hotplug in:

 [PATCH 17/27] acpi: initialize memory hotplug ACPI ICH9 hardware
liguang Nov. 22, 2013, 12:57 a.m. UTC | #12
Michael S. Tsirkin wrote:
> On Thu, Nov 21, 2013 at 04:32:27PM +0800, Li Guang wrote:
>    
>> Michael S. Tsirkin wrote:
>>      
>>> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
>>>        
>>>> Hu Tao wrote:
>>>>          
>>>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
>>>>>            
>>>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
>>>>>>              
>>>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
>>>>>>> after it has been handled when it clears GPE event.
>>>>>>>
>>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>>>>>                
>>>>>> The storm is only on memory hotplug right?
>>>>>>              
>>>>> IIRC, it happens on cpu hotplug, too.
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> :-), that made remember EC implementation,
>>>> with EC, SCI will be safer, I think.
>>>>          
>>> Hmm you are saying let's use EC for memory hotplug?
>>>
>>>
>>>        
>> It can be a bridge between guest and QEMU,
>> with it, we may don't have to bother ASL writing
>> and south-bridge hardware related work(or very
>> little) if we implement EC correctly.
>>
>>
>>      
>
> I'd like to see that. Can you write a document (just text)
> for an imaginary EC support for memory hotplug?
>
>
>
>    

Hmm..., with EC,

For memory hotplug, at least,
ASL at [PATCH 27/27] can be replaced
by a simple Method(_Qx) under EC device,
IO base operations at [PATCH 15/27]
are dispensable,  we can relay data
by standard operations of EC space

and also for SCI, all device changes want to
notify guest OS can share same SCI with EC,
and the operations are specified at ACPI SPEC.

likewise, for CPU hotplug, pvpanic,
and even debugcon.

and, for odd devices, like pvpanic, guest OS may complain
about it, and we may also have to bother on maintaining state of
it at QEMU, and writing a driver for guest OS,
with EC, functions of device like pvpanic may be implemented silently,
and EC is ACPI standard device, each ACPI compatible OS will
have a driver for it natively.
liguang Nov. 22, 2013, 1:03 a.m. UTC | #13
Igor Mammedov wrote:
> On Thu, 21 Nov 2013 16:32:27 +0800
> Li Guang<lig.fnst@cn.fujitsu.com>  wrote:
>
>    
>> Michael S. Tsirkin wrote:
>>      
>>> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
>>>
>>>        
>>>> Hu Tao wrote:
>>>>
>>>>          
>>>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
>>>>>
>>>>>            
>>>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
>>>>>>
>>>>>>              
>>>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
>>>>>>> after it has been handled when it clears GPE event.
>>>>>>>
>>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>>>>>
>>>>>>>                
>>>>>> The storm is only on memory hotplug right?
>>>>>>
>>>>>>              
>>>>> IIRC, it happens on cpu hotplug, too.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> :-), that made remember EC implementation,
>>>> with EC, SCI will be safer, I think.
>>>>
>>>>          
>>> Hmm you are saying let's use EC for memory hotplug?
>>>
>>>
>>>
>>>        
>> It can be a bridge between guest and QEMU,
>> with it, we may don't have to bother ASL writing
>> and south-bridge hardware related work(or very
>> little) if we implement EC correctly.
>>
>>      
> Wouldn't it require guest driver though?
> Beauty of ASL/GPE it's that it supported by Windows and Linux
> out of box.
>
>    

it did require guest driver, but as a ACPI standard device,
the driver is natively implemented by ACPI compatible OS.
Igor Mammedov Nov. 25, 2013, 11:13 a.m. UTC | #14
On Fri, 22 Nov 2013 08:57:40 +0800
Li Guang <lig.fnst@cn.fujitsu.com> wrote:

> Michael S. Tsirkin wrote:
> > On Thu, Nov 21, 2013 at 04:32:27PM +0800, Li Guang wrote:
> >    
> >> Michael S. Tsirkin wrote:
> >>      
> >>> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
> >>>        
> >>>> Hu Tao wrote:
> >>>>          
> >>>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> >>>>>            
> >>>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> >>>>>>              
> >>>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
> >>>>>>> after it has been handled when it clears GPE event.
> >>>>>>>
> >>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>>>>>>                
> >>>>>> The storm is only on memory hotplug right?
> >>>>>>              
> >>>>> IIRC, it happens on cpu hotplug, too.
> >>>>>
> >>>>>
> >>>>>
> >>>>>            
> >>>> :-), that made remember EC implementation,
> >>>> with EC, SCI will be safer, I think.
> >>>>          
> >>> Hmm you are saying let's use EC for memory hotplug?
> >>>
> >>>
> >>>        
> >> It can be a bridge between guest and QEMU,
> >> with it, we may don't have to bother ASL writing
> >> and south-bridge hardware related work(or very
> >> little) if we implement EC correctly.
> >>
> >>
> >>      
> >
> > I'd like to see that. Can you write a document (just text)
> > for an imaginary EC support for memory hotplug?
> >
> >
> >
> >    
> 
> Hmm..., with EC,
> 
> For memory hotplug, at least,
> ASL at [PATCH 27/27] can be replaced
> by a simple Method(_Qx) under EC device,
> IO base operations at [PATCH 15/27]
> are dispensable,  we can relay data
> by standard operations of EC space
> 
> and also for SCI, all device changes want to
> notify guest OS can share same SCI with EC,
> and the operations are specified at ACPI SPEC.
> 
> likewise, for CPU hotplug, pvpanic,
> and even debugcon.
> 
> and, for odd devices, like pvpanic, guest OS may complain
> about it, and we may also have to bother on maintaining state of
> it at QEMU, and writing a driver for guest OS,
> with EC, functions of device like pvpanic may be implemented silently,
> and EC is ACPI standard device, each ACPI compatible OS will
> have a driver for it natively.
> 
From what I remember about them EC was adding essentially another
side-channel but more sophisticated for OSPM communication with
platform but for not benefit so far, since what we need from ACPI
for hotplug could be implemented by using GPE handlers without
adding any EC.

I think there was EC patches on list (perhaps yours) but I couldn't
find them. Could you point me to them if they are demonstrating
how hotplug could be done with EC approach.
liguang Nov. 26, 2013, 12:29 a.m. UTC | #15
Igor Mammedov wrote:
> On Fri, 22 Nov 2013 08:57:40 +0800
> Li Guang<lig.fnst@cn.fujitsu.com>  wrote:
>
>    
>> Michael S. Tsirkin wrote:
>>      
>>> On Thu, Nov 21, 2013 at 04:32:27PM +0800, Li Guang wrote:
>>>
>>>        
>>>> Michael S. Tsirkin wrote:
>>>>
>>>>          
>>>>> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
>>>>>
>>>>>            
>>>>>> Hu Tao wrote:
>>>>>>
>>>>>>              
>>>>>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
>>>>>>>
>>>>>>>                
>>>>>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
>>>>>>>>
>>>>>>>>                  
>>>>>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
>>>>>>>>> after it has been handled when it clears GPE event.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>> The storm is only on memory hotplug right?
>>>>>>>>
>>>>>>>>                  
>>>>>>> IIRC, it happens on cpu hotplug, too.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>> :-), that made remember EC implementation,
>>>>>> with EC, SCI will be safer, I think.
>>>>>>
>>>>>>              
>>>>> Hmm you are saying let's use EC for memory hotplug?
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> It can be a bridge between guest and QEMU,
>>>> with it, we may don't have to bother ASL writing
>>>> and south-bridge hardware related work(or very
>>>> little) if we implement EC correctly.
>>>>
>>>>
>>>>
>>>>          
>>> I'd like to see that. Can you write a document (just text)
>>> for an imaginary EC support for memory hotplug?
>>>
>>>
>>>
>>>
>>>        
>> Hmm..., with EC,
>>
>> For memory hotplug, at least,
>> ASL at [PATCH 27/27] can be replaced
>> by a simple Method(_Qx) under EC device,
>> IO base operations at [PATCH 15/27]
>> are dispensable,  we can relay data
>> by standard operations of EC space
>>
>> and also for SCI, all device changes want to
>> notify guest OS can share same SCI with EC,
>> and the operations are specified at ACPI SPEC.
>>
>> likewise, for CPU hotplug, pvpanic,
>> and even debugcon.
>>
>> and, for odd devices, like pvpanic, guest OS may complain
>> about it, and we may also have to bother on maintaining state of
>> it at QEMU, and writing a driver for guest OS,
>> with EC, functions of device like pvpanic may be implemented silently,
>> and EC is ACPI standard device, each ACPI compatible OS will
>> have a driver for it natively.
>>
>>      
>  From what I remember about them EC was adding essentially another
> side-channel but more sophisticated for OSPM communication with
> platform but for not benefit so far, since what we need from ACPI
> for hotplug could be implemented by using GPE handlers without
> adding any EC.
>
> I think there was EC patches on list (perhaps yours) but I couldn't
> find them. Could you point me to them if they are demonstrating
> how hotplug could be done with EC approach.
>
>
>
>    
you can find my previous raw patch-set here,
http://lists.gnu.org/archive/html/qemu-devel/2013-05/msg02845.html

Thanks!
Li Guang
Igor Mammedov Nov. 26, 2013, 10:36 p.m. UTC | #16
On Tue, 26 Nov 2013 08:29:27 +0800
Li Guang <lig.fnst@cn.fujitsu.com> wrote:

> Igor Mammedov wrote:
> > On Fri, 22 Nov 2013 08:57:40 +0800
> > Li Guang<lig.fnst@cn.fujitsu.com>  wrote:
> >
> >    
> >> Michael S. Tsirkin wrote:
> >>      
> >>> On Thu, Nov 21, 2013 at 04:32:27PM +0800, Li Guang wrote:
> >>>
> >>>        
> >>>> Michael S. Tsirkin wrote:
> >>>>
> >>>>          
> >>>>> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
> >>>>>
> >>>>>            
> >>>>>> Hu Tao wrote:
> >>>>>>
> >>>>>>              
> >>>>>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> >>>>>>>
> >>>>>>>                
> >>>>>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> >>>>>>>>
> >>>>>>>>                  
> >>>>>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
> >>>>>>>>> after it has been handled when it clears GPE event.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>>>>>>>>
> >>>>>>>>>                    
> >>>>>>>> The storm is only on memory hotplug right?
> >>>>>>>>
> >>>>>>>>                  
> >>>>>>> IIRC, it happens on cpu hotplug, too.
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>                
> >>>>>> :-), that made remember EC implementation,
> >>>>>> with EC, SCI will be safer, I think.
> >>>>>>
> >>>>>>              
> >>>>> Hmm you are saying let's use EC for memory hotplug?
> >>>>>
> >>>>>
> >>>>>
> >>>>>            
> >>>> It can be a bridge between guest and QEMU,
> >>>> with it, we may don't have to bother ASL writing
> >>>> and south-bridge hardware related work(or very
> >>>> little) if we implement EC correctly.
> >>>>
> >>>>
> >>>>
> >>>>          
> >>> I'd like to see that. Can you write a document (just text)
> >>> for an imaginary EC support for memory hotplug?
> >>>
> >>>
> >>>
> >>>
> >>>        
> >> Hmm..., with EC,
> >>
> >> For memory hotplug, at least,
> >> ASL at [PATCH 27/27] can be replaced
> >> by a simple Method(_Qx) under EC device,
> >> IO base operations at [PATCH 15/27]
> >> are dispensable,  we can relay data
> >> by standard operations of EC space
> >>
> >> and also for SCI, all device changes want to
> >> notify guest OS can share same SCI with EC,
> >> and the operations are specified at ACPI SPEC.
> >>
> >> likewise, for CPU hotplug, pvpanic,
> >> and even debugcon.
> >>
> >> and, for odd devices, like pvpanic, guest OS may complain
> >> about it, and we may also have to bother on maintaining state of
> >> it at QEMU, and writing a driver for guest OS,
> >> with EC, functions of device like pvpanic may be implemented silently,
> >> and EC is ACPI standard device, each ACPI compatible OS will
> >> have a driver for it natively.
> >>
> >>      
> >  From what I remember about them EC was adding essentially another
> > side-channel but more sophisticated for OSPM communication with
> > platform but for not benefit so far, since what we need from ACPI
> > for hotplug could be implemented by using GPE handlers without
> > adding any EC.
> >
> > I think there was EC patches on list (perhaps yours) but I couldn't
> > find them. Could you point me to them if they are demonstrating
> > how hotplug could be done with EC approach.
> >
> >
> >
> >    
> you can find my previous raw patch-set here,
> http://lists.gnu.org/archive/html/qemu-devel/2013-05/msg02845.html
There you are trying to overcome linux kernel limitation with help of EC
AND additional guest driver to online CPU.
Memory hotplug essentially has the same issue, UDEV is responsible for
onlining hot added ranges. So it's upto userspace policy whether to do
it automatically or not.

But even discarding qemu specific kernel driver, it boils down to using
_Qxx handler vs _Exx one with basically the same ASL code.

So question becomes: Why using EC would be better than using already
present GPE registers for handling event?

> Thanks!
> Li Guang
>
liguang Nov. 27, 2013, 12:15 a.m. UTC | #17
Igor Mammedov wrote:
> On Tue, 26 Nov 2013 08:29:27 +0800
> Li Guang<lig.fnst@cn.fujitsu.com>  wrote:
>
>    
>> Igor Mammedov wrote:
>>      
>>> On Fri, 22 Nov 2013 08:57:40 +0800
>>> Li Guang<lig.fnst@cn.fujitsu.com>   wrote:
>>>
>>>
>>>        
>>>> Michael S. Tsirkin wrote:
>>>>
>>>>          
>>>>> On Thu, Nov 21, 2013 at 04:32:27PM +0800, Li Guang wrote:
>>>>>
>>>>>
>>>>>            
>>>>>> Michael S. Tsirkin wrote:
>>>>>>
>>>>>>
>>>>>>              
>>>>>>> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>>>> Hu Tao wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>                  
>>>>>>>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>                      
>>>>>>>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
>>>>>>>>>>> after it has been handled when it clears GPE event.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>                        
>>>>>>>>>> The storm is only on memory hotplug right?
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>                      
>>>>>>>>> IIRC, it happens on cpu hotplug, too.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>> :-), that made remember EC implementation,
>>>>>>>> with EC, SCI will be safer, I think.
>>>>>>>>
>>>>>>>>
>>>>>>>>                  
>>>>>>> Hmm you are saying let's use EC for memory hotplug?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>> It can be a bridge between guest and QEMU,
>>>>>> with it, we may don't have to bother ASL writing
>>>>>> and south-bridge hardware related work(or very
>>>>>> little) if we implement EC correctly.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>              
>>>>> I'd like to see that. Can you write a document (just text)
>>>>> for an imaginary EC support for memory hotplug?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> Hmm..., with EC,
>>>>
>>>> For memory hotplug, at least,
>>>> ASL at [PATCH 27/27] can be replaced
>>>> by a simple Method(_Qx) under EC device,
>>>> IO base operations at [PATCH 15/27]
>>>> are dispensable,  we can relay data
>>>> by standard operations of EC space
>>>>
>>>> and also for SCI, all device changes want to
>>>> notify guest OS can share same SCI with EC,
>>>> and the operations are specified at ACPI SPEC.
>>>>
>>>> likewise, for CPU hotplug, pvpanic,
>>>> and even debugcon.
>>>>
>>>> and, for odd devices, like pvpanic, guest OS may complain
>>>> about it, and we may also have to bother on maintaining state of
>>>> it at QEMU, and writing a driver for guest OS,
>>>> with EC, functions of device like pvpanic may be implemented silently,
>>>> and EC is ACPI standard device, each ACPI compatible OS will
>>>> have a driver for it natively.
>>>>
>>>>
>>>>          
>>>    From what I remember about them EC was adding essentially another
>>> side-channel but more sophisticated for OSPM communication with
>>> platform but for not benefit so far, since what we need from ACPI
>>> for hotplug could be implemented by using GPE handlers without
>>> adding any EC.
>>>
>>> I think there was EC patches on list (perhaps yours) but I couldn't
>>> find them. Could you point me to them if they are demonstrating
>>> how hotplug could be done with EC approach.
>>>
>>>
>>>
>>>
>>>        
>> you can find my previous raw patch-set here,
>> http://lists.gnu.org/archive/html/qemu-devel/2013-05/msg02845.html
>>      
> There you are trying to overcome linux kernel limitation with help of EC
> AND additional guest driver to online CPU.
> Memory hotplug essentially has the same issue, UDEV is responsible for
> onlining hot added ranges. So it's upto userspace policy whether to do
> it automatically or not.
>    

really?
AFAIK, all hotplug-able  memory can be described at SRAT,
and you can plug and unplug, just need a GPE.

> But even discarding qemu specific kernel driver, it boils down to using
> _Qxx handler vs _Exx one with basically the same ASL code.
>
> So question becomes: Why using EC would be better than using already
> present GPE registers for handling event?
>
>    
1. we didn't need to bother IO memory operations,
      because we relay data via EC
2. we didn't need to bother GPE handling,
      because EC can do it for us
3. for extension, if need like pvpanic device, can be satisfied
     by EC operations easily
Igor Mammedov Nov. 27, 2013, 12:57 a.m. UTC | #18
On Wed, 27 Nov 2013 08:15:31 +0800
Li Guang <lig.fnst@cn.fujitsu.com> wrote:

> Igor Mammedov wrote:
> > On Tue, 26 Nov 2013 08:29:27 +0800
> > Li Guang<lig.fnst@cn.fujitsu.com>  wrote:
> >
> >    
> >> Igor Mammedov wrote:
> >>      
> >>> On Fri, 22 Nov 2013 08:57:40 +0800
> >>> Li Guang<lig.fnst@cn.fujitsu.com>   wrote:
> >>>
> >>>
> >>>        
> >>>> Michael S. Tsirkin wrote:
> >>>>
> >>>>          
> >>>>> On Thu, Nov 21, 2013 at 04:32:27PM +0800, Li Guang wrote:
> >>>>>
> >>>>>
> >>>>>            
> >>>>>> Michael S. Tsirkin wrote:
> >>>>>>
> >>>>>>
> >>>>>>              
> >>>>>>> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>>                
> >>>>>>>> Hu Tao wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>                  
> >>>>>>>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>                    
> >>>>>>>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>                      
> >>>>>>>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
> >>>>>>>>>>> after it has been handled when it clears GPE event.
> >>>>>>>>>>>
> >>>>>>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>>                        
> >>>>>>>>>> The storm is only on memory hotplug right?
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>                      
> >>>>>>>>> IIRC, it happens on cpu hotplug, too.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>>                    
> >>>>>>>> :-), that made remember EC implementation,
> >>>>>>>> with EC, SCI will be safer, I think.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>                  
> >>>>>>> Hmm you are saying let's use EC for memory hotplug?
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>                
> >>>>>> It can be a bridge between guest and QEMU,
> >>>>>> with it, we may don't have to bother ASL writing
> >>>>>> and south-bridge hardware related work(or very
> >>>>>> little) if we implement EC correctly.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>              
> >>>>> I'd like to see that. Can you write a document (just text)
> >>>>> for an imaginary EC support for memory hotplug?
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>
> >>>>>            
> >>>> Hmm..., with EC,
> >>>>
> >>>> For memory hotplug, at least,
> >>>> ASL at [PATCH 27/27] can be replaced
> >>>> by a simple Method(_Qx) under EC device,
> >>>> IO base operations at [PATCH 15/27]
> >>>> are dispensable,  we can relay data
> >>>> by standard operations of EC space
> >>>>
> >>>> and also for SCI, all device changes want to
> >>>> notify guest OS can share same SCI with EC,
> >>>> and the operations are specified at ACPI SPEC.
> >>>>
> >>>> likewise, for CPU hotplug, pvpanic,
> >>>> and even debugcon.
> >>>>
> >>>> and, for odd devices, like pvpanic, guest OS may complain
> >>>> about it, and we may also have to bother on maintaining state of
> >>>> it at QEMU, and writing a driver for guest OS,
> >>>> with EC, functions of device like pvpanic may be implemented silently,
> >>>> and EC is ACPI standard device, each ACPI compatible OS will
> >>>> have a driver for it natively.
> >>>>
> >>>>
> >>>>          
> >>>    From what I remember about them EC was adding essentially another
> >>> side-channel but more sophisticated for OSPM communication with
> >>> platform but for not benefit so far, since what we need from ACPI
> >>> for hotplug could be implemented by using GPE handlers without
> >>> adding any EC.
> >>>
> >>> I think there was EC patches on list (perhaps yours) but I couldn't
> >>> find them. Could you point me to them if they are demonstrating
> >>> how hotplug could be done with EC approach.
> >>>
> >>>
> >>>
> >>>
> >>>        
> >> you can find my previous raw patch-set here,
> >> http://lists.gnu.org/archive/html/qemu-devel/2013-05/msg02845.html
> >>      
> > There you are trying to overcome linux kernel limitation with help of EC
> > AND additional guest driver to online CPU.
> > Memory hotplug essentially has the same issue, UDEV is responsible for
> > onlining hot added ranges. So it's upto userspace policy whether to do
> > it automatically or not.
> >    
> 
> really?
> AFAIK, all hotplug-able  memory can be described at SRAT,
> and you can plug and unplug, just need a GPE.
Just try it :)
kernel creates entries for hotplugged memory but userspace has to online
it manually, issue doesn't have any relation to SRAT table.

> > But even discarding qemu specific kernel driver, it boils down to using
> > _Qxx handler vs _Exx one with basically the same ASL code.
> >
> > So question becomes: Why using EC would be better than using already
> > present GPE registers for handling event?
> >
> >    
> 1. we didn't need to bother IO memory operations,
>       because we relay data via EC
I guess we will have to write/read EC's IO memory instead, serializing data
into byte stream [as in proposed earlier impl.], which will complicate
ACPI interface part of memory hotplug. On QEMU side reader/writer handler might
look different but will implement the same logic as now, EC will not implement
it magically for us.

> 2. we didn't need to bother GPE handling,
>       because EC can do it for us
It will do EC handling instead, aren't it?

Looks like discussion turned into just arguing.
You have on hand this series, would you demonstrate with patches that EC
allows to implement series simpler/better so we could evaluate alternative?

> 3. for extension, if need like pvpanic device, can be satisfied
>      by EC operations easily
> 
>
liguang Nov. 27, 2013, 1:48 a.m. UTC | #19
Igor Mammedov wrote:
> On Wed, 27 Nov 2013 08:15:31 +0800
> Li Guang<lig.fnst@cn.fujitsu.com>  wrote:
>
>    
>> Igor Mammedov wrote:
>>      
>>> On Tue, 26 Nov 2013 08:29:27 +0800
>>> Li Guang<lig.fnst@cn.fujitsu.com>   wrote:
>>>
>>>
>>>        
>>>> Igor Mammedov wrote:
>>>>
>>>>          
>>>>> On Fri, 22 Nov 2013 08:57:40 +0800
>>>>> Li Guang<lig.fnst@cn.fujitsu.com>    wrote:
>>>>>
>>>>>
>>>>>
>>>>>            
>>>>>> Michael S. Tsirkin wrote:
>>>>>>
>>>>>>
>>>>>>              
>>>>>>> On Thu, Nov 21, 2013 at 04:32:27PM +0800, Li Guang wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>>>> Michael S. Tsirkin wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                  
>>>>>>>>> On Thu, Nov 21, 2013 at 04:18:45PM +0800, Li Guang wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>>>> Hu Tao wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>                      
>>>>>>>>>>> On Thu, Nov 21, 2013 at 09:14:18AM +0200, Michael S. Tsirkin wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>                        
>>>>>>>>>>>> On Thu, Nov 21, 2013 at 03:38:37AM +0100, Igor Mammedov wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>                          
>>>>>>>>>>>>> it fixes IRQ storm since guest isn't able to lower SCI IRQ
>>>>>>>>>>>>> after it has been handled when it clears GPE event.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Igor Mammedov<imammedo@redhat.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>                            
>>>>>>>>>>>> The storm is only on memory hotplug right?
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>                          
>>>>>>>>>>> IIRC, it happens on cpu hotplug, too.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>                        
>>>>>>>>>> :-), that made remember EC implementation,
>>>>>>>>>> with EC, SCI will be safer, I think.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>                      
>>>>>>>>> Hmm you are saying let's use EC for memory hotplug?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>                    
>>>>>>>> It can be a bridge between guest and QEMU,
>>>>>>>> with it, we may don't have to bother ASL writing
>>>>>>>> and south-bridge hardware related work(or very
>>>>>>>> little) if we implement EC correctly.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>                  
>>>>>>> I'd like to see that. Can you write a document (just text)
>>>>>>> for an imaginary EC support for memory hotplug?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>                
>>>>>> Hmm..., with EC,
>>>>>>
>>>>>> For memory hotplug, at least,
>>>>>> ASL at [PATCH 27/27] can be replaced
>>>>>> by a simple Method(_Qx) under EC device,
>>>>>> IO base operations at [PATCH 15/27]
>>>>>> are dispensable,  we can relay data
>>>>>> by standard operations of EC space
>>>>>>
>>>>>> and also for SCI, all device changes want to
>>>>>> notify guest OS can share same SCI with EC,
>>>>>> and the operations are specified at ACPI SPEC.
>>>>>>
>>>>>> likewise, for CPU hotplug, pvpanic,
>>>>>> and even debugcon.
>>>>>>
>>>>>> and, for odd devices, like pvpanic, guest OS may complain
>>>>>> about it, and we may also have to bother on maintaining state of
>>>>>> it at QEMU, and writing a driver for guest OS,
>>>>>> with EC, functions of device like pvpanic may be implemented silently,
>>>>>> and EC is ACPI standard device, each ACPI compatible OS will
>>>>>> have a driver for it natively.
>>>>>>
>>>>>>
>>>>>>
>>>>>>              
>>>>>      From what I remember about them EC was adding essentially another
>>>>> side-channel but more sophisticated for OSPM communication with
>>>>> platform but for not benefit so far, since what we need from ACPI
>>>>> for hotplug could be implemented by using GPE handlers without
>>>>> adding any EC.
>>>>>
>>>>> I think there was EC patches on list (perhaps yours) but I couldn't
>>>>> find them. Could you point me to them if they are demonstrating
>>>>> how hotplug could be done with EC approach.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>            
>>>> you can find my previous raw patch-set here,
>>>> http://lists.gnu.org/archive/html/qemu-devel/2013-05/msg02845.html
>>>>
>>>>          
>>> There you are trying to overcome linux kernel limitation with help of EC
>>> AND additional guest driver to online CPU.
>>> Memory hotplug essentially has the same issue, UDEV is responsible for
>>> onlining hot added ranges. So it's upto userspace policy whether to do
>>> it automatically or not.
>>>
>>>        
>> really?
>> AFAIK, all hotplug-able  memory can be described at SRAT,
>> and you can plug and unplug, just need a GPE.
>>      
> Just try it :)
> kernel creates entries for hotplugged memory but userspace has to online
> it manually, issue doesn't have any relation to SRAT table.
>
>    
>>> But even discarding qemu specific kernel driver, it boils down to using
>>> _Qxx handler vs _Exx one with basically the same ASL code.
>>>
>>> So question becomes: Why using EC would be better than using already
>>> present GPE registers for handling event?
>>>
>>>
>>>        
>> 1. we didn't need to bother IO memory operations,
>>        because we relay data via EC
>>      
> I guess we will have to write/read EC's IO memory instead,

the premise is supposed EC is implemented already.

> serializing data
> into byte stream [as in proposed earlier impl.], which will complicate
> ACPI interface part of memory hotplug. On QEMU side reader/writer handler might
> look different but will implement the same logic as now, EC will not implement
> it magically for us.
>
>    

Yes, of course, but, as you can see,  info relay will turn into standard
EC r/w operations.

>> 2. we didn't need to bother GPE handling,
>>        because EC can do it for us
>>      
> It will do EC handling instead, aren't it?
>    
Yes.
> Looks like discussion turned into just arguing.
>    

:-),  I just answer your questions.

> You have on hand this series, would you demonstrate with patches that EC
> allows to implement series simpler/better so we could evaluate alternative?
>
>    

I am never going say EC is deemed to be simpler or better,
for me,  it's just flexible.

The mainly approach be demoed at my previous patch-set
for cpu-hotplug.

Thanks!
Li Guang
>> 3. for extension, if need like pvpanic device, can be satisfied
>>       by EC operations easily
>>
>>
>>      
>
>
Paolo Bonzini Nov. 27, 2013, 9:43 a.m. UTC | #20
Il 27/11/2013 02:48, Li Guang ha scritto:
> I am never going say EC is deemed to be simpler or better,
> for me,  it's just flexible.

I think it's entirely the same.  Instead of the I/O address space you
would use EC OperationRegions, instead of _Exx you would use _Qxx.

If we had an embedded controller device model perhaps it would make
sense to use it for all of PCI hotplug, CPU hotplug, memory hotplug.  We
don't, hence it makes sense to use I/O and GPE for all three of them.

Paolo
diff mbox

Patch

diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index e59688b..fb88162 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -61,6 +61,7 @@  static void ich9_gpe_writeb(void *opaque, hwaddr addr, uint64_t val,
 {
     ICH9LPCPMRegs *pm = opaque;
     acpi_gpe_ioport_writeb(&pm->acpi_regs, addr, val);
+    acpi_update_sci(&pm->acpi_regs, pm->irq, ACPI_MEMORY_HOTPLUG_STATUS);
 }
 
 static const MemoryRegionOps ich9_gpe_ops = {