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

2015-12-22 Thread Michael S. Tsirkin 
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"  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.

> 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

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

2015-12-22 Thread Igor Mammedov 
On Tue, 22 Dec 2015 17:11:46 +0200
"Michael S. Tsirkin"  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"  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

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

2015-12-21 Thread Igor Mammedov 
On Sun, 20 Dec 2015 15:41:22 +0200
"Michael S. Tsirkin"  wrote:

> On Wed, Dec 16, 2015 at 03:47:35PM +0100, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov 
> > ---
> > v2:
> >  - add parentheses around ifctx block
> >Suggested-by: Marcel Apfelbaum 
> > ---
> >  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

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

2015-12-21 Thread Igor Mammedov 
On Sun, 20 Dec 2015 15:41:22 +0200
"Michael S. Tsirkin"  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.

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.


> 
> >  }
> >  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

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

2015-12-20 Thread Marcel Apfelbaum 

On 12/16/2015 04:47 PM, Igor Mammedov wrote:

Signed-off-by: Igor Mammedov 
---
v2:
  - add parentheses around ifctx block
Suggested-by: Marcel Apfelbaum 
---
  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 */
+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);
  }
  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) {




Reviewed-by: Marcel Apfelbaum

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

2015-12-20 Thread Michael S. Tsirkin 
On Wed, Dec 16, 2015 at 03:47:35PM +0100, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov 
> ---
> v2:
>  - add parentheses around ifctx block
>Suggested-by: Marcel Apfelbaum 
> ---
>  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.

> +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));


>  }
>  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

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

2015-12-16 Thread Igor Mammedov 
Signed-off-by: Igor Mammedov 
---
v2:
 - add parentheses around ifctx block
   Suggested-by: Marcel Apfelbaum 
---
 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 */
+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);
 }
 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