Re: [Qemu-devel] [PATCH v2 26/74] pc: acpi: memhp: move MHPD._STA method into SSDT

[Igor Mammedov]

On Sun, 20 Dec 2015 15:41:22 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Wed, Dec 16, 2015 at 03:47:35PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > v2:
> >  - add parentheses around ifctx block
> >    Suggested-by: Marcel Apfelbaum <address@hidden>
> > ---
> >  hw/acpi/memory_hotplug_acpi_table.c | 14 ++++++++++++++
> >  hw/i386/acpi-dsdt-mem-hotplug.dsl   |  8 --------
> >  2 files changed, 14 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/acpi/memory_hotplug_acpi_table.c 
> > b/hw/acpi/memory_hotplug_acpi_table.c
> > index 9b61b1c..e1a6551 100644
> > --- a/hw/acpi/memory_hotplug_acpi_table.c
> > +++ b/hw/acpi/memory_hotplug_acpi_table.c
> > @@ -29,11 +29,25 @@ void build_memory_hotplug_aml(Aml *ctx, uint32_t nr_mem,
> >  {
> >      Aml *pci_scope;
> >      Aml *ctrl_dev;
> > +    Aml *method;
> > +    Aml *ifctx;
> > +    Aml *a_zero = aml_int(0);
> > +    Aml *a_slots_nr = aml_name(stringify(MEMORY_SLOTS_NUMBER));
> >  
> >      /* scope for memory hotplug controller device node */
> >      pci_scope = aml_scope("_SB.PCI0");
> >      ctrl_dev = aml_scope(stringify(MEMORY_HOTPLUG_DEVICE));
> >      {
> > +        /* MHPD._STA() method */
> 
> You know something's wrong if you are documenting C using ASL.  In this
> case, rename ctrl_dev to mem_hotplug_dev, move ifctx zero and slots nr
> within {} and drop a_ prefix for zero and slots_nr and code will be
> clear without this comment.
ok, will do it on respin.
> 
> > +        method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> > +        ifctx = aml_if(aml_equal(a_slots_nr, a_zero));
> > +        {
> > +             aml_append(ifctx, aml_return(a_zero));
> > +        }
> > +        aml_append(method, ifctx);
> > +        /* present, functioning, decoding, not shown in UI */
> > +        aml_append(method, aml_return(aml_int(0xB)));
> > +        aml_append(ctrl_dev, method);
> 
> 
> Simple unserialized methods that return value depending on input
> being zero seem very common.  How about a helper for this case?
> 
> E.g.
> 
> aml_method_if_then_else(slots_nr, aml_int(0xB), aml_int(0));
sure

> 
> 
> >      }
> >      aml_append(pci_scope, ctrl_dev);
> >      aml_append(ctx, pci_scope);
> > diff --git a/hw/i386/acpi-dsdt-mem-hotplug.dsl 
> > b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> > index c2bb6a1..b4eacc9 100644
> > --- a/hw/i386/acpi-dsdt-mem-hotplug.dsl
> > +++ b/hw/i386/acpi-dsdt-mem-hotplug.dsl
> > @@ -35,14 +35,6 @@
> >              External(MEMORY_SLOT_OST_EVENT, FieldUnitObj) // _OST event 
> > code, write only
> >              External(MEMORY_SLOT_OST_STATUS, FieldUnitObj) // _OST status 
> > code, write only
> >  
> > -            Method(_STA, 0) {
> > -                If (LEqual(MEMORY_SLOTS_NUMBER, Zero)) {
> > -                    Return(0x0)
> > -                }
> > -                /* present, functioning, decoding, not shown in UI */
> > -                Return(0xB)
> > -            }
> > -
> >              Mutex (MEMORY_SLOT_LOCK, 0)
> >  
> >              Method(MEMORY_SLOT_SCAN_METHOD, 0) {
> > -- 
> > 1.8.3.1