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

[Igor Mammedov]

On Tue, 22 Dec 2015 17:11:46 +0200
"Michael S. Tsirkin" <address@hidden> wrote:

> On Mon, Dec 21, 2015 at 06:31:40PM +0100, Igor Mammedov wrote:
> > 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:
> > [...]
> > > > +        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));
> > I've looked through all _STA methods that exist in DSDT
> > and though this method for mem hotplug doesn't belong to common pattern,
> > I could use you suggestion for 4 ISA devices.
> > 
> > Or perhaps I can save 2-3 LOC by making common
> >   build_if_else(cond, truectx, false_ctx);
> > I'll try both but it looks like build_if_else() will be more useful.
> 
> Yes but the issue is it makes code look weird since
> you need to prepare the context ahead of the time.
I was thinking using it only for simple true/falsectx where it
could be inlined as argument, like:
   aml_append(method,
       build_if_else(aml_equal(slots_nr, zero),
                     aml_return(zero), aml_return(aml_int(0xB))));

maybe save this idea for after this series to see what is left after
cleanups and if it's worth a trouble.
                 
> 
> > And after this series is applied there is a plenty to optimize,
> > for example most of these _STA methods will be gone or replaced
> > by simple _STA = FOO expression.
> 
> OK.
> 
> > 
> > > 
> > > >      }
> > > >      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