[Qemu-devel] [PATCH 0/9] pc: acpi: various fixes and cleanups

[Qemu-devel] [PATCH 0/9] pc: acpi: various fixes and cleanups

[Igor Mammedov]

this series is an early attempt to shave off a bunch of
not directly related patches from already big dynamic
AML series (although it's dependency for it)

main target of this series is:

pc: acpi-build: replace recursive PCI bus tree generation with loop
    based

A simplified PCI description generation, which replaces tree
structure with a set of ACPI scopes and allows later easily replace
template patching with direct AML generation with further
simplification.

Tested: on XPsp3 to WS2012R2 and REHL6/7 guests.

Git tree for testing:
 https://github.com/imammedo/qemu/commits/acpi_pci_gen_simplification


Igor Mammedov (9):
  pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled
  pc: acpi: decribe bridge device as not hotpluggable
  pc: acpi-build: cleanup AcpiPmInfo initialization
  acpi: build_append_nameseg(): add padding if necessary
  acpi: move generic aml building helpers into dedictated file
  acpi: add build_append_namestring() helper
  acpi: replace opencoded notify codes with named values
  acpi: drop min-bytes in build_package()
  pc: acpi-build: replace recursive PCI bus tree generation with loop
    based

 hw/acpi/Makefile.objs             |   1 +
 hw/acpi/acpi_gen_utils.c          | 247 +++++++++++++++++
 hw/i386/acpi-build.c              | 561 +++++++++++++-------------------------
 hw/i386/acpi-dsdt-cpu-hotplug.dsl |   1 +
 include/hw/acpi/acpi_gen_utils.h  |  29 ++
 5 files changed, 462 insertions(+), 377 deletions(-)
 create mode 100644 hw/acpi/acpi_gen_utils.c
 create mode 100644 include/hw/acpi/acpi_gen_utils.h

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/9] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled

[Igor Mammedov]

ACPI parser in XP considers PNP0A06 devices of CPU and
memory hotplug as duplicates. Adding unique _UID
to CPU hotplug device fixes BSOD.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-dsdt-cpu-hotplug.dsl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
index 34aab5a..268d870 100644
--- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
@@ -94,6 +94,7 @@ Scope(\_SB) {
 
     Device(CPU_HOTPLUG_RESOURCE_DEVICE) {
         Name(_HID, EisaId("PNP0A06"))
+        Name(_UID, "CPU hotplug resources")
 
         Name(_CRS, ResourceTemplate() {
             IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, CPU_STATUS_LEN)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/9] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:00PM +0000, Igor Mammedov wrote:
> ACPI parser in XP considers PNP0A06 devices of CPU and
> memory hotplug as duplicates. Adding unique _UID
> to CPU hotplug device fixes BSOD.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

And let's add them for memory hotplug as well?
Also, if we do stable branch release, we probably
want to only do it for memory hotplug in a separate
patch, right?
This way users who don't enable memory hotplug
are unaffected, reduces risk slightly.

> ---
>  hw/i386/acpi-dsdt-cpu-hotplug.dsl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> index 34aab5a..268d870 100644
> --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> @@ -94,6 +94,7 @@ Scope(\_SB) {
>  
>      Device(CPU_HOTPLUG_RESOURCE_DEVICE) {
>          Name(_HID, EisaId("PNP0A06"))
> +        Name(_UID, "CPU hotplug resources")
>  
>          Name(_CRS, ResourceTemplate() {
>              IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, CPU_STATUS_LEN)
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 1/9] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled

[Igor Mammedov]

On Mon, 8 Dec 2014 22:57:05 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 08, 2014 at 04:08:00PM +0000, Igor Mammedov wrote:
> > ACPI parser in XP considers PNP0A06 devices of CPU and
> > memory hotplug as duplicates. Adding unique _UID
> > to CPU hotplug device fixes BSOD.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> And let's add them for memory hotplug as well?
XP doesn't support it.

> Also, if we do stable branch release, we probably
> want to only do it for memory hotplug in a separate
> patch, right?
> This way users who don't enable memory hotplug
> are unaffected, reduces risk slightly.
Memory hotplug device already has _UID, so it doesn't need patching.
This patch just fixes BSOD if QEMU has been started with
hotplug enabled i.e. for example -m 2G,slots=2,maxmem=4G,
and prevents clashing between memory hotplug and
cpu hotplug devices on XP (i.e. XP specific quirk).

I've tested it with XPsp3 and all later version upto WS2012R2.


> 
> > ---
> >  hw/i386/acpi-dsdt-cpu-hotplug.dsl | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > index 34aab5a..268d870 100644
> > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > @@ -94,6 +94,7 @@ Scope(\_SB) {
> >  
> >      Device(CPU_HOTPLUG_RESOURCE_DEVICE) {
> >          Name(_HID, EisaId("PNP0A06"))
> > +        Name(_UID, "CPU hotplug resources")
> >  
> >          Name(_CRS, ResourceTemplate() {
> >              IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, CPU_STATUS_LEN)
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH 1/9] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 11:05:37AM +0100, Igor Mammedov wrote:
> On Mon, 8 Dec 2014 22:57:05 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Dec 08, 2014 at 04:08:00PM +0000, Igor Mammedov wrote:
> > > ACPI parser in XP considers PNP0A06 devices of CPU and
> > > memory hotplug as duplicates. Adding unique _UID
> > > to CPU hotplug device fixes BSOD.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > And let's add them for memory hotplug as well?
> XP doesn't support it.
> > Also, if we do stable branch release, we probably
> > want to only do it for memory hotplug in a separate
> > patch, right?
> > This way users who don't enable memory hotplug
> > are unaffected, reduces risk slightly.
> Memory hotplug device already has _UID, so it doesn't need patching.
> This patch just fixes BSOD if QEMU has been started with
> hotplug enabled i.e. for example -m 2G,slots=2,maxmem=4G,
> and prevents clashing between memory hotplug and
> cpu hotplug devices on XP (i.e. XP specific quirk).
> 
> I've tested it with XPsp3 and all later version upto WS2012R2.


I see, I misunderstood.
Thanks, I'll apply this one.

> 
> > 
> > > ---
> > >  hw/i386/acpi-dsdt-cpu-hotplug.dsl | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > index 34aab5a..268d870 100644
> > > --- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > +++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
> > > @@ -94,6 +94,7 @@ Scope(\_SB) {
> > >  
> > >      Device(CPU_HOTPLUG_RESOURCE_DEVICE) {
> > >          Name(_HID, EisaId("PNP0A06"))
> > > +        Name(_UID, "CPU hotplug resources")
> > >  
> > >          Name(_CRS, ResourceTemplate() {
> > >              IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, CPU_STATUS_LEN)
> > > -- 
> > > 1.8.3.1

[Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Igor Mammedov]

when bridge hotplug is disabled, i.e. for machine
types less then 2.0, bridge device was created as
hotpluggable by mistake since commit 133a2da.

Fix it by just creating it as a present device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b37a397..1fb92e5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             }
         }
 
-        if (!dc->hotpluggable || bridge_in_acpi) {
+        if (!dc->hotpluggable || pc->is_bridge) {
             clear_bit(slot, slot_hotplug_enable);
         }
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:01PM +0000, Igor Mammedov wrote:
> when bridge hotplug is disabled, i.e. for machine
> types less then 2.0, bridge device was created as
> hotpluggable by mistake since commit 133a2da.
> 
> Fix it by just creating it as a present device.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

What exactly is the problem here?
It seems that such bridge is hotpluggable, even though
e.g. windows guests lacks drivers to support this.


> ---
>  hw/i386/acpi-build.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b37a397..1fb92e5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>              }
>          }
>  
> -        if (!dc->hotpluggable || bridge_in_acpi) {
> +        if (!dc->hotpluggable || pc->is_bridge) {
>              clear_bit(slot, slot_hotplug_enable);
>          }
>      }
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Igor Mammedov]

On Mon, 8 Dec 2014 21:13:25 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 08, 2014 at 04:08:01PM +0000, Igor Mammedov wrote:
> > when bridge hotplug is disabled, i.e. for machine
> > types less then 2.0, bridge device was created as
> > hotpluggable by mistake since commit 133a2da.
> > 
> > Fix it by just creating it as a present device.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> What exactly is the problem here?
> It seems that such bridge is hotpluggable, even though
> e.g. windows guests lacks drivers to support this.
before 133a2da slot with existing at startup bridge weren't
marked as hotpluggable nor described in SSDT. But after
133a2da it's described as hotpluggable slot for compat
machines (2.0 and lower) which doesn't match with original
behavior.

Also Marcel mentioned that bridges could be hotpluggable
but that they should not be hot-UNpluggable.

> 
> 
> > ---
> >  hw/i386/acpi-build.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index b37a397..1fb92e5 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >              }
> >          }
> >  
> > -        if (!dc->hotpluggable || bridge_in_acpi) {
> > +        if (!dc->hotpluggable || pc->is_bridge) {
> >              clear_bit(slot, slot_hotplug_enable);
> >          }
> >      }
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 11:27:16AM +0100, Igor Mammedov wrote:
> On Mon, 8 Dec 2014 21:13:25 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Dec 08, 2014 at 04:08:01PM +0000, Igor Mammedov wrote:
> > > when bridge hotplug is disabled, i.e. for machine
> > > types less then 2.0, bridge device was created as
> > > hotpluggable by mistake since commit 133a2da.
> > > 
> > > Fix it by just creating it as a present device.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > What exactly is the problem here?
> > It seems that such bridge is hotpluggable, even though
> > e.g. windows guests lacks drivers to support this.
> before 133a2da slot with existing at startup bridge weren't
> marked as hotpluggable nor described in SSDT. But after
> 133a2da it's described as hotpluggable slot for compat
> machines (2.0 and lower) which doesn't match with original
> behavior.
> 
> Also Marcel mentioned that bridges could be hotpluggable
> but that they should not be hot-UNpluggable.

OK so is there some guest that's confused?
What's the bug?

> > 
> > 
> > > ---
> > >  hw/i386/acpi-build.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index b37a397..1fb92e5 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >              }
> > >          }
> > >  
> > > -        if (!dc->hotpluggable || bridge_in_acpi) {
> > > +        if (!dc->hotpluggable || pc->is_bridge) {
> > >              clear_bit(slot, slot_hotplug_enable);
> > >          }
> > >      }
> > > -- 
> > > 1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Igor Mammedov]

On Tue, 9 Dec 2014 12:34:02 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 09, 2014 at 11:27:16AM +0100, Igor Mammedov wrote:
> > On Mon, 8 Dec 2014 21:13:25 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Dec 08, 2014 at 04:08:01PM +0000, Igor Mammedov wrote:
> > > > when bridge hotplug is disabled, i.e. for machine
> > > > types less then 2.0, bridge device was created as
> > > > hotpluggable by mistake since commit 133a2da.
> > > > 
> > > > Fix it by just creating it as a present device.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > What exactly is the problem here?
> > > It seems that such bridge is hotpluggable, even though
> > > e.g. windows guests lacks drivers to support this.
> > before 133a2da slot with existing at startup bridge weren't
> > marked as hotpluggable nor described in SSDT. But after
> > 133a2da it's described as hotpluggable slot for compat
> > machines (2.0 and lower) which doesn't match with original
> > behavior.
> > 
> > Also Marcel mentioned that bridges could be hotpluggable
> > but that they should not be hot-UNpluggable.
> 
> OK so is there some guest that's confused?
> What's the bug?
It allows to do eject on bridge when it shouldn't.
I don't know about consequences of it,
it's better not to allow invalid action in the first place.




> 
> > > 
> > > 
> > > > ---
> > > >  hw/i386/acpi-build.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index b37a397..1fb92e5 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > >              }
> > > >          }
> > > >  
> > > > -        if (!dc->hotpluggable || bridge_in_acpi) {
> > > > +        if (!dc->hotpluggable || pc->is_bridge) {
> > > >              clear_bit(slot, slot_hotplug_enable);
> > > >          }
> > > >      }
> > > > -- 
> > > > 1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 12:45:27PM +0100, Igor Mammedov wrote:
> On Tue, 9 Dec 2014 12:34:02 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Dec 09, 2014 at 11:27:16AM +0100, Igor Mammedov wrote:
> > > On Mon, 8 Dec 2014 21:13:25 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Mon, Dec 08, 2014 at 04:08:01PM +0000, Igor Mammedov wrote:
> > > > > when bridge hotplug is disabled, i.e. for machine
> > > > > types less then 2.0, bridge device was created as
> > > > > hotpluggable by mistake since commit 133a2da.
> > > > > 
> > > > > Fix it by just creating it as a present device.
> > > > > 
> > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > 
> > > > What exactly is the problem here?
> > > > It seems that such bridge is hotpluggable, even though
> > > > e.g. windows guests lacks drivers to support this.
> > > before 133a2da slot with existing at startup bridge weren't
> > > marked as hotpluggable nor described in SSDT. But after
> > > 133a2da it's described as hotpluggable slot for compat
> > > machines (2.0 and lower) which doesn't match with original
> > > behavior.
> > > 
> > > Also Marcel mentioned that bridges could be hotpluggable
> > > but that they should not be hot-UNpluggable.
> > 
> > OK so is there some guest that's confused?
> > What's the bug?
> It allows to do eject on bridge when it shouldn't.

I'm not sure it shouldn't. Assuming guest supports SHPC, eject on such a
bridge might well work correctly.


> I don't know about consequences of it,
> it's better not to allow invalid action in the first place.
> 
> 
> > 
> > > > 
> > > > 
> > > > > ---
> > > > >  hw/i386/acpi-build.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > index b37a397..1fb92e5 100644
> > > > > --- a/hw/i386/acpi-build.c
> > > > > +++ b/hw/i386/acpi-build.c
> > > > > @@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > > >              }
> > > > >          }
> > > > >  
> > > > > -        if (!dc->hotpluggable || bridge_in_acpi) {
> > > > > +        if (!dc->hotpluggable || pc->is_bridge) {
> > > > >              clear_bit(slot, slot_hotplug_enable);
> > > > >          }
> > > > >      }
> > > > > -- 
> > > > > 1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Igor Mammedov]

On Tue, 9 Dec 2014 14:51:59 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 09, 2014 at 12:45:27PM +0100, Igor Mammedov wrote:
> > On Tue, 9 Dec 2014 12:34:02 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Dec 09, 2014 at 11:27:16AM +0100, Igor Mammedov wrote:
> > > > On Mon, 8 Dec 2014 21:13:25 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Mon, Dec 08, 2014 at 04:08:01PM +0000, Igor Mammedov wrote:
> > > > > > when bridge hotplug is disabled, i.e. for machine
> > > > > > types less then 2.0, bridge device was created as
> > > > > > hotpluggable by mistake since commit 133a2da.
> > > > > > 
> > > > > > Fix it by just creating it as a present device.
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > 
> > > > > What exactly is the problem here?
> > > > > It seems that such bridge is hotpluggable, even though
> > > > > e.g. windows guests lacks drivers to support this.
> > > > before 133a2da slot with existing at startup bridge weren't
> > > > marked as hotpluggable nor described in SSDT. But after
> > > > 133a2da it's described as hotpluggable slot for compat
> > > > machines (2.0 and lower) which doesn't match with original
> > > > behavior.
> > > > 
> > > > Also Marcel mentioned that bridges could be hotpluggable
> > > > but that they should not be hot-UNpluggable.
> > > 
> > > OK so is there some guest that's confused?
> > > What's the bug?
> > It allows to do eject on bridge when it shouldn't.
> 
> I'm not sure it shouldn't. Assuming guest supports SHPC, eject on such a
> bridge might well work correctly.
If I remember correctly SHPC hotplug doesn't need ASL parts at all.

> 
> 
> > I don't know about consequences of it,
> > it's better not to allow invalid action in the first place.
> > 
> > 
> > > 
> > > > > 
> > > > > 
> > > > > > ---
> > > > > >  hw/i386/acpi-build.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > index b37a397..1fb92e5 100644
> > > > > > --- a/hw/i386/acpi-build.c
> > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > @@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > > > >              }
> > > > > >          }
> > > > > >  
> > > > > > -        if (!dc->hotpluggable || bridge_in_acpi) {
> > > > > > +        if (!dc->hotpluggable || pc->is_bridge) {
> > > > > >              clear_bit(slot, slot_hotplug_enable);
> > > > > >          }
> > > > > >      }
> > > > > > -- 
> > > > > > 1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 01:57:51PM +0100, Igor Mammedov wrote:
> On Tue, 9 Dec 2014 14:51:59 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Dec 09, 2014 at 12:45:27PM +0100, Igor Mammedov wrote:
> > > On Tue, 9 Dec 2014 12:34:02 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > 
> > > > On Tue, Dec 09, 2014 at 11:27:16AM +0100, Igor Mammedov wrote:
> > > > > On Mon, 8 Dec 2014 21:13:25 +0200
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > 
> > > > > > On Mon, Dec 08, 2014 at 04:08:01PM +0000, Igor Mammedov wrote:
> > > > > > > when bridge hotplug is disabled, i.e. for machine
> > > > > > > types less then 2.0, bridge device was created as
> > > > > > > hotpluggable by mistake since commit 133a2da.
> > > > > > > 
> > > > > > > Fix it by just creating it as a present device.
> > > > > > > 
> > > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > > 
> > > > > > What exactly is the problem here?
> > > > > > It seems that such bridge is hotpluggable, even though
> > > > > > e.g. windows guests lacks drivers to support this.
> > > > > before 133a2da slot with existing at startup bridge weren't
> > > > > marked as hotpluggable nor described in SSDT. But after
> > > > > 133a2da it's described as hotpluggable slot for compat
> > > > > machines (2.0 and lower) which doesn't match with original
> > > > > behavior.
> > > > > 
> > > > > Also Marcel mentioned that bridges could be hotpluggable
> > > > > but that they should not be hot-UNpluggable.
> > > > 
> > > > OK so is there some guest that's confused?
> > > > What's the bug?
> > > It allows to do eject on bridge when it shouldn't.
> > 
> > I'm not sure it shouldn't. Assuming guest supports SHPC, eject on such a
> > bridge might well work correctly.
> If I remember correctly SHPC hotplug doesn't need ASL parts at all.

Right. And then presumably bridge itself can be removed.

> > 
> > 
> > > I don't know about consequences of it,
> > > it's better not to allow invalid action in the first place.
> > > 
> > > 
> > > > 
> > > > > > 
> > > > > > 
> > > > > > > ---
> > > > > > >  hw/i386/acpi-build.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > 
> > > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > > index b37a397..1fb92e5 100644
> > > > > > > --- a/hw/i386/acpi-build.c
> > > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > > @@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > > > > >              }
> > > > > > >          }
> > > > > > >  
> > > > > > > -        if (!dc->hotpluggable || bridge_in_acpi) {
> > > > > > > +        if (!dc->hotpluggable || pc->is_bridge) {
> > > > > > >              clear_bit(slot, slot_hotplug_enable);
> > > > > > >          }
> > > > > > >      }
> > > > > > > -- 
> > > > > > > 1.8.3.1

Re: [Qemu-devel] [PATCH 2/9] pc: acpi: decribe bridge device as not hotpluggable

[Igor Mammedov]

On Tue, 9 Dec 2014 14:51:59 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 09, 2014 at 12:45:27PM +0100, Igor Mammedov wrote:
> > On Tue, 9 Dec 2014 12:34:02 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Tue, Dec 09, 2014 at 11:27:16AM +0100, Igor Mammedov wrote:
> > > > On Mon, 8 Dec 2014 21:13:25 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > 
> > > > > On Mon, Dec 08, 2014 at 04:08:01PM +0000, Igor Mammedov wrote:
> > > > > > when bridge hotplug is disabled, i.e. for machine
> > > > > > types less then 2.0, bridge device was created as
> > > > > > hotpluggable by mistake since commit 133a2da.
> > > > > > 
> > > > > > Fix it by just creating it as a present device.
> > > > > > 
> > > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > > 
> > > > > What exactly is the problem here?
> > > > > It seems that such bridge is hotpluggable, even though
> > > > > e.g. windows guests lacks drivers to support this.
> > > > before 133a2da slot with existing at startup bridge weren't
> > > > marked as hotpluggable nor described in SSDT. But after
> > > > 133a2da it's described as hotpluggable slot for compat
> > > > machines (2.0 and lower) which doesn't match with original
> > > > behavior.
> > > > 
> > > > Also Marcel mentioned that bridges could be hotpluggable
> > > > but that they should not be hot-UNpluggable.
> > > 
> > > OK so is there some guest that's confused?
> > > What's the bug?
> > It allows to do eject on bridge when it shouldn't.
> 
> I'm not sure it shouldn't. Assuming guest supports SHPC, eject on such a
> bridge might well work correctly.
Question here is should we keep compat mode as it was for old machine types
(i.e. not hot-removable), for new machines bridge is not hot-removable now
or just ignore silent change made by 133a2da?

> 
> 
> > I don't know about consequences of it,
> > it's better not to allow invalid action in the first place.
> > 
> > 
> > > 
> > > > > 
> > > > > 
> > > > > > ---
> > > > > >  hw/i386/acpi-build.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > 
> > > > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > > > index b37a397..1fb92e5 100644
> > > > > > --- a/hw/i386/acpi-build.c
> > > > > > +++ b/hw/i386/acpi-build.c
> > > > > > @@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > > > >              }
> > > > > >          }
> > > > > >  
> > > > > > -        if (!dc->hotpluggable || bridge_in_acpi) {
> > > > > > +        if (!dc->hotpluggable || pc->is_bridge) {
> > > > > >              clear_bit(slot, slot_hotplug_enable);
> > > > > >          }
> > > > > >      }
> > > > > > -- 
> > > > > > 1.8.3.1

[Qemu-devel] [PATCH 3/9] pc: acpi-build: cleanup AcpiPmInfo initialization

[Igor Mammedov]

zero initialize AcpiPmInfo struct to reduce code bloat
a little bit.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1fb92e5..f5ec66a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -161,6 +161,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     Object *obj = NULL;
     QObject *o;
 
+    memset(pm, 0, sizeof(*pm));
+
     if (piix) {
         obj = piix;
     }
@@ -173,22 +175,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
     if (o) {
         pm->s3_disabled = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s3_disabled = false;
     }
     qobject_decref(o);
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
     if (o) {
         pm->s4_disabled = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s4_disabled = false;
     }
     qobject_decref(o);
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
     if (o) {
         pm->s4_val = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s4_val = false;
     }
     qobject_decref(o);
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/9] pc: acpi-build: cleanup AcpiPmInfo initialization

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:02PM +0000, Igor Mammedov wrote:
> zero initialize AcpiPmInfo struct to reduce code bloat
> a little bit.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I generally prefer explicit initialization, but
it's a matter of taste.

> ---
>  hw/i386/acpi-build.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1fb92e5..f5ec66a 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -161,6 +161,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      Object *obj = NULL;
>      QObject *o;
>  
> +    memset(pm, 0, sizeof(*pm));
> +
>      if (piix) {
>          obj = piix;
>      }
> @@ -173,22 +175,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
>      if (o) {
>          pm->s3_disabled = qint_get_int(qobject_to_qint(o));
> -    } else {
> -        pm->s3_disabled = false;
>      }
>      qobject_decref(o);
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
>      if (o) {
>          pm->s4_disabled = qint_get_int(qobject_to_qint(o));
> -    } else {
> -        pm->s4_disabled = false;
>      }
>      qobject_decref(o);
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
>      if (o) {
>          pm->s4_val = qint_get_int(qobject_to_qint(o));
> -    } else {
> -        pm->s4_val = false;
>      }
>      qobject_decref(o);
>  
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 3/9] pc: acpi-build: cleanup AcpiPmInfo initialization

[Igor Mammedov]

On Mon, 8 Dec 2014 23:03:02 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 08, 2014 at 04:08:02PM +0000, Igor Mammedov wrote:
> > zero initialize AcpiPmInfo struct to reduce code bloat
> > a little bit.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> I generally prefer explicit initialization, but
> it's a matter of taste.
I did this since it saves some LOCs and will save even
more later with patches for dynamic _CRS.

> > ---
> >  hw/i386/acpi-build.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 1fb92e5..f5ec66a 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -161,6 +161,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> >      Object *obj = NULL;
> >      QObject *o;
> >  
> > +    memset(pm, 0, sizeof(*pm));
> > +
> >      if (piix) {
> >          obj = piix;
> >      }
> > @@ -173,22 +175,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> >      o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
> >      if (o) {
> >          pm->s3_disabled = qint_get_int(qobject_to_qint(o));
> > -    } else {
> > -        pm->s3_disabled = false;
> >      }
> >      qobject_decref(o);
> >      o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
> >      if (o) {
> >          pm->s4_disabled = qint_get_int(qobject_to_qint(o));
> > -    } else {
> > -        pm->s4_disabled = false;
> >      }
> >      qobject_decref(o);
> >      o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
> >      if (o) {
> >          pm->s4_val = qint_get_int(qobject_to_qint(o));
> > -    } else {
> > -        pm->s4_val = false;
> >      }
> >      qobject_decref(o);
> >  
> > -- 
> > 1.8.3.1
> 

[Qemu-devel] [PATCH 4/9] acpi: build_append_nameseg(): add padding if necessary

[Igor Mammedov]

According to ACPI spec NameSeg shorter than 4 characters
must be padded up to 4 characters with "_" symbol.
ACPI 5.0:  20.2.2 "Name Objects Encoding"

Do it in build_append_nameseg() so that caller shouldn't know
or care about it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f5ec66a..a8b7a2b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -292,6 +292,8 @@ static inline void build_append_array(GArray *array, GArray *val)
     g_array_append_vals(array, val->data, val->len);
 }
 
+#define ACPI_NAMESEG_LEN 4
+
 static void GCC_FMT_ATTR(2, 3)
 build_append_nameseg(GArray *array, const char *format, ...)
 {
@@ -299,13 +301,19 @@ build_append_nameseg(GArray *array, const char *format, ...)
     char s[] = "XXXX";
     int len;
     va_list args;
+    const char padding = '_';
 
     va_start(args, format);
     len = vsnprintf(s, sizeof s, format, args);
     va_end(args);
 
-    assert(len == 4);
+    g_assert(len <= ACPI_NAMESEG_LEN);
+
     g_array_append_vals(array, s, len);
+    while (len != ACPI_NAMESEG_LEN) {
+        g_array_append_val(array, padding);
+        ++len;
+    }
 }
 
 /* 5.4 Definition Block Encoding */
@@ -846,7 +854,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 
     if (bus->parent_dev) {
         op = 0x82; /* DeviceOp */
-        build_append_nameseg(bus_table, "S%.02X_",
+        build_append_nameseg(bus_table, "S%.02X",
                              bus->parent_dev->devfn);
         build_append_byte(bus_table, 0x08); /* NameOp */
         build_append_nameseg(bus_table, "_SUN");
@@ -966,7 +974,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             build_append_int(notify, 0x1U << i);
             build_append_byte(notify, 0x00); /* NullName */
             build_append_byte(notify, 0x86); /* NotifyOp */
-            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
+            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
             build_append_byte(notify, 0x69); /* Arg1Op */
 
             /* Pack it up */
@@ -1023,7 +1031,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         if (bus->parent_dev) {
             build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
             build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
-            build_append_nameseg(parent->notify_table, "S%.02X_",
+            build_append_nameseg(parent->notify_table, "S%.02X",
                                  bus->parent_dev->devfn);
             build_append_nameseg(parent->notify_table, "PCNT");
         }
@@ -1093,7 +1101,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         GArray *sb_scope = build_alloc_array();
         uint8_t op = 0x10; /* ScopeOp */
 
-        build_append_nameseg(sb_scope, "_SB_");
+        build_append_nameseg(sb_scope, "_SB");
 
         /* build Processor object for each processor */
         for (i = 0; i < acpi_cpus; i++) {
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/9] acpi: build_append_nameseg(): add padding if necessary

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:03PM +0000, Igor Mammedov wrote:
> According to ACPI spec NameSeg shorter than 4 characters
> must be padded up to 4 characters with "_" symbol.
> ACPI 5.0:  20.2.2 "Name Objects Encoding"
> 
> Do it in build_append_nameseg() so that caller shouldn't know
> or care about it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

To me just doing it right in callers seems just as easy, but
I guess you disagree :)

> ---
>  hw/i386/acpi-build.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index f5ec66a..a8b7a2b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -292,6 +292,8 @@ static inline void build_append_array(GArray *array, GArray *val)
>      g_array_append_vals(array, val->data, val->len);
>  }
>  
> +#define ACPI_NAMESEG_LEN 4
> +
>  static void GCC_FMT_ATTR(2, 3)
>  build_append_nameseg(GArray *array, const char *format, ...)
>  {
> @@ -299,13 +301,19 @@ build_append_nameseg(GArray *array, const char *format, ...)
>      char s[] = "XXXX";
>      int len;
>      va_list args;
> +    const char padding = '_';
>  
>      va_start(args, format);
>      len = vsnprintf(s, sizeof s, format, args);
>      va_end(args);
>  
> -    assert(len == 4);
> +    g_assert(len <= ACPI_NAMESEG_LEN);

I'm not sure when is g_assert preferable to assert.
What's the motivation here?


> +
>      g_array_append_vals(array, s, len);
> +    while (len != ACPI_NAMESEG_LEN) {
> +        g_array_append_val(array, padding);
> +        ++len;
> +    }

Easier

	/* Pad up to 4 characters if necessary. */
	g_array_append_vals(array, "____", 4 - len);



>  }
>  
>  /* 5.4 Definition Block Encoding */
> @@ -846,7 +854,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>  
>      if (bus->parent_dev) {
>          op = 0x82; /* DeviceOp */
> -        build_append_nameseg(bus_table, "S%.02X_",
> +        build_append_nameseg(bus_table, "S%.02X",
>                               bus->parent_dev->devfn);
>          build_append_byte(bus_table, 0x08); /* NameOp */
>          build_append_nameseg(bus_table, "_SUN");
> @@ -966,7 +974,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>              build_append_int(notify, 0x1U << i);
>              build_append_byte(notify, 0x00); /* NullName */
>              build_append_byte(notify, 0x86); /* NotifyOp */
> -            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
> +            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
>              build_append_byte(notify, 0x69); /* Arg1Op */
>  
>              /* Pack it up */
> @@ -1023,7 +1031,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>          if (bus->parent_dev) {
>              build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
>              build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> -            build_append_nameseg(parent->notify_table, "S%.02X_",
> +            build_append_nameseg(parent->notify_table, "S%.02X",
>                                   bus->parent_dev->devfn);
>              build_append_nameseg(parent->notify_table, "PCNT");
>          }
> @@ -1093,7 +1101,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          GArray *sb_scope = build_alloc_array();
>          uint8_t op = 0x10; /* ScopeOp */
>  
> -        build_append_nameseg(sb_scope, "_SB_");
> +        build_append_nameseg(sb_scope, "_SB");
>  
>          /* build Processor object for each processor */
>          for (i = 0; i < acpi_cpus; i++) {
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 4/9] acpi: build_append_nameseg(): add padding if necessary

[Igor Mammedov]

On Mon, 8 Dec 2014 23:15:32 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 08, 2014 at 04:08:03PM +0000, Igor Mammedov wrote:
> > According to ACPI spec NameSeg shorter than 4 characters
> > must be padded up to 4 characters with "_" symbol.
> > ACPI 5.0:  20.2.2 "Name Objects Encoding"
> > 
> > Do it in build_append_nameseg() so that caller shouldn't know
> > or care about it.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> To me just doing it right in callers seems just as easy, but
> I guess you disagree :)
That means, author MUST know about padding, if he/she doesn't
or forget about it that would introduce error usually resulting
in BSOD. This patch allows to avoid such mistakes.

> 
> > ---
> >  hw/i386/acpi-build.c | 18 +++++++++++++-----
> >  1 file changed, 13 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index f5ec66a..a8b7a2b 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -292,6 +292,8 @@ static inline void build_append_array(GArray *array, GArray *val)
> >      g_array_append_vals(array, val->data, val->len);
> >  }
> >  
> > +#define ACPI_NAMESEG_LEN 4
> > +
> >  static void GCC_FMT_ATTR(2, 3)
> >  build_append_nameseg(GArray *array, const char *format, ...)
> >  {
> > @@ -299,13 +301,19 @@ build_append_nameseg(GArray *array, const char *format, ...)
> >      char s[] = "XXXX";
> >      int len;
> >      va_list args;
> > +    const char padding = '_';
> >  
> >      va_start(args, format);
> >      len = vsnprintf(s, sizeof s, format, args);
> >      va_end(args);
> >  
> > -    assert(len == 4);
> > +    g_assert(len <= ACPI_NAMESEG_LEN);
> 
> I'm not sure when is g_assert preferable to assert.
> What's the motivation here?
> 
> 
> > +
> >      g_array_append_vals(array, s, len);
> > +    while (len != ACPI_NAMESEG_LEN) {
> > +        g_array_append_val(array, padding);
> > +        ++len;
> > +    }
> 
> Easier
> 
> 	/* Pad up to 4 characters if necessary. */
> 	g_array_append_vals(array, "____", 4 - len);
> 
> 
> 
> >  }
> >  
> >  /* 5.4 Definition Block Encoding */
> > @@ -846,7 +854,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >  
> >      if (bus->parent_dev) {
> >          op = 0x82; /* DeviceOp */
> > -        build_append_nameseg(bus_table, "S%.02X_",
> > +        build_append_nameseg(bus_table, "S%.02X",
> >                               bus->parent_dev->devfn);
> >          build_append_byte(bus_table, 0x08); /* NameOp */
> >          build_append_nameseg(bus_table, "_SUN");
> > @@ -966,7 +974,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >              build_append_int(notify, 0x1U << i);
> >              build_append_byte(notify, 0x00); /* NullName */
> >              build_append_byte(notify, 0x86); /* NotifyOp */
> > -            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
> > +            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> >              build_append_byte(notify, 0x69); /* Arg1Op */
> >  
> >              /* Pack it up */
> > @@ -1023,7 +1031,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >          if (bus->parent_dev) {
> >              build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> >              build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> > -            build_append_nameseg(parent->notify_table, "S%.02X_",
> > +            build_append_nameseg(parent->notify_table, "S%.02X",
> >                                   bus->parent_dev->devfn);
> >              build_append_nameseg(parent->notify_table, "PCNT");
> >          }
> > @@ -1093,7 +1101,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >          GArray *sb_scope = build_alloc_array();
> >          uint8_t op = 0x10; /* ScopeOp */
> >  
> > -        build_append_nameseg(sb_scope, "_SB_");
> > +        build_append_nameseg(sb_scope, "_SB");
> >  
> >          /* build Processor object for each processor */
> >          for (i = 0; i < acpi_cpus; i++) {
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH 4/9] acpi: build_append_nameseg(): add padding if necessary

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 11:32:45AM +0100, Igor Mammedov wrote:
> On Mon, 8 Dec 2014 23:15:32 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Dec 08, 2014 at 04:08:03PM +0000, Igor Mammedov wrote:
> > > According to ACPI spec NameSeg shorter than 4 characters
> > > must be padded up to 4 characters with "_" symbol.
> > > ACPI 5.0:  20.2.2 "Name Objects Encoding"
> > > 
> > > Do it in build_append_nameseg() so that caller shouldn't know
> > > or care about it.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > To me just doing it right in callers seems just as easy, but
> > I guess you disagree :)
> That means, author MUST know about padding, if he/she doesn't
> or forget about it that would introduce error usually resulting
> in BSOD.

Not really. assert will trigger on qemu start.

> This patch allows to avoid such mistakes.

Even the most basic testing (e.g. make check) will find these mistakes.
It's more a question of which API we prefer.

Anyway, could you respond on g_assert vs assert please?

> > 
> > > ---
> > >  hw/i386/acpi-build.c | 18 +++++++++++++-----
> > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index f5ec66a..a8b7a2b 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -292,6 +292,8 @@ static inline void build_append_array(GArray *array, GArray *val)
> > >      g_array_append_vals(array, val->data, val->len);
> > >  }
> > >  
> > > +#define ACPI_NAMESEG_LEN 4
> > > +
> > >  static void GCC_FMT_ATTR(2, 3)
> > >  build_append_nameseg(GArray *array, const char *format, ...)
> > >  {
> > > @@ -299,13 +301,19 @@ build_append_nameseg(GArray *array, const char *format, ...)
> > >      char s[] = "XXXX";
> > >      int len;
> > >      va_list args;
> > > +    const char padding = '_';
> > >  
> > >      va_start(args, format);
> > >      len = vsnprintf(s, sizeof s, format, args);
> > >      va_end(args);
> > >  
> > > -    assert(len == 4);
> > > +    g_assert(len <= ACPI_NAMESEG_LEN);
> > 
> > I'm not sure when is g_assert preferable to assert.
> > What's the motivation here?
> > 
> > 
> > > +
> > >      g_array_append_vals(array, s, len);
> > > +    while (len != ACPI_NAMESEG_LEN) {
> > > +        g_array_append_val(array, padding);
> > > +        ++len;
> > > +    }
> > 
> > Easier
> > 
> > 	/* Pad up to 4 characters if necessary. */
> > 	g_array_append_vals(array, "____", 4 - len);
> > 
> > 
> > 
> > >  }
> > >  
> > >  /* 5.4 Definition Block Encoding */
> > > @@ -846,7 +854,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >  
> > >      if (bus->parent_dev) {
> > >          op = 0x82; /* DeviceOp */
> > > -        build_append_nameseg(bus_table, "S%.02X_",
> > > +        build_append_nameseg(bus_table, "S%.02X",
> > >                               bus->parent_dev->devfn);
> > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > >          build_append_nameseg(bus_table, "_SUN");
> > > @@ -966,7 +974,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >              build_append_int(notify, 0x1U << i);
> > >              build_append_byte(notify, 0x00); /* NullName */
> > >              build_append_byte(notify, 0x86); /* NotifyOp */
> > > -            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
> > > +            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> > >              build_append_byte(notify, 0x69); /* Arg1Op */
> > >  
> > >              /* Pack it up */
> > > @@ -1023,7 +1031,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >          if (bus->parent_dev) {
> > >              build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> > >              build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> > > -            build_append_nameseg(parent->notify_table, "S%.02X_",
> > > +            build_append_nameseg(parent->notify_table, "S%.02X",
> > >                                   bus->parent_dev->devfn);
> > >              build_append_nameseg(parent->notify_table, "PCNT");
> > >          }
> > > @@ -1093,7 +1101,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >          GArray *sb_scope = build_alloc_array();
> > >          uint8_t op = 0x10; /* ScopeOp */
> > >  
> > > -        build_append_nameseg(sb_scope, "_SB_");
> > > +        build_append_nameseg(sb_scope, "_SB");
> > >  
> > >          /* build Processor object for each processor */
> > >          for (i = 0; i < acpi_cpus; i++) {
> > > -- 
> > > 1.8.3.1

Re: [Qemu-devel] [PATCH 4/9] acpi: build_append_nameseg(): add padding if necessary

[Igor Mammedov]

On Tue, 9 Dec 2014 12:38:12 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 09, 2014 at 11:32:45AM +0100, Igor Mammedov wrote:
> > On Mon, 8 Dec 2014 23:15:32 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Dec 08, 2014 at 04:08:03PM +0000, Igor Mammedov wrote:
> > > > According to ACPI spec NameSeg shorter than 4 characters
> > > > must be padded up to 4 characters with "_" symbol.
> > > > ACPI 5.0:  20.2.2 "Name Objects Encoding"
> > > > 
> > > > Do it in build_append_nameseg() so that caller shouldn't know
> > > > or care about it.
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > To me just doing it right in callers seems just as easy, but
> > > I guess you disagree :)
> > That means, author MUST know about padding, if he/she doesn't
> > or forget about it that would introduce error usually resulting
> > in BSOD.
> 
> Not really. assert will trigger on qemu start.
it sure will, but a bit confusing according to ASL spec name is up to
4 charactes and only AML spec specifies that there should be padding
if necessary.
I'm trying to get away from constructing AML by hands and the next step
for API would be ASL like one which is my eventual target in the end.

> 
> > This patch allows to avoid such mistakes.
> 
> Even the most basic testing (e.g. make check) will find these mistakes.
> It's more a question of which API we prefer.
> 
> Anyway, could you respond on g_assert vs assert please?
Ah, I'm sorry about that, haven't noticed comments below.

> 
> > > 
> > > > ---
> > > >  hw/i386/acpi-build.c | 18 +++++++++++++-----
> > > >  1 file changed, 13 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index f5ec66a..a8b7a2b 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -292,6 +292,8 @@ static inline void build_append_array(GArray *array, GArray *val)
> > > >      g_array_append_vals(array, val->data, val->len);
> > > >  }
> > > >  
> > > > +#define ACPI_NAMESEG_LEN 4
> > > > +
> > > >  static void GCC_FMT_ATTR(2, 3)
> > > >  build_append_nameseg(GArray *array, const char *format, ...)
> > > >  {
> > > > @@ -299,13 +301,19 @@ build_append_nameseg(GArray *array, const char *format, ...)
> > > >      char s[] = "XXXX";
> > > >      int len;
> > > >      va_list args;
> > > > +    const char padding = '_';
> > > >  
> > > >      va_start(args, format);
> > > >      len = vsnprintf(s, sizeof s, format, args);
> > > >      va_end(args);
> > > >  
> > > > -    assert(len == 4);
> > > > +    g_assert(len <= ACPI_NAMESEG_LEN);
> > > 
> > > I'm not sure when is g_assert preferable to assert.
> > > What's the motivation here?
None, I'm not sure what policy on it either
I can keep assert here if preferable.

> > > 
> > > 
> > > > +
> > > >      g_array_append_vals(array, s, len);
> > > > +    while (len != ACPI_NAMESEG_LEN) {
> > > > +        g_array_append_val(array, padding);
> > > > +        ++len;
> > > > +    }
> > > 
> > > Easier
> > > 
> > > 	/* Pad up to 4 characters if necessary. */
> > > 	g_array_append_vals(array, "____", 4 - len);
Thanks, I'll fix it up.

> > > 
> > > 
> > > 
> > > >  }
> > > >  
> > > >  /* 5.4 Definition Block Encoding */
> > > > @@ -846,7 +854,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > >  
> > > >      if (bus->parent_dev) {
> > > >          op = 0x82; /* DeviceOp */
> > > > -        build_append_nameseg(bus_table, "S%.02X_",
> > > > +        build_append_nameseg(bus_table, "S%.02X",
> > > >                               bus->parent_dev->devfn);
> > > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > >          build_append_nameseg(bus_table, "_SUN");
> > > > @@ -966,7 +974,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > >              build_append_int(notify, 0x1U << i);
> > > >              build_append_byte(notify, 0x00); /* NullName */
> > > >              build_append_byte(notify, 0x86); /* NotifyOp */
> > > > -            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
> > > > +            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> > > >              build_append_byte(notify, 0x69); /* Arg1Op */
> > > >  
> > > >              /* Pack it up */
> > > > @@ -1023,7 +1031,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > >          if (bus->parent_dev) {
> > > >              build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> > > >              build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> > > > -            build_append_nameseg(parent->notify_table, "S%.02X_",
> > > > +            build_append_nameseg(parent->notify_table, "S%.02X",
> > > >                                   bus->parent_dev->devfn);
> > > >              build_append_nameseg(parent->notify_table, "PCNT");
> > > >          }
> > > > @@ -1093,7 +1101,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >          GArray *sb_scope = build_alloc_array();
> > > >          uint8_t op = 0x10; /* ScopeOp */
> > > >  
> > > > -        build_append_nameseg(sb_scope, "_SB_");
> > > > +        build_append_nameseg(sb_scope, "_SB");
> > > >  
> > > >          /* build Processor object for each processor */
> > > >          for (i = 0; i < acpi_cpus; i++) {
> > > > -- 
> > > > 1.8.3.1

[Qemu-devel] [PATCH 5/9] acpi: move generic aml building helpers into dedictated file

[Igor Mammedov]

the will be later used for composing AML primitives
and all that could be reused later for ARM machines
as well.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/Makefile.objs            |   1 +
 hw/acpi/acpi_gen_utils.c         | 169 +++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c             | 165 +-------------------------------------
 include/hw/acpi/acpi_gen_utils.h |  23 ++++++
 4 files changed, 195 insertions(+), 163 deletions(-)
 create mode 100644 hw/acpi/acpi_gen_utils.c
 create mode 100644 include/hw/acpi/acpi_gen_utils.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index acd2389..4237232 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -1,3 +1,4 @@
 common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
 common-obj-$(CONFIG_ACPI) += memory_hotplug.o
 common-obj-$(CONFIG_ACPI) += acpi_interface.o
+common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
new file mode 100644
index 0000000..1583b35
--- /dev/null
+++ b/hw/acpi/acpi_gen_utils.c
@@ -0,0 +1,169 @@
+#include <stdio.h>
+#include <stdarg.h>
+#include <assert.h>
+#include <stdbool.h>
+#include "hw/acpi/acpi_gen_utils.h"
+
+GArray *build_alloc_array(void)
+{
+        return g_array_new(false, true /* clear */, 1);
+}
+
+void build_free_array(GArray *array)
+{
+        g_array_free(array, true);
+}
+
+void build_prepend_byte(GArray *array, uint8_t val)
+{
+    g_array_prepend_val(array, val);
+}
+
+void build_append_byte(GArray *array, uint8_t val)
+{
+    g_array_append_val(array, val);
+}
+
+void build_append_array(GArray *array, GArray *val)
+{
+    g_array_append_vals(array, val->data, val->len);
+}
+
+#define ACPI_NAMESEG_LEN 4
+
+void GCC_FMT_ATTR(2, 3)
+build_append_nameseg(GArray *array, const char *format, ...)
+{
+    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+    char s[] = "XXXX";
+    int len;
+    va_list args;
+    const char padding = '_';
+
+    va_start(args, format);
+    len = vsnprintf(s, sizeof s, format, args);
+    va_end(args);
+
+    g_assert(len <= ACPI_NAMESEG_LEN);
+
+    g_array_append_vals(array, s, len);
+    while (len != ACPI_NAMESEG_LEN) {
+        g_array_append_val(array, padding);
+        ++len;
+    }
+}
+
+/* 5.4 Definition Block Encoding */
+enum {
+    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
+    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
+    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
+    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
+};
+
+void build_prepend_package_length(GArray *package, unsigned min_bytes)
+{
+    uint8_t byte;
+    unsigned length = package->len;
+    unsigned length_bytes;
+
+    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
+        length_bytes = 1;
+    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
+        length_bytes = 2;
+    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
+        length_bytes = 3;
+    } else {
+        length_bytes = 4;
+    }
+
+    /* Force length to at least min_bytes.
+     * This wastes memory but that's how bios did it.
+     */
+    length_bytes = MAX(length_bytes, min_bytes);
+
+    /* PkgLength is the length of the inclusive length of the data. */
+    length += length_bytes;
+
+    switch (length_bytes) {
+    case 1:
+        byte = length;
+        build_prepend_byte(package, byte);
+        return;
+    case 4:
+        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
+        build_prepend_byte(package, byte);
+        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
+        /* fall through */
+    case 3:
+        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
+        build_prepend_byte(package, byte);
+        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
+        /* fall through */
+    case 2:
+        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
+        build_prepend_byte(package, byte);
+        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
+        /* fall through */
+    }
+    /*
+     * Most significant two bits of byte zero indicate how many following bytes
+     * are in PkgLength encoding.
+     */
+    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
+    build_prepend_byte(package, byte);
+}
+
+void build_package(GArray *package, uint8_t op, unsigned min_bytes)
+{
+    build_prepend_package_length(package, min_bytes);
+    build_prepend_byte(package, op);
+}
+
+void build_extop_package(GArray *package, uint8_t op)
+{
+    build_package(package, op, 1);
+    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
+}
+
+void build_append_value(GArray *table, uint32_t value, int size)
+{
+    uint8_t prefix;
+    int i;
+
+    switch (size) {
+    case 1:
+        prefix = 0x0A; /* BytePrefix */
+        break;
+    case 2:
+        prefix = 0x0B; /* WordPrefix */
+        break;
+    case 4:
+        prefix = 0x0C; /* DWordPrefix */
+        break;
+    default:
+        assert(0);
+        return;
+    }
+    build_append_byte(table, prefix);
+    for (i = 0; i < size; ++i) {
+        build_append_byte(table, value & 0xFF);
+        value = value >> 8;
+    }
+}
+
+void build_append_int(GArray *table, uint32_t value)
+{
+    if (value == 0x00) {
+        build_append_byte(table, 0x00); /* ZeroOp */
+    } else if (value == 0x01) {
+        build_append_byte(table, 0x01); /* OneOp */
+    } else if (value <= 0xFF) {
+        build_append_value(table, value, 1);
+    } else if (value <= 0xFFFF) {
+        build_append_value(table, value, 2);
+    } else {
+        build_append_value(table, value, 4);
+    }
+}
+
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a8b7a2b..870e4a0 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -54,6 +54,8 @@
 #include "hw/i386/q35-acpi-dsdt.hex"
 #include "hw/i386/acpi-dsdt.hex"
 
+#include "hw/acpi/acpi_gen_utils.h"
+
 #include "qapi/qmp/qint.h"
 #include "qom/qom-qobject.h"
 #include "exec/ram_addr.h"
@@ -267,169 +269,6 @@ build_header(GArray *linker, GArray *table_data,
                                     table_data->data, h, len, &h->checksum);
 }
 
-static inline GArray *build_alloc_array(void)
-{
-        return g_array_new(false, true /* clear */, 1);
-}
-
-static inline void build_free_array(GArray *array)
-{
-        g_array_free(array, true);
-}
-
-static inline void build_prepend_byte(GArray *array, uint8_t val)
-{
-    g_array_prepend_val(array, val);
-}
-
-static inline void build_append_byte(GArray *array, uint8_t val)
-{
-    g_array_append_val(array, val);
-}
-
-static inline void build_append_array(GArray *array, GArray *val)
-{
-    g_array_append_vals(array, val->data, val->len);
-}
-
-#define ACPI_NAMESEG_LEN 4
-
-static void GCC_FMT_ATTR(2, 3)
-build_append_nameseg(GArray *array, const char *format, ...)
-{
-    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
-    char s[] = "XXXX";
-    int len;
-    va_list args;
-    const char padding = '_';
-
-    va_start(args, format);
-    len = vsnprintf(s, sizeof s, format, args);
-    va_end(args);
-
-    g_assert(len <= ACPI_NAMESEG_LEN);
-
-    g_array_append_vals(array, s, len);
-    while (len != ACPI_NAMESEG_LEN) {
-        g_array_append_val(array, padding);
-        ++len;
-    }
-}
-
-/* 5.4 Definition Block Encoding */
-enum {
-    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
-    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
-    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
-    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
-};
-
-static void build_prepend_package_length(GArray *package, unsigned min_bytes)
-{
-    uint8_t byte;
-    unsigned length = package->len;
-    unsigned length_bytes;
-
-    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
-        length_bytes = 1;
-    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
-        length_bytes = 2;
-    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
-        length_bytes = 3;
-    } else {
-        length_bytes = 4;
-    }
-
-    /* Force length to at least min_bytes.
-     * This wastes memory but that's how bios did it.
-     */
-    length_bytes = MAX(length_bytes, min_bytes);
-
-    /* PkgLength is the length of the inclusive length of the data. */
-    length += length_bytes;
-
-    switch (length_bytes) {
-    case 1:
-        byte = length;
-        build_prepend_byte(package, byte);
-        return;
-    case 4:
-        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
-        build_prepend_byte(package, byte);
-        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
-        /* fall through */
-    case 3:
-        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
-        build_prepend_byte(package, byte);
-        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
-        /* fall through */
-    case 2:
-        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
-        build_prepend_byte(package, byte);
-        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
-        /* fall through */
-    }
-    /*
-     * Most significant two bits of byte zero indicate how many following bytes
-     * are in PkgLength encoding.
-     */
-    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
-    build_prepend_byte(package, byte);
-}
-
-static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
-{
-    build_prepend_package_length(package, min_bytes);
-    build_prepend_byte(package, op);
-}
-
-static void build_extop_package(GArray *package, uint8_t op)
-{
-    build_package(package, op, 1);
-    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
-}
-
-static void build_append_value(GArray *table, uint32_t value, int size)
-{
-    uint8_t prefix;
-    int i;
-
-    switch (size) {
-    case 1:
-        prefix = 0x0A; /* BytePrefix */
-        break;
-    case 2:
-        prefix = 0x0B; /* WordPrefix */
-        break;
-    case 4:
-        prefix = 0x0C; /* DWordPrefix */
-        break;
-    default:
-        assert(0);
-        return;
-    }
-    build_append_byte(table, prefix);
-    for (i = 0; i < size; ++i) {
-        build_append_byte(table, value & 0xFF);
-        value = value >> 8;
-    }
-}
-
-static void build_append_int(GArray *table, uint32_t value)
-{
-    if (value == 0x00) {
-        build_append_byte(table, 0x00); /* ZeroOp */
-    } else if (value == 0x01) {
-        build_append_byte(table, 0x01); /* OneOp */
-    } else if (value <= 0xFF) {
-        build_append_value(table, value, 1);
-    } else if (value <= 0xFFFF) {
-        build_append_value(table, value, 2);
-    } else {
-        build_append_value(table, value, 4);
-    }
-}
-
 static GArray *build_alloc_method(const char *name, uint8_t arg_count)
 {
     GArray *method = build_alloc_array();
diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
new file mode 100644
index 0000000..e6a0b28
--- /dev/null
+++ b/include/hw/acpi/acpi_gen_utils.h
@@ -0,0 +1,23 @@
+#ifndef HW_ACPI_GEN_UTILS_H
+#define HW_ACPI_GEN_UTILS_H
+
+#include <stdint.h>
+#include <glib.h>
+#include "qemu/compiler.h"
+
+GArray *build_alloc_array(void);
+void build_free_array(GArray *array);
+void build_prepend_byte(GArray *array, uint8_t val);
+void build_append_byte(GArray *array, uint8_t val);
+void build_append_array(GArray *array, GArray *val);
+
+void GCC_FMT_ATTR(2, 3)
+build_append_nameseg(GArray *array, const char *format, ...);
+
+void build_prepend_package_length(GArray *package, unsigned min_bytes);
+void build_package(GArray *package, uint8_t op, unsigned min_bytes);
+void build_append_value(GArray *table, uint32_t value, int size);
+void build_append_int(GArray *table, uint32_t value);
+void build_extop_package(GArray *package, uint8_t op);
+
+#endif
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/9] acpi: move generic aml building helpers into dedictated file

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:04PM +0000, Igor Mammedov wrote:
> the will be later used for composing AML primitives
> and all that could be reused later for ARM machines
> as well.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

It will be easy to move when needed.
Why do it now?


> ---
>  hw/acpi/Makefile.objs            |   1 +
>  hw/acpi/acpi_gen_utils.c         | 169 +++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c             | 165 +-------------------------------------
>  include/hw/acpi/acpi_gen_utils.h |  23 ++++++
>  4 files changed, 195 insertions(+), 163 deletions(-)
>  create mode 100644 hw/acpi/acpi_gen_utils.c
>  create mode 100644 include/hw/acpi/acpi_gen_utils.h
> 
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index acd2389..4237232 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -1,3 +1,4 @@
>  common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
>  common-obj-$(CONFIG_ACPI) += memory_hotplug.o
>  common-obj-$(CONFIG_ACPI) += acpi_interface.o
> +common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> new file mode 100644
> index 0000000..1583b35
> --- /dev/null
> +++ b/hw/acpi/acpi_gen_utils.c
> @@ -0,0 +1,169 @@
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <assert.h>
> +#include <stdbool.h>
> +#include "hw/acpi/acpi_gen_utils.h"
> +
> +GArray *build_alloc_array(void)
> +{
> +        return g_array_new(false, true /* clear */, 1);
> +}
> +
> +void build_free_array(GArray *array)
> +{
> +        g_array_free(array, true);
> +}
> +
> +void build_prepend_byte(GArray *array, uint8_t val)
> +{
> +    g_array_prepend_val(array, val);
> +}
> +
> +void build_append_byte(GArray *array, uint8_t val)
> +{
> +    g_array_append_val(array, val);
> +}
> +
> +void build_append_array(GArray *array, GArray *val)
> +{
> +    g_array_append_vals(array, val->data, val->len);
> +}
> +
> +#define ACPI_NAMESEG_LEN 4
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_nameseg(GArray *array, const char *format, ...)
> +{
> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> +    char s[] = "XXXX";
> +    int len;
> +    va_list args;
> +    const char padding = '_';
> +
> +    va_start(args, format);
> +    len = vsnprintf(s, sizeof s, format, args);
> +    va_end(args);
> +
> +    g_assert(len <= ACPI_NAMESEG_LEN);
> +
> +    g_array_append_vals(array, s, len);
> +    while (len != ACPI_NAMESEG_LEN) {
> +        g_array_append_val(array, padding);
> +        ++len;
> +    }
> +}
> +
> +/* 5.4 Definition Block Encoding */
> +enum {
> +    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> +    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> +    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> +    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> +};
> +
> +void build_prepend_package_length(GArray *package, unsigned min_bytes)
> +{
> +    uint8_t byte;
> +    unsigned length = package->len;
> +    unsigned length_bytes;
> +
> +    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> +        length_bytes = 1;
> +    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> +        length_bytes = 2;
> +    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> +        length_bytes = 3;
> +    } else {
> +        length_bytes = 4;
> +    }
> +
> +    /* Force length to at least min_bytes.
> +     * This wastes memory but that's how bios did it.
> +     */
> +    length_bytes = MAX(length_bytes, min_bytes);
> +
> +    /* PkgLength is the length of the inclusive length of the data. */
> +    length += length_bytes;
> +
> +    switch (length_bytes) {
> +    case 1:
> +        byte = length;
> +        build_prepend_byte(package, byte);
> +        return;
> +    case 4:
> +        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> +        build_prepend_byte(package, byte);
> +        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> +        /* fall through */
> +    case 3:
> +        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> +        build_prepend_byte(package, byte);
> +        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> +        /* fall through */
> +    case 2:
> +        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> +        build_prepend_byte(package, byte);
> +        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> +        /* fall through */
> +    }
> +    /*
> +     * Most significant two bits of byte zero indicate how many following bytes
> +     * are in PkgLength encoding.
> +     */
> +    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> +    build_prepend_byte(package, byte);
> +}
> +
> +void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> +{
> +    build_prepend_package_length(package, min_bytes);
> +    build_prepend_byte(package, op);
> +}
> +
> +void build_extop_package(GArray *package, uint8_t op)
> +{
> +    build_package(package, op, 1);
> +    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> +}
> +
> +void build_append_value(GArray *table, uint32_t value, int size)
> +{
> +    uint8_t prefix;
> +    int i;
> +
> +    switch (size) {
> +    case 1:
> +        prefix = 0x0A; /* BytePrefix */
> +        break;
> +    case 2:
> +        prefix = 0x0B; /* WordPrefix */
> +        break;
> +    case 4:
> +        prefix = 0x0C; /* DWordPrefix */
> +        break;
> +    default:
> +        assert(0);
> +        return;
> +    }
> +    build_append_byte(table, prefix);
> +    for (i = 0; i < size; ++i) {
> +        build_append_byte(table, value & 0xFF);
> +        value = value >> 8;
> +    }
> +}
> +
> +void build_append_int(GArray *table, uint32_t value)
> +{
> +    if (value == 0x00) {
> +        build_append_byte(table, 0x00); /* ZeroOp */
> +    } else if (value == 0x01) {
> +        build_append_byte(table, 0x01); /* OneOp */
> +    } else if (value <= 0xFF) {
> +        build_append_value(table, value, 1);
> +    } else if (value <= 0xFFFF) {
> +        build_append_value(table, value, 2);
> +    } else {
> +        build_append_value(table, value, 4);
> +    }
> +}
> +
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a8b7a2b..870e4a0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -54,6 +54,8 @@
>  #include "hw/i386/q35-acpi-dsdt.hex"
>  #include "hw/i386/acpi-dsdt.hex"
>  
> +#include "hw/acpi/acpi_gen_utils.h"
> +
>  #include "qapi/qmp/qint.h"
>  #include "qom/qom-qobject.h"
>  #include "exec/ram_addr.h"
> @@ -267,169 +269,6 @@ build_header(GArray *linker, GArray *table_data,
>                                      table_data->data, h, len, &h->checksum);
>  }
>  
> -static inline GArray *build_alloc_array(void)
> -{
> -        return g_array_new(false, true /* clear */, 1);
> -}
> -
> -static inline void build_free_array(GArray *array)
> -{
> -        g_array_free(array, true);
> -}
> -
> -static inline void build_prepend_byte(GArray *array, uint8_t val)
> -{
> -    g_array_prepend_val(array, val);
> -}
> -
> -static inline void build_append_byte(GArray *array, uint8_t val)
> -{
> -    g_array_append_val(array, val);
> -}
> -
> -static inline void build_append_array(GArray *array, GArray *val)
> -{
> -    g_array_append_vals(array, val->data, val->len);
> -}
> -
> -#define ACPI_NAMESEG_LEN 4
> -
> -static void GCC_FMT_ATTR(2, 3)
> -build_append_nameseg(GArray *array, const char *format, ...)
> -{
> -    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> -    char s[] = "XXXX";
> -    int len;
> -    va_list args;
> -    const char padding = '_';
> -
> -    va_start(args, format);
> -    len = vsnprintf(s, sizeof s, format, args);
> -    va_end(args);
> -
> -    g_assert(len <= ACPI_NAMESEG_LEN);
> -
> -    g_array_append_vals(array, s, len);
> -    while (len != ACPI_NAMESEG_LEN) {
> -        g_array_append_val(array, padding);
> -        ++len;
> -    }
> -}
> -
> -/* 5.4 Definition Block Encoding */
> -enum {
> -    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> -    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> -    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> -    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> -};
> -
> -static void build_prepend_package_length(GArray *package, unsigned min_bytes)
> -{
> -    uint8_t byte;
> -    unsigned length = package->len;
> -    unsigned length_bytes;
> -
> -    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> -        length_bytes = 1;
> -    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> -        length_bytes = 2;
> -    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> -        length_bytes = 3;
> -    } else {
> -        length_bytes = 4;
> -    }
> -
> -    /* Force length to at least min_bytes.
> -     * This wastes memory but that's how bios did it.
> -     */
> -    length_bytes = MAX(length_bytes, min_bytes);
> -
> -    /* PkgLength is the length of the inclusive length of the data. */
> -    length += length_bytes;
> -
> -    switch (length_bytes) {
> -    case 1:
> -        byte = length;
> -        build_prepend_byte(package, byte);
> -        return;
> -    case 4:
> -        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> -        build_prepend_byte(package, byte);
> -        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> -        /* fall through */
> -    case 3:
> -        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> -        build_prepend_byte(package, byte);
> -        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> -        /* fall through */
> -    case 2:
> -        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> -        build_prepend_byte(package, byte);
> -        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> -        /* fall through */
> -    }
> -    /*
> -     * Most significant two bits of byte zero indicate how many following bytes
> -     * are in PkgLength encoding.
> -     */
> -    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> -    build_prepend_byte(package, byte);
> -}
> -
> -static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> -{
> -    build_prepend_package_length(package, min_bytes);
> -    build_prepend_byte(package, op);
> -}
> -
> -static void build_extop_package(GArray *package, uint8_t op)
> -{
> -    build_package(package, op, 1);
> -    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> -}
> -
> -static void build_append_value(GArray *table, uint32_t value, int size)
> -{
> -    uint8_t prefix;
> -    int i;
> -
> -    switch (size) {
> -    case 1:
> -        prefix = 0x0A; /* BytePrefix */
> -        break;
> -    case 2:
> -        prefix = 0x0B; /* WordPrefix */
> -        break;
> -    case 4:
> -        prefix = 0x0C; /* DWordPrefix */
> -        break;
> -    default:
> -        assert(0);
> -        return;
> -    }
> -    build_append_byte(table, prefix);
> -    for (i = 0; i < size; ++i) {
> -        build_append_byte(table, value & 0xFF);
> -        value = value >> 8;
> -    }
> -}
> -
> -static void build_append_int(GArray *table, uint32_t value)
> -{
> -    if (value == 0x00) {
> -        build_append_byte(table, 0x00); /* ZeroOp */
> -    } else if (value == 0x01) {
> -        build_append_byte(table, 0x01); /* OneOp */
> -    } else if (value <= 0xFF) {
> -        build_append_value(table, value, 1);
> -    } else if (value <= 0xFFFF) {
> -        build_append_value(table, value, 2);
> -    } else {
> -        build_append_value(table, value, 4);
> -    }
> -}
> -
>  static GArray *build_alloc_method(const char *name, uint8_t arg_count)
>  {
>      GArray *method = build_alloc_array();
> diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> new file mode 100644
> index 0000000..e6a0b28
> --- /dev/null
> +++ b/include/hw/acpi/acpi_gen_utils.h
> @@ -0,0 +1,23 @@
> +#ifndef HW_ACPI_GEN_UTILS_H
> +#define HW_ACPI_GEN_UTILS_H
> +
> +#include <stdint.h>
> +#include <glib.h>
> +#include "qemu/compiler.h"
> +
> +GArray *build_alloc_array(void);
> +void build_free_array(GArray *array);
> +void build_prepend_byte(GArray *array, uint8_t val);
> +void build_append_byte(GArray *array, uint8_t val);
> +void build_append_array(GArray *array, GArray *val);
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_nameseg(GArray *array, const char *format, ...);
> +
> +void build_prepend_package_length(GArray *package, unsigned min_bytes);
> +void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> +void build_append_value(GArray *table, uint32_t value, int size);
> +void build_append_int(GArray *table, uint32_t value);
> +void build_extop_package(GArray *package, uint8_t op);
> +
> +#endif
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 5/9] acpi: move generic aml building helpers into dedictated file

[Igor Mammedov]

On Mon, 8 Dec 2014 22:43:40 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 08, 2014 at 04:08:04PM +0000, Igor Mammedov wrote:
> > the will be later used for composing AML primitives
> > and all that could be reused later for ARM machines
> > as well.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> It will be easy to move when needed.
> Why do it now?
I'm trying to reduce already huge dynamic AML series,
and moving this now is not any way worse than moving it
later. Since it should be moved anyway for ARM machines
to reuse.

> 
> 
> > ---
> >  hw/acpi/Makefile.objs            |   1 +
> >  hw/acpi/acpi_gen_utils.c         | 169 +++++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c             | 165 +-------------------------------------
> >  include/hw/acpi/acpi_gen_utils.h |  23 ++++++
> >  4 files changed, 195 insertions(+), 163 deletions(-)
> >  create mode 100644 hw/acpi/acpi_gen_utils.c
> >  create mode 100644 include/hw/acpi/acpi_gen_utils.h
> > 
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index acd2389..4237232 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -1,3 +1,4 @@
> >  common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> >  common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> >  common-obj-$(CONFIG_ACPI) += acpi_interface.o
> > +common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
> > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > new file mode 100644
> > index 0000000..1583b35
> > --- /dev/null
> > +++ b/hw/acpi/acpi_gen_utils.c
> > @@ -0,0 +1,169 @@
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <assert.h>
> > +#include <stdbool.h>
> > +#include "hw/acpi/acpi_gen_utils.h"
> > +
> > +GArray *build_alloc_array(void)
> > +{
> > +        return g_array_new(false, true /* clear */, 1);
> > +}
> > +
> > +void build_free_array(GArray *array)
> > +{
> > +        g_array_free(array, true);
> > +}
> > +
> > +void build_prepend_byte(GArray *array, uint8_t val)
> > +{
> > +    g_array_prepend_val(array, val);
> > +}
> > +
> > +void build_append_byte(GArray *array, uint8_t val)
> > +{
> > +    g_array_append_val(array, val);
> > +}
> > +
> > +void build_append_array(GArray *array, GArray *val)
> > +{
> > +    g_array_append_vals(array, val->data, val->len);
> > +}
> > +
> > +#define ACPI_NAMESEG_LEN 4
> > +
> > +void GCC_FMT_ATTR(2, 3)
> > +build_append_nameseg(GArray *array, const char *format, ...)
> > +{
> > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > +    char s[] = "XXXX";
> > +    int len;
> > +    va_list args;
> > +    const char padding = '_';
> > +
> > +    va_start(args, format);
> > +    len = vsnprintf(s, sizeof s, format, args);
> > +    va_end(args);
> > +
> > +    g_assert(len <= ACPI_NAMESEG_LEN);
> > +
> > +    g_array_append_vals(array, s, len);
> > +    while (len != ACPI_NAMESEG_LEN) {
> > +        g_array_append_val(array, padding);
> > +        ++len;
> > +    }
> > +}
> > +
> > +/* 5.4 Definition Block Encoding */
> > +enum {
> > +    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > +    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> > +    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> > +    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> > +};
> > +
> > +void build_prepend_package_length(GArray *package, unsigned min_bytes)
> > +{
> > +    uint8_t byte;
> > +    unsigned length = package->len;
> > +    unsigned length_bytes;
> > +
> > +    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> > +        length_bytes = 1;
> > +    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> > +        length_bytes = 2;
> > +    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> > +        length_bytes = 3;
> > +    } else {
> > +        length_bytes = 4;
> > +    }
> > +
> > +    /* Force length to at least min_bytes.
> > +     * This wastes memory but that's how bios did it.
> > +     */
> > +    length_bytes = MAX(length_bytes, min_bytes);
> > +
> > +    /* PkgLength is the length of the inclusive length of the data. */
> > +    length += length_bytes;
> > +
> > +    switch (length_bytes) {
> > +    case 1:
> > +        byte = length;
> > +        build_prepend_byte(package, byte);
> > +        return;
> > +    case 4:
> > +        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> > +        build_prepend_byte(package, byte);
> > +        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> > +        /* fall through */
> > +    case 3:
> > +        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> > +        build_prepend_byte(package, byte);
> > +        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> > +        /* fall through */
> > +    case 2:
> > +        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> > +        build_prepend_byte(package, byte);
> > +        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> > +        /* fall through */
> > +    }
> > +    /*
> > +     * Most significant two bits of byte zero indicate how many following bytes
> > +     * are in PkgLength encoding.
> > +     */
> > +    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> > +    build_prepend_byte(package, byte);
> > +}
> > +
> > +void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> > +{
> > +    build_prepend_package_length(package, min_bytes);
> > +    build_prepend_byte(package, op);
> > +}
> > +
> > +void build_extop_package(GArray *package, uint8_t op)
> > +{
> > +    build_package(package, op, 1);
> > +    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> > +}
> > +
> > +void build_append_value(GArray *table, uint32_t value, int size)
> > +{
> > +    uint8_t prefix;
> > +    int i;
> > +
> > +    switch (size) {
> > +    case 1:
> > +        prefix = 0x0A; /* BytePrefix */
> > +        break;
> > +    case 2:
> > +        prefix = 0x0B; /* WordPrefix */
> > +        break;
> > +    case 4:
> > +        prefix = 0x0C; /* DWordPrefix */
> > +        break;
> > +    default:
> > +        assert(0);
> > +        return;
> > +    }
> > +    build_append_byte(table, prefix);
> > +    for (i = 0; i < size; ++i) {
> > +        build_append_byte(table, value & 0xFF);
> > +        value = value >> 8;
> > +    }
> > +}
> > +
> > +void build_append_int(GArray *table, uint32_t value)
> > +{
> > +    if (value == 0x00) {
> > +        build_append_byte(table, 0x00); /* ZeroOp */
> > +    } else if (value == 0x01) {
> > +        build_append_byte(table, 0x01); /* OneOp */
> > +    } else if (value <= 0xFF) {
> > +        build_append_value(table, value, 1);
> > +    } else if (value <= 0xFFFF) {
> > +        build_append_value(table, value, 2);
> > +    } else {
> > +        build_append_value(table, value, 4);
> > +    }
> > +}
> > +
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index a8b7a2b..870e4a0 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -54,6 +54,8 @@
> >  #include "hw/i386/q35-acpi-dsdt.hex"
> >  #include "hw/i386/acpi-dsdt.hex"
> >  
> > +#include "hw/acpi/acpi_gen_utils.h"
> > +
> >  #include "qapi/qmp/qint.h"
> >  #include "qom/qom-qobject.h"
> >  #include "exec/ram_addr.h"
> > @@ -267,169 +269,6 @@ build_header(GArray *linker, GArray *table_data,
> >                                      table_data->data, h, len, &h->checksum);
> >  }
> >  
> > -static inline GArray *build_alloc_array(void)
> > -{
> > -        return g_array_new(false, true /* clear */, 1);
> > -}
> > -
> > -static inline void build_free_array(GArray *array)
> > -{
> > -        g_array_free(array, true);
> > -}
> > -
> > -static inline void build_prepend_byte(GArray *array, uint8_t val)
> > -{
> > -    g_array_prepend_val(array, val);
> > -}
> > -
> > -static inline void build_append_byte(GArray *array, uint8_t val)
> > -{
> > -    g_array_append_val(array, val);
> > -}
> > -
> > -static inline void build_append_array(GArray *array, GArray *val)
> > -{
> > -    g_array_append_vals(array, val->data, val->len);
> > -}
> > -
> > -#define ACPI_NAMESEG_LEN 4
> > -
> > -static void GCC_FMT_ATTR(2, 3)
> > -build_append_nameseg(GArray *array, const char *format, ...)
> > -{
> > -    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > -    char s[] = "XXXX";
> > -    int len;
> > -    va_list args;
> > -    const char padding = '_';
> > -
> > -    va_start(args, format);
> > -    len = vsnprintf(s, sizeof s, format, args);
> > -    va_end(args);
> > -
> > -    g_assert(len <= ACPI_NAMESEG_LEN);
> > -
> > -    g_array_append_vals(array, s, len);
> > -    while (len != ACPI_NAMESEG_LEN) {
> > -        g_array_append_val(array, padding);
> > -        ++len;
> > -    }
> > -}
> > -
> > -/* 5.4 Definition Block Encoding */
> > -enum {
> > -    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > -    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> > -    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> > -    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> > -};
> > -
> > -static void build_prepend_package_length(GArray *package, unsigned min_bytes)
> > -{
> > -    uint8_t byte;
> > -    unsigned length = package->len;
> > -    unsigned length_bytes;
> > -
> > -    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> > -        length_bytes = 1;
> > -    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> > -        length_bytes = 2;
> > -    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> > -        length_bytes = 3;
> > -    } else {
> > -        length_bytes = 4;
> > -    }
> > -
> > -    /* Force length to at least min_bytes.
> > -     * This wastes memory but that's how bios did it.
> > -     */
> > -    length_bytes = MAX(length_bytes, min_bytes);
> > -
> > -    /* PkgLength is the length of the inclusive length of the data. */
> > -    length += length_bytes;
> > -
> > -    switch (length_bytes) {
> > -    case 1:
> > -        byte = length;
> > -        build_prepend_byte(package, byte);
> > -        return;
> > -    case 4:
> > -        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> > -        build_prepend_byte(package, byte);
> > -        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> > -        /* fall through */
> > -    case 3:
> > -        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> > -        build_prepend_byte(package, byte);
> > -        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> > -        /* fall through */
> > -    case 2:
> > -        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> > -        build_prepend_byte(package, byte);
> > -        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> > -        /* fall through */
> > -    }
> > -    /*
> > -     * Most significant two bits of byte zero indicate how many following bytes
> > -     * are in PkgLength encoding.
> > -     */
> > -    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> > -    build_prepend_byte(package, byte);
> > -}
> > -
> > -static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> > -{
> > -    build_prepend_package_length(package, min_bytes);
> > -    build_prepend_byte(package, op);
> > -}
> > -
> > -static void build_extop_package(GArray *package, uint8_t op)
> > -{
> > -    build_package(package, op, 1);
> > -    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> > -}
> > -
> > -static void build_append_value(GArray *table, uint32_t value, int size)
> > -{
> > -    uint8_t prefix;
> > -    int i;
> > -
> > -    switch (size) {
> > -    case 1:
> > -        prefix = 0x0A; /* BytePrefix */
> > -        break;
> > -    case 2:
> > -        prefix = 0x0B; /* WordPrefix */
> > -        break;
> > -    case 4:
> > -        prefix = 0x0C; /* DWordPrefix */
> > -        break;
> > -    default:
> > -        assert(0);
> > -        return;
> > -    }
> > -    build_append_byte(table, prefix);
> > -    for (i = 0; i < size; ++i) {
> > -        build_append_byte(table, value & 0xFF);
> > -        value = value >> 8;
> > -    }
> > -}
> > -
> > -static void build_append_int(GArray *table, uint32_t value)
> > -{
> > -    if (value == 0x00) {
> > -        build_append_byte(table, 0x00); /* ZeroOp */
> > -    } else if (value == 0x01) {
> > -        build_append_byte(table, 0x01); /* OneOp */
> > -    } else if (value <= 0xFF) {
> > -        build_append_value(table, value, 1);
> > -    } else if (value <= 0xFFFF) {
> > -        build_append_value(table, value, 2);
> > -    } else {
> > -        build_append_value(table, value, 4);
> > -    }
> > -}
> > -
> >  static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> >  {
> >      GArray *method = build_alloc_array();
> > diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > new file mode 100644
> > index 0000000..e6a0b28
> > --- /dev/null
> > +++ b/include/hw/acpi/acpi_gen_utils.h
> > @@ -0,0 +1,23 @@
> > +#ifndef HW_ACPI_GEN_UTILS_H
> > +#define HW_ACPI_GEN_UTILS_H
> > +
> > +#include <stdint.h>
> > +#include <glib.h>
> > +#include "qemu/compiler.h"
> > +
> > +GArray *build_alloc_array(void);
> > +void build_free_array(GArray *array);
> > +void build_prepend_byte(GArray *array, uint8_t val);
> > +void build_append_byte(GArray *array, uint8_t val);
> > +void build_append_array(GArray *array, GArray *val);
> > +
> > +void GCC_FMT_ATTR(2, 3)
> > +build_append_nameseg(GArray *array, const char *format, ...);
> > +
> > +void build_prepend_package_length(GArray *package, unsigned min_bytes);
> > +void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> > +void build_append_value(GArray *table, uint32_t value, int size);
> > +void build_append_int(GArray *table, uint32_t value);
> > +void build_extop_package(GArray *package, uint8_t op);
> > +
> > +#endif
> > -- 
> > 1.8.3.1
> 

[Qemu-devel] [PATCH 6/9] acpi: add build_append_namestring() helper

[Igor Mammedov]

Use build_append_namestring() instead of build_append_nameseg()
So user won't have to care whether name is NameSeg, NamePath or
NameString.

See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/acpi_gen_utils.c         | 86 +++++++++++++++++++++++++++++++++++++++-
 hw/i386/acpi-build.c             | 35 ++++++++--------
 include/hw/acpi/acpi_gen_utils.h |  2 +-
 3 files changed, 102 insertions(+), 21 deletions(-)

diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
index 1583b35..31e2694 100644
--- a/hw/acpi/acpi_gen_utils.c
+++ b/hw/acpi/acpi_gen_utils.c
@@ -31,7 +31,7 @@ void build_append_array(GArray *array, GArray *val)
 
 #define ACPI_NAMESEG_LEN 4
 
-void GCC_FMT_ATTR(2, 3)
+static void GCC_FMT_ATTR(2, 3)
 build_append_nameseg(GArray *array, const char *format, ...)
 {
     /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
@@ -53,6 +53,90 @@ build_append_nameseg(GArray *array, const char *format, ...)
     }
 }
 
+static const uint8_t ACPI_DualNamePrefix  = 0x2E;
+static const uint8_t ACPI_MultiNamePrefix = 0x2F;
+
+static void
+build_append_namestringv(GArray *array, const char *format, va_list ap)
+{
+    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+    char *s;
+    int len;
+    va_list va_len;
+    char **segs;
+    char **segs_iter;
+    int seg_count = 0;
+
+    va_copy(va_len, ap);
+    len = vsnprintf(NULL, 0, format, va_len);
+    va_end(va_len);
+    len += 1;
+    s = g_new(typeof(*s), len);
+
+    len = vsnprintf(s, len, format, ap);
+
+    segs = g_strsplit(s, ".", 0);
+    g_free(s);
+
+    /* count segments */
+    segs_iter = segs;
+    while (*segs_iter) {
+        ++segs_iter;
+        ++seg_count;
+    }
+
+    /* handle RootPath || PrefixPath */
+    s = *segs;
+    while (*s == '\\' || *s == '^') {
+        g_array_append_val(array, *s);
+        ++s;
+    }
+
+    switch (seg_count) {
+    case 1:
+        if (!*s) { /* NullName */
+            const uint8_t nullpath = 0;
+            g_array_append_val(array, nullpath);
+        } else {
+            build_append_nameseg(array, "%s", s);
+        }
+        break;
+
+    case 2:
+        g_array_append_val(array, ACPI_DualNamePrefix);
+        build_append_nameseg(array, "%s", s);
+        build_append_nameseg(array, "%s", segs[1]);
+        break;
+
+    default:
+        g_array_append_val(array, ACPI_MultiNamePrefix);
+        assert(seg_count <= 0xFF); /* ByteData Max */
+        g_array_append_val(array, seg_count);
+
+        /* handle the 1st segment manually due to prefix/root path */
+        build_append_nameseg(array, "%s", s);
+
+        /* add the rest of segments */
+        segs_iter = segs + 1;
+        while (*segs_iter) {
+            build_append_nameseg(array, "%s", *segs_iter);
+            ++segs_iter;
+        }
+        break;
+    }
+    g_strfreev(segs);
+}
+
+void GCC_FMT_ATTR(2, 3)
+build_append_namestring(GArray *array, const char *format, ...)
+{
+    va_list ap;
+
+    va_start(ap, format);
+    build_append_namestringv(array, format, ap);
+    va_end(ap);
+}
+
 /* 5.4 Definition Block Encoding */
 enum {
     PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 870e4a0..0f6202d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -273,7 +273,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
 {
     GArray *method = build_alloc_array();
 
-    build_append_nameseg(method, "%s", name);
+    build_append_namestring(method, "%s", name);
     build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
 
     return method;
@@ -565,7 +565,7 @@ build_append_notify_method(GArray *device, const char *name,
 
     for (i = 0; i < count; i++) {
         GArray *target = build_alloc_array();
-        build_append_nameseg(target, format, i);
+        build_append_namestring(target, format, i);
         assert(i < 256); /* Fits in 1 byte */
         build_append_notify_target_ifequal(method, target, i, 1);
         build_free_array(target);
@@ -693,24 +693,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 
     if (bus->parent_dev) {
         op = 0x82; /* DeviceOp */
-        build_append_nameseg(bus_table, "S%.02X",
+        build_append_namestring(bus_table, "S%.02X",
                              bus->parent_dev->devfn);
         build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_nameseg(bus_table, "_SUN");
+        build_append_namestring(bus_table, "_SUN");
         build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
         build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_nameseg(bus_table, "_ADR");
+        build_append_namestring(bus_table, "_ADR");
         build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
                            PCI_FUNC(bus->parent_dev->devfn), 4);
     } else {
         op = 0x10; /* ScopeOp */;
-        build_append_nameseg(bus_table, "PCI0");
+        build_append_namestring(bus_table, "PCI0");
     }
 
     bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
     if (bsel) {
         build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_nameseg(bus_table, "BSEL");
+        build_append_namestring(bus_table, "BSEL");
         build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
         memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
     } else {
@@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             build_append_int(notify, 0x1U << i);
             build_append_byte(notify, 0x00); /* NullName */
             build_append_byte(notify, 0x86); /* NotifyOp */
-            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
+            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
             build_append_byte(notify, 0x69); /* Arg1Op */
 
             /* Pack it up */
@@ -837,12 +837,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         if (bsel) {
             build_append_byte(method, 0x70); /* StoreOp */
             build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
-            build_append_nameseg(method, "BNUM");
-            build_append_nameseg(method, "DVNT");
-            build_append_nameseg(method, "PCIU");
+            build_append_namestring(method, "BNUM");
+            build_append_namestring(method, "DVNT");
+            build_append_namestring(method, "PCIU");
             build_append_int(method, 1); /* Device Check */
-            build_append_nameseg(method, "DVNT");
-            build_append_nameseg(method, "PCID");
+            build_append_namestring(method, "DVNT");
+            build_append_namestring(method, "PCID");
             build_append_int(method, 3); /* Eject Request */
         }
 
@@ -868,11 +868,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
          * At the moment this is not needed for root as we have a single root.
          */
         if (bus->parent_dev) {
-            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
-            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
-            build_append_nameseg(parent->notify_table, "S%.02X",
+            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
                                  bus->parent_dev->devfn);
-            build_append_nameseg(parent->notify_table, "PCNT");
         }
     }
 
@@ -940,7 +937,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         GArray *sb_scope = build_alloc_array();
         uint8_t op = 0x10; /* ScopeOp */
 
-        build_append_nameseg(sb_scope, "_SB");
+        build_append_namestring(sb_scope, "_SB");
 
         /* build Processor object for each processor */
         for (i = 0; i < acpi_cpus; i++) {
@@ -960,7 +957,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 
         /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
         build_append_byte(sb_scope, 0x08); /* NameOp */
-        build_append_nameseg(sb_scope, "CPON");
+        build_append_namestring(sb_scope, "CPON");
 
         {
             GArray *package = build_alloc_array();
diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
index e6a0b28..fd50625 100644
--- a/include/hw/acpi/acpi_gen_utils.h
+++ b/include/hw/acpi/acpi_gen_utils.h
@@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
 void build_append_array(GArray *array, GArray *val);
 
 void GCC_FMT_ATTR(2, 3)
-build_append_nameseg(GArray *array, const char *format, ...);
+build_append_namestring(GArray *array, const char *format, ...);
 
 void build_prepend_package_length(GArray *package, unsigned min_bytes);
 void build_package(GArray *package, uint8_t op, unsigned min_bytes);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 6/9] acpi: add build_append_namestring() helper

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:05PM +0000, Igor Mammedov wrote:
> Use build_append_namestring() instead of build_append_nameseg()
> So user won't have to care whether name is NameSeg, NamePath or
> NameString.
> 
> See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Well that's wasteful since most places do have simple 4 byte names.
How about we replace where this is needed?


> ---
>  hw/acpi/acpi_gen_utils.c         | 86 +++++++++++++++++++++++++++++++++++++++-
>  hw/i386/acpi-build.c             | 35 ++++++++--------
>  include/hw/acpi/acpi_gen_utils.h |  2 +-
>  3 files changed, 102 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> index 1583b35..31e2694 100644
> --- a/hw/acpi/acpi_gen_utils.c
> +++ b/hw/acpi/acpi_gen_utils.c
> @@ -31,7 +31,7 @@ void build_append_array(GArray *array, GArray *val)
>  
>  #define ACPI_NAMESEG_LEN 4
>  
> -void GCC_FMT_ATTR(2, 3)
> +static void GCC_FMT_ATTR(2, 3)
>  build_append_nameseg(GArray *array, const char *format, ...)
>  {
>      /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> @@ -53,6 +53,90 @@ build_append_nameseg(GArray *array, const char *format, ...)
>      }
>  }
>  
> +static const uint8_t ACPI_DualNamePrefix  = 0x2E;
> +static const uint8_t ACPI_MultiNamePrefix = 0x2F;
> +
> +static void
> +build_append_namestringv(GArray *array, const char *format, va_list ap)
> +{
> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> +    char *s;
> +    int len;
> +    va_list va_len;
> +    char **segs;
> +    char **segs_iter;
> +    int seg_count = 0;
> +
> +    va_copy(va_len, ap);
> +    len = vsnprintf(NULL, 0, format, va_len);
> +    va_end(va_len);
> +    len += 1;
> +    s = g_new(typeof(*s), len);
> +
> +    len = vsnprintf(s, len, format, ap);
> +
> +    segs = g_strsplit(s, ".", 0);
> +    g_free(s);
> +
> +    /* count segments */
> +    segs_iter = segs;
> +    while (*segs_iter) {
> +        ++segs_iter;
> +        ++seg_count;
> +    }
> +
> +    /* handle RootPath || PrefixPath */
> +    s = *segs;
> +    while (*s == '\\' || *s == '^') {
> +        g_array_append_val(array, *s);
> +        ++s;
> +    }
> +
> +    switch (seg_count) {
> +    case 1:
> +        if (!*s) { /* NullName */
> +            const uint8_t nullpath = 0;
> +            g_array_append_val(array, nullpath);
> +        } else {
> +            build_append_nameseg(array, "%s", s);
> +        }
> +        break;
> +
> +    case 2:
> +        g_array_append_val(array, ACPI_DualNamePrefix);
> +        build_append_nameseg(array, "%s", s);
> +        build_append_nameseg(array, "%s", segs[1]);
> +        break;
> +
> +    default:
> +        g_array_append_val(array, ACPI_MultiNamePrefix);
> +        assert(seg_count <= 0xFF); /* ByteData Max */
> +        g_array_append_val(array, seg_count);
> +
> +        /* handle the 1st segment manually due to prefix/root path */
> +        build_append_nameseg(array, "%s", s);
> +
> +        /* add the rest of segments */
> +        segs_iter = segs + 1;
> +        while (*segs_iter) {
> +            build_append_nameseg(array, "%s", *segs_iter);
> +            ++segs_iter;
> +        }
> +        break;
> +    }
> +    g_strfreev(segs);
> +}
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_namestring(GArray *array, const char *format, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, format);
> +    build_append_namestringv(array, format, ap);
> +    va_end(ap);
> +}
> +
>  /* 5.4 Definition Block Encoding */
>  enum {
>      PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 870e4a0..0f6202d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -273,7 +273,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
>  {
>      GArray *method = build_alloc_array();
>  
> -    build_append_nameseg(method, "%s", name);
> +    build_append_namestring(method, "%s", name);
>      build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
>  
>      return method;
> @@ -565,7 +565,7 @@ build_append_notify_method(GArray *device, const char *name,
>  
>      for (i = 0; i < count; i++) {
>          GArray *target = build_alloc_array();
> -        build_append_nameseg(target, format, i);
> +        build_append_namestring(target, format, i);
>          assert(i < 256); /* Fits in 1 byte */
>          build_append_notify_target_ifequal(method, target, i, 1);
>          build_free_array(target);
> @@ -693,24 +693,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>  
>      if (bus->parent_dev) {
>          op = 0x82; /* DeviceOp */
> -        build_append_nameseg(bus_table, "S%.02X",
> +        build_append_namestring(bus_table, "S%.02X",
>                               bus->parent_dev->devfn);
>          build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_nameseg(bus_table, "_SUN");
> +        build_append_namestring(bus_table, "_SUN");
>          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
>          build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_nameseg(bus_table, "_ADR");
> +        build_append_namestring(bus_table, "_ADR");
>          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
>                             PCI_FUNC(bus->parent_dev->devfn), 4);
>      } else {
>          op = 0x10; /* ScopeOp */;
> -        build_append_nameseg(bus_table, "PCI0");
> +        build_append_namestring(bus_table, "PCI0");
>      }
>  
>      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
>      if (bsel) {
>          build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_nameseg(bus_table, "BSEL");
> +        build_append_namestring(bus_table, "BSEL");
>          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
>          memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
>      } else {
> @@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>              build_append_int(notify, 0x1U << i);
>              build_append_byte(notify, 0x00); /* NullName */
>              build_append_byte(notify, 0x86); /* NotifyOp */
> -            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> +            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
>              build_append_byte(notify, 0x69); /* Arg1Op */
>  
>              /* Pack it up */
> @@ -837,12 +837,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>          if (bsel) {
>              build_append_byte(method, 0x70); /* StoreOp */
>              build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> -            build_append_nameseg(method, "BNUM");
> -            build_append_nameseg(method, "DVNT");
> -            build_append_nameseg(method, "PCIU");
> +            build_append_namestring(method, "BNUM");
> +            build_append_namestring(method, "DVNT");
> +            build_append_namestring(method, "PCIU");
>              build_append_int(method, 1); /* Device Check */
> -            build_append_nameseg(method, "DVNT");
> -            build_append_nameseg(method, "PCID");
> +            build_append_namestring(method, "DVNT");
> +            build_append_namestring(method, "PCID");
>              build_append_int(method, 3); /* Eject Request */
>          }
>  
> @@ -868,11 +868,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>           * At the moment this is not needed for root as we have a single root.
>           */
>          if (bus->parent_dev) {
> -            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> -            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> -            build_append_nameseg(parent->notify_table, "S%.02X",
> +            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
>                                   bus->parent_dev->devfn);
> -            build_append_nameseg(parent->notify_table, "PCNT");
>          }
>      }
>  
> @@ -940,7 +937,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          GArray *sb_scope = build_alloc_array();
>          uint8_t op = 0x10; /* ScopeOp */
>  
> -        build_append_nameseg(sb_scope, "_SB");
> +        build_append_namestring(sb_scope, "_SB");
>  
>          /* build Processor object for each processor */
>          for (i = 0; i < acpi_cpus; i++) {
> @@ -960,7 +957,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>  
>          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
>          build_append_byte(sb_scope, 0x08); /* NameOp */
> -        build_append_nameseg(sb_scope, "CPON");
> +        build_append_namestring(sb_scope, "CPON");
>  
>          {
>              GArray *package = build_alloc_array();
> diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> index e6a0b28..fd50625 100644
> --- a/include/hw/acpi/acpi_gen_utils.h
> +++ b/include/hw/acpi/acpi_gen_utils.h
> @@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
>  void build_append_array(GArray *array, GArray *val);
>  
>  void GCC_FMT_ATTR(2, 3)
> -build_append_nameseg(GArray *array, const char *format, ...);
> +build_append_namestring(GArray *array, const char *format, ...);
>  
>  void build_prepend_package_length(GArray *package, unsigned min_bytes);
>  void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 6/9] acpi: add build_append_namestring() helper

[Igor Mammedov]

On Mon, 8 Dec 2014 22:21:20 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 08, 2014 at 04:08:05PM +0000, Igor Mammedov wrote:
> > Use build_append_namestring() instead of build_append_nameseg()
> > So user won't have to care whether name is NameSeg, NamePath or
> > NameString.
> > 
> > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> Well that's wasteful since most places do have simple 4 byte names.
What do you mean under wasteful?

> How about we replace where this is needed?
> 
> 
> > ---
> >  hw/acpi/acpi_gen_utils.c         | 86 +++++++++++++++++++++++++++++++++++++++-
> >  hw/i386/acpi-build.c             | 35 ++++++++--------
> >  include/hw/acpi/acpi_gen_utils.h |  2 +-
> >  3 files changed, 102 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > index 1583b35..31e2694 100644
> > --- a/hw/acpi/acpi_gen_utils.c
> > +++ b/hw/acpi/acpi_gen_utils.c
> > @@ -31,7 +31,7 @@ void build_append_array(GArray *array, GArray *val)
> >  
> >  #define ACPI_NAMESEG_LEN 4
> >  
> > -void GCC_FMT_ATTR(2, 3)
> > +static void GCC_FMT_ATTR(2, 3)
> >  build_append_nameseg(GArray *array, const char *format, ...)
> >  {
> >      /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > @@ -53,6 +53,90 @@ build_append_nameseg(GArray *array, const char *format, ...)
> >      }
> >  }
> >  
> > +static const uint8_t ACPI_DualNamePrefix  = 0x2E;
> > +static const uint8_t ACPI_MultiNamePrefix = 0x2F;
> > +
> > +static void
> > +build_append_namestringv(GArray *array, const char *format, va_list ap)
> > +{
> > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > +    char *s;
> > +    int len;
> > +    va_list va_len;
> > +    char **segs;
> > +    char **segs_iter;
> > +    int seg_count = 0;
> > +
> > +    va_copy(va_len, ap);
> > +    len = vsnprintf(NULL, 0, format, va_len);
> > +    va_end(va_len);
> > +    len += 1;
> > +    s = g_new(typeof(*s), len);
> > +
> > +    len = vsnprintf(s, len, format, ap);
> > +
> > +    segs = g_strsplit(s, ".", 0);
> > +    g_free(s);
> > +
> > +    /* count segments */
> > +    segs_iter = segs;
> > +    while (*segs_iter) {
> > +        ++segs_iter;
> > +        ++seg_count;
> > +    }
> > +
> > +    /* handle RootPath || PrefixPath */
> > +    s = *segs;
> > +    while (*s == '\\' || *s == '^') {
> > +        g_array_append_val(array, *s);
> > +        ++s;
> > +    }
> > +
> > +    switch (seg_count) {
> > +    case 1:
> > +        if (!*s) { /* NullName */
> > +            const uint8_t nullpath = 0;
> > +            g_array_append_val(array, nullpath);
> > +        } else {
> > +            build_append_nameseg(array, "%s", s);
> > +        }
> > +        break;
> > +
> > +    case 2:
> > +        g_array_append_val(array, ACPI_DualNamePrefix);
> > +        build_append_nameseg(array, "%s", s);
> > +        build_append_nameseg(array, "%s", segs[1]);
> > +        break;
> > +
> > +    default:
> > +        g_array_append_val(array, ACPI_MultiNamePrefix);
> > +        assert(seg_count <= 0xFF); /* ByteData Max */
> > +        g_array_append_val(array, seg_count);
> > +
> > +        /* handle the 1st segment manually due to prefix/root path */
> > +        build_append_nameseg(array, "%s", s);
> > +
> > +        /* add the rest of segments */
> > +        segs_iter = segs + 1;
> > +        while (*segs_iter) {
> > +            build_append_nameseg(array, "%s", *segs_iter);
> > +            ++segs_iter;
> > +        }
> > +        break;
> > +    }
> > +    g_strfreev(segs);
> > +}
> > +
> > +void GCC_FMT_ATTR(2, 3)
> > +build_append_namestring(GArray *array, const char *format, ...)
> > +{
> > +    va_list ap;
> > +
> > +    va_start(ap, format);
> > +    build_append_namestringv(array, format, ap);
> > +    va_end(ap);
> > +}
> > +
> >  /* 5.4 Definition Block Encoding */
> >  enum {
> >      PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 870e4a0..0f6202d 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -273,7 +273,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> >  {
> >      GArray *method = build_alloc_array();
> >  
> > -    build_append_nameseg(method, "%s", name);
> > +    build_append_namestring(method, "%s", name);
> >      build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
> >  
> >      return method;
> > @@ -565,7 +565,7 @@ build_append_notify_method(GArray *device, const char *name,
> >  
> >      for (i = 0; i < count; i++) {
> >          GArray *target = build_alloc_array();
> > -        build_append_nameseg(target, format, i);
> > +        build_append_namestring(target, format, i);
> >          assert(i < 256); /* Fits in 1 byte */
> >          build_append_notify_target_ifequal(method, target, i, 1);
> >          build_free_array(target);
> > @@ -693,24 +693,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >  
> >      if (bus->parent_dev) {
> >          op = 0x82; /* DeviceOp */
> > -        build_append_nameseg(bus_table, "S%.02X",
> > +        build_append_namestring(bus_table, "S%.02X",
> >                               bus->parent_dev->devfn);
> >          build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_nameseg(bus_table, "_SUN");
> > +        build_append_namestring(bus_table, "_SUN");
> >          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> >          build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_nameseg(bus_table, "_ADR");
> > +        build_append_namestring(bus_table, "_ADR");
> >          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> >                             PCI_FUNC(bus->parent_dev->devfn), 4);
> >      } else {
> >          op = 0x10; /* ScopeOp */;
> > -        build_append_nameseg(bus_table, "PCI0");
> > +        build_append_namestring(bus_table, "PCI0");
> >      }
> >  
> >      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> >      if (bsel) {
> >          build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_nameseg(bus_table, "BSEL");
> > +        build_append_namestring(bus_table, "BSEL");
> >          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> >          memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> >      } else {
> > @@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >              build_append_int(notify, 0x1U << i);
> >              build_append_byte(notify, 0x00); /* NullName */
> >              build_append_byte(notify, 0x86); /* NotifyOp */
> > -            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> > +            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> >              build_append_byte(notify, 0x69); /* Arg1Op */
> >  
> >              /* Pack it up */
> > @@ -837,12 +837,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >          if (bsel) {
> >              build_append_byte(method, 0x70); /* StoreOp */
> >              build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> > -            build_append_nameseg(method, "BNUM");
> > -            build_append_nameseg(method, "DVNT");
> > -            build_append_nameseg(method, "PCIU");
> > +            build_append_namestring(method, "BNUM");
> > +            build_append_namestring(method, "DVNT");
> > +            build_append_namestring(method, "PCIU");
> >              build_append_int(method, 1); /* Device Check */
> > -            build_append_nameseg(method, "DVNT");
> > -            build_append_nameseg(method, "PCID");
> > +            build_append_namestring(method, "DVNT");
> > +            build_append_namestring(method, "PCID");
> >              build_append_int(method, 3); /* Eject Request */
> >          }
> >  
> > @@ -868,11 +868,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >           * At the moment this is not needed for root as we have a single root.
> >           */
> >          if (bus->parent_dev) {
> > -            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> > -            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> > -            build_append_nameseg(parent->notify_table, "S%.02X",
> > +            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> >                                   bus->parent_dev->devfn);
> > -            build_append_nameseg(parent->notify_table, "PCNT");
> >          }
> >      }
> >  
> > @@ -940,7 +937,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >          GArray *sb_scope = build_alloc_array();
> >          uint8_t op = 0x10; /* ScopeOp */
> >  
> > -        build_append_nameseg(sb_scope, "_SB");
> > +        build_append_namestring(sb_scope, "_SB");
> >  
> >          /* build Processor object for each processor */
> >          for (i = 0; i < acpi_cpus; i++) {
> > @@ -960,7 +957,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >  
> >          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> >          build_append_byte(sb_scope, 0x08); /* NameOp */
> > -        build_append_nameseg(sb_scope, "CPON");
> > +        build_append_namestring(sb_scope, "CPON");
> >  
> >          {
> >              GArray *package = build_alloc_array();
> > diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > index e6a0b28..fd50625 100644
> > --- a/include/hw/acpi/acpi_gen_utils.h
> > +++ b/include/hw/acpi/acpi_gen_utils.h
> > @@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
> >  void build_append_array(GArray *array, GArray *val);
> >  
> >  void GCC_FMT_ATTR(2, 3)
> > -build_append_nameseg(GArray *array, const char *format, ...);
> > +build_append_namestring(GArray *array, const char *format, ...);
> >  
> >  void build_prepend_package_length(GArray *package, unsigned min_bytes);
> >  void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH 6/9] acpi: add build_append_namestring() helper

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 11:39:39AM +0100, Igor Mammedov wrote:
> On Mon, 8 Dec 2014 22:21:20 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Dec 08, 2014 at 04:08:05PM +0000, Igor Mammedov wrote:
> > > Use build_append_namestring() instead of build_append_nameseg()
> > > So user won't have to care whether name is NameSeg, NamePath or
> > > NameString.
> > > 
> > > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > 
> > Well that's wasteful since most places do have simple 4 byte names.
> What do you mean under wasteful?

I think I misunderstood what this patch does:
you mean "select the correct name type based on
the string used"?

I'll have to re-read it, sorry about the noise.

> > How about we replace where this is needed?
> > 
> > 
> > > ---
> > >  hw/acpi/acpi_gen_utils.c         | 86 +++++++++++++++++++++++++++++++++++++++-
> > >  hw/i386/acpi-build.c             | 35 ++++++++--------
> > >  include/hw/acpi/acpi_gen_utils.h |  2 +-
> > >  3 files changed, 102 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > > index 1583b35..31e2694 100644
> > > --- a/hw/acpi/acpi_gen_utils.c
> > > +++ b/hw/acpi/acpi_gen_utils.c
> > > @@ -31,7 +31,7 @@ void build_append_array(GArray *array, GArray *val)
> > >  
> > >  #define ACPI_NAMESEG_LEN 4
> > >  
> > > -void GCC_FMT_ATTR(2, 3)
> > > +static void GCC_FMT_ATTR(2, 3)
> > >  build_append_nameseg(GArray *array, const char *format, ...)
> > >  {
> > >      /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > > @@ -53,6 +53,90 @@ build_append_nameseg(GArray *array, const char *format, ...)
> > >      }
> > >  }
> > >  
> > > +static const uint8_t ACPI_DualNamePrefix  = 0x2E;
> > > +static const uint8_t ACPI_MultiNamePrefix = 0x2F;
> > > +
> > > +static void
> > > +build_append_namestringv(GArray *array, const char *format, va_list ap)
> > > +{
> > > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > > +    char *s;
> > > +    int len;
> > > +    va_list va_len;
> > > +    char **segs;
> > > +    char **segs_iter;
> > > +    int seg_count = 0;
> > > +
> > > +    va_copy(va_len, ap);
> > > +    len = vsnprintf(NULL, 0, format, va_len);
> > > +    va_end(va_len);
> > > +    len += 1;
> > > +    s = g_new(typeof(*s), len);
> > > +
> > > +    len = vsnprintf(s, len, format, ap);
> > > +
> > > +    segs = g_strsplit(s, ".", 0);
> > > +    g_free(s);
> > > +
> > > +    /* count segments */
> > > +    segs_iter = segs;
> > > +    while (*segs_iter) {
> > > +        ++segs_iter;
> > > +        ++seg_count;
> > > +    }
> > > +
> > > +    /* handle RootPath || PrefixPath */
> > > +    s = *segs;
> > > +    while (*s == '\\' || *s == '^') {
> > > +        g_array_append_val(array, *s);
> > > +        ++s;
> > > +    }
> > > +
> > > +    switch (seg_count) {
> > > +    case 1:
> > > +        if (!*s) { /* NullName */
> > > +            const uint8_t nullpath = 0;
> > > +            g_array_append_val(array, nullpath);
> > > +        } else {
> > > +            build_append_nameseg(array, "%s", s);
> > > +        }
> > > +        break;
> > > +
> > > +    case 2:
> > > +        g_array_append_val(array, ACPI_DualNamePrefix);
> > > +        build_append_nameseg(array, "%s", s);
> > > +        build_append_nameseg(array, "%s", segs[1]);
> > > +        break;
> > > +
> > > +    default:
> > > +        g_array_append_val(array, ACPI_MultiNamePrefix);
> > > +        assert(seg_count <= 0xFF); /* ByteData Max */
> > > +        g_array_append_val(array, seg_count);
> > > +
> > > +        /* handle the 1st segment manually due to prefix/root path */
> > > +        build_append_nameseg(array, "%s", s);
> > > +
> > > +        /* add the rest of segments */
> > > +        segs_iter = segs + 1;
> > > +        while (*segs_iter) {
> > > +            build_append_nameseg(array, "%s", *segs_iter);
> > > +            ++segs_iter;
> > > +        }
> > > +        break;
> > > +    }
> > > +    g_strfreev(segs);
> > > +}
> > > +
> > > +void GCC_FMT_ATTR(2, 3)
> > > +build_append_namestring(GArray *array, const char *format, ...)
> > > +{
> > > +    va_list ap;
> > > +
> > > +    va_start(ap, format);
> > > +    build_append_namestringv(array, format, ap);
> > > +    va_end(ap);
> > > +}
> > > +
> > >  /* 5.4 Definition Block Encoding */
> > >  enum {
> > >      PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 870e4a0..0f6202d 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -273,7 +273,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> > >  {
> > >      GArray *method = build_alloc_array();
> > >  
> > > -    build_append_nameseg(method, "%s", name);
> > > +    build_append_namestring(method, "%s", name);
> > >      build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
> > >  
> > >      return method;
> > > @@ -565,7 +565,7 @@ build_append_notify_method(GArray *device, const char *name,
> > >  
> > >      for (i = 0; i < count; i++) {
> > >          GArray *target = build_alloc_array();
> > > -        build_append_nameseg(target, format, i);
> > > +        build_append_namestring(target, format, i);
> > >          assert(i < 256); /* Fits in 1 byte */
> > >          build_append_notify_target_ifequal(method, target, i, 1);
> > >          build_free_array(target);
> > > @@ -693,24 +693,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >  
> > >      if (bus->parent_dev) {
> > >          op = 0x82; /* DeviceOp */
> > > -        build_append_nameseg(bus_table, "S%.02X",
> > > +        build_append_namestring(bus_table, "S%.02X",
> > >                               bus->parent_dev->devfn);
> > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_nameseg(bus_table, "_SUN");
> > > +        build_append_namestring(bus_table, "_SUN");
> > >          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_nameseg(bus_table, "_ADR");
> > > +        build_append_namestring(bus_table, "_ADR");
> > >          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> > >                             PCI_FUNC(bus->parent_dev->devfn), 4);
> > >      } else {
> > >          op = 0x10; /* ScopeOp */;
> > > -        build_append_nameseg(bus_table, "PCI0");
> > > +        build_append_namestring(bus_table, "PCI0");
> > >      }
> > >  
> > >      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> > >      if (bsel) {
> > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_nameseg(bus_table, "BSEL");
> > > +        build_append_namestring(bus_table, "BSEL");
> > >          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> > >          memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> > >      } else {
> > > @@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >              build_append_int(notify, 0x1U << i);
> > >              build_append_byte(notify, 0x00); /* NullName */
> > >              build_append_byte(notify, 0x86); /* NotifyOp */
> > > -            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> > > +            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> > >              build_append_byte(notify, 0x69); /* Arg1Op */
> > >  
> > >              /* Pack it up */
> > > @@ -837,12 +837,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >          if (bsel) {
> > >              build_append_byte(method, 0x70); /* StoreOp */
> > >              build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> > > -            build_append_nameseg(method, "BNUM");
> > > -            build_append_nameseg(method, "DVNT");
> > > -            build_append_nameseg(method, "PCIU");
> > > +            build_append_namestring(method, "BNUM");
> > > +            build_append_namestring(method, "DVNT");
> > > +            build_append_namestring(method, "PCIU");
> > >              build_append_int(method, 1); /* Device Check */
> > > -            build_append_nameseg(method, "DVNT");
> > > -            build_append_nameseg(method, "PCID");
> > > +            build_append_namestring(method, "DVNT");
> > > +            build_append_namestring(method, "PCID");
> > >              build_append_int(method, 3); /* Eject Request */
> > >          }
> > >  
> > > @@ -868,11 +868,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >           * At the moment this is not needed for root as we have a single root.
> > >           */
> > >          if (bus->parent_dev) {
> > > -            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> > > -            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> > > -            build_append_nameseg(parent->notify_table, "S%.02X",
> > > +            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> > >                                   bus->parent_dev->devfn);
> > > -            build_append_nameseg(parent->notify_table, "PCNT");
> > >          }
> > >      }
> > >  
> > > @@ -940,7 +937,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >          GArray *sb_scope = build_alloc_array();
> > >          uint8_t op = 0x10; /* ScopeOp */
> > >  
> > > -        build_append_nameseg(sb_scope, "_SB");
> > > +        build_append_namestring(sb_scope, "_SB");
> > >  
> > >          /* build Processor object for each processor */
> > >          for (i = 0; i < acpi_cpus; i++) {
> > > @@ -960,7 +957,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >  
> > >          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> > >          build_append_byte(sb_scope, 0x08); /* NameOp */
> > > -        build_append_nameseg(sb_scope, "CPON");
> > > +        build_append_namestring(sb_scope, "CPON");
> > >  
> > >          {
> > >              GArray *package = build_alloc_array();
> > > diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > > index e6a0b28..fd50625 100644
> > > --- a/include/hw/acpi/acpi_gen_utils.h
> > > +++ b/include/hw/acpi/acpi_gen_utils.h
> > > @@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
> > >  void build_append_array(GArray *array, GArray *val);
> > >  
> > >  void GCC_FMT_ATTR(2, 3)
> > > -build_append_nameseg(GArray *array, const char *format, ...);
> > > +build_append_namestring(GArray *array, const char *format, ...);
> > >  
> > >  void build_prepend_package_length(GArray *package, unsigned min_bytes);
> > >  void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> > > -- 
> > > 1.8.3.1

Re: [Qemu-devel] [PATCH 6/9] acpi: add build_append_namestring() helper

[Igor Mammedov]

On Tue, 9 Dec 2014 14:02:17 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Dec 09, 2014 at 11:39:39AM +0100, Igor Mammedov wrote:
> > On Mon, 8 Dec 2014 22:21:20 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > 
> > > On Mon, Dec 08, 2014 at 04:08:05PM +0000, Igor Mammedov wrote:
> > > > Use build_append_namestring() instead of build_append_nameseg()
> > > > So user won't have to care whether name is NameSeg, NamePath or
> > > > NameString.
> > > > 
> > > > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > > > 
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > 
> > > Well that's wasteful since most places do have simple 4 byte names.
> > What do you mean under wasteful?
> 
> I think I misunderstood what this patch does:
> you mean "select the correct name type based on
> the string used"?
Yep, it chooses appropriate type depending on input.

> 
> I'll have to re-read it, sorry about the noise.
> 
> > > How about we replace where this is needed?
> > > 
> > > 
> > > > ---
> > > >  hw/acpi/acpi_gen_utils.c         | 86 +++++++++++++++++++++++++++++++++++++++-
> > > >  hw/i386/acpi-build.c             | 35 ++++++++--------
> > > >  include/hw/acpi/acpi_gen_utils.h |  2 +-
> > > >  3 files changed, 102 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > > > index 1583b35..31e2694 100644
> > > > --- a/hw/acpi/acpi_gen_utils.c
> > > > +++ b/hw/acpi/acpi_gen_utils.c
> > > > @@ -31,7 +31,7 @@ void build_append_array(GArray *array, GArray *val)
> > > >  
> > > >  #define ACPI_NAMESEG_LEN 4
> > > >  
> > > > -void GCC_FMT_ATTR(2, 3)
> > > > +static void GCC_FMT_ATTR(2, 3)
> > > >  build_append_nameseg(GArray *array, const char *format, ...)
> > > >  {
> > > >      /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > > > @@ -53,6 +53,90 @@ build_append_nameseg(GArray *array, const char *format, ...)
> > > >      }
> > > >  }
> > > >  
> > > > +static const uint8_t ACPI_DualNamePrefix  = 0x2E;
> > > > +static const uint8_t ACPI_MultiNamePrefix = 0x2F;
> > > > +
> > > > +static void
> > > > +build_append_namestringv(GArray *array, const char *format, va_list ap)
> > > > +{
> > > > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > > > +    char *s;
> > > > +    int len;
> > > > +    va_list va_len;
> > > > +    char **segs;
> > > > +    char **segs_iter;
> > > > +    int seg_count = 0;
> > > > +
> > > > +    va_copy(va_len, ap);
> > > > +    len = vsnprintf(NULL, 0, format, va_len);
> > > > +    va_end(va_len);
> > > > +    len += 1;
> > > > +    s = g_new(typeof(*s), len);
> > > > +
> > > > +    len = vsnprintf(s, len, format, ap);
> > > > +
> > > > +    segs = g_strsplit(s, ".", 0);
> > > > +    g_free(s);
> > > > +
> > > > +    /* count segments */
> > > > +    segs_iter = segs;
> > > > +    while (*segs_iter) {
> > > > +        ++segs_iter;
> > > > +        ++seg_count;
> > > > +    }
> > > > +
> > > > +    /* handle RootPath || PrefixPath */
> > > > +    s = *segs;
> > > > +    while (*s == '\\' || *s == '^') {
> > > > +        g_array_append_val(array, *s);
> > > > +        ++s;
> > > > +    }
> > > > +
> > > > +    switch (seg_count) {
> > > > +    case 1:
> > > > +        if (!*s) { /* NullName */
> > > > +            const uint8_t nullpath = 0;
> > > > +            g_array_append_val(array, nullpath);
> > > > +        } else {
> > > > +            build_append_nameseg(array, "%s", s);
> > > > +        }
> > > > +        break;
> > > > +
> > > > +    case 2:
> > > > +        g_array_append_val(array, ACPI_DualNamePrefix);
> > > > +        build_append_nameseg(array, "%s", s);
> > > > +        build_append_nameseg(array, "%s", segs[1]);
> > > > +        break;
> > > > +
> > > > +    default:
> > > > +        g_array_append_val(array, ACPI_MultiNamePrefix);
> > > > +        assert(seg_count <= 0xFF); /* ByteData Max */
> > > > +        g_array_append_val(array, seg_count);
> > > > +
> > > > +        /* handle the 1st segment manually due to prefix/root path */
> > > > +        build_append_nameseg(array, "%s", s);
> > > > +
> > > > +        /* add the rest of segments */
> > > > +        segs_iter = segs + 1;
> > > > +        while (*segs_iter) {
> > > > +            build_append_nameseg(array, "%s", *segs_iter);
> > > > +            ++segs_iter;
> > > > +        }
> > > > +        break;
> > > > +    }
> > > > +    g_strfreev(segs);
> > > > +}
> > > > +
> > > > +void GCC_FMT_ATTR(2, 3)
> > > > +build_append_namestring(GArray *array, const char *format, ...)
> > > > +{
> > > > +    va_list ap;
> > > > +
> > > > +    va_start(ap, format);
> > > > +    build_append_namestringv(array, format, ap);
> > > > +    va_end(ap);
> > > > +}
> > > > +
> > > >  /* 5.4 Definition Block Encoding */
> > > >  enum {
> > > >      PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > > index 870e4a0..0f6202d 100644
> > > > --- a/hw/i386/acpi-build.c
> > > > +++ b/hw/i386/acpi-build.c
> > > > @@ -273,7 +273,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> > > >  {
> > > >      GArray *method = build_alloc_array();
> > > >  
> > > > -    build_append_nameseg(method, "%s", name);
> > > > +    build_append_namestring(method, "%s", name);
> > > >      build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
> > > >  
> > > >      return method;
> > > > @@ -565,7 +565,7 @@ build_append_notify_method(GArray *device, const char *name,
> > > >  
> > > >      for (i = 0; i < count; i++) {
> > > >          GArray *target = build_alloc_array();
> > > > -        build_append_nameseg(target, format, i);
> > > > +        build_append_namestring(target, format, i);
> > > >          assert(i < 256); /* Fits in 1 byte */
> > > >          build_append_notify_target_ifequal(method, target, i, 1);
> > > >          build_free_array(target);
> > > > @@ -693,24 +693,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > >  
> > > >      if (bus->parent_dev) {
> > > >          op = 0x82; /* DeviceOp */
> > > > -        build_append_nameseg(bus_table, "S%.02X",
> > > > +        build_append_namestring(bus_table, "S%.02X",
> > > >                               bus->parent_dev->devfn);
> > > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > > -        build_append_nameseg(bus_table, "_SUN");
> > > > +        build_append_namestring(bus_table, "_SUN");
> > > >          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> > > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > > -        build_append_nameseg(bus_table, "_ADR");
> > > > +        build_append_namestring(bus_table, "_ADR");
> > > >          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> > > >                             PCI_FUNC(bus->parent_dev->devfn), 4);
> > > >      } else {
> > > >          op = 0x10; /* ScopeOp */;
> > > > -        build_append_nameseg(bus_table, "PCI0");
> > > > +        build_append_namestring(bus_table, "PCI0");
> > > >      }
> > > >  
> > > >      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> > > >      if (bsel) {
> > > >          build_append_byte(bus_table, 0x08); /* NameOp */
> > > > -        build_append_nameseg(bus_table, "BSEL");
> > > > +        build_append_namestring(bus_table, "BSEL");
> > > >          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> > > >          memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> > > >      } else {
> > > > @@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > >              build_append_int(notify, 0x1U << i);
> > > >              build_append_byte(notify, 0x00); /* NullName */
> > > >              build_append_byte(notify, 0x86); /* NotifyOp */
> > > > -            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> > > > +            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> > > >              build_append_byte(notify, 0x69); /* Arg1Op */
> > > >  
> > > >              /* Pack it up */
> > > > @@ -837,12 +837,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > >          if (bsel) {
> > > >              build_append_byte(method, 0x70); /* StoreOp */
> > > >              build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> > > > -            build_append_nameseg(method, "BNUM");
> > > > -            build_append_nameseg(method, "DVNT");
> > > > -            build_append_nameseg(method, "PCIU");
> > > > +            build_append_namestring(method, "BNUM");
> > > > +            build_append_namestring(method, "DVNT");
> > > > +            build_append_namestring(method, "PCIU");
> > > >              build_append_int(method, 1); /* Device Check */
> > > > -            build_append_nameseg(method, "DVNT");
> > > > -            build_append_nameseg(method, "PCID");
> > > > +            build_append_namestring(method, "DVNT");
> > > > +            build_append_namestring(method, "PCID");
> > > >              build_append_int(method, 3); /* Eject Request */
> > > >          }
> > > >  
> > > > @@ -868,11 +868,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > >           * At the moment this is not needed for root as we have a single root.
> > > >           */
> > > >          if (bus->parent_dev) {
> > > > -            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> > > > -            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> > > > -            build_append_nameseg(parent->notify_table, "S%.02X",
> > > > +            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> > > >                                   bus->parent_dev->devfn);
> > > > -            build_append_nameseg(parent->notify_table, "PCNT");
> > > >          }
> > > >      }
> > > >  
> > > > @@ -940,7 +937,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >          GArray *sb_scope = build_alloc_array();
> > > >          uint8_t op = 0x10; /* ScopeOp */
> > > >  
> > > > -        build_append_nameseg(sb_scope, "_SB");
> > > > +        build_append_namestring(sb_scope, "_SB");
> > > >  
> > > >          /* build Processor object for each processor */
> > > >          for (i = 0; i < acpi_cpus; i++) {
> > > > @@ -960,7 +957,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> > > >  
> > > >          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> > > >          build_append_byte(sb_scope, 0x08); /* NameOp */
> > > > -        build_append_nameseg(sb_scope, "CPON");
> > > > +        build_append_namestring(sb_scope, "CPON");
> > > >  
> > > >          {
> > > >              GArray *package = build_alloc_array();
> > > > diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > > > index e6a0b28..fd50625 100644
> > > > --- a/include/hw/acpi/acpi_gen_utils.h
> > > > +++ b/include/hw/acpi/acpi_gen_utils.h
> > > > @@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
> > > >  void build_append_array(GArray *array, GArray *val);
> > > >  
> > > >  void GCC_FMT_ATTR(2, 3)
> > > > -build_append_nameseg(GArray *array, const char *format, ...);
> > > > +build_append_namestring(GArray *array, const char *format, ...);
> > > >  
> > > >  void build_prepend_package_length(GArray *package, unsigned min_bytes);
> > > >  void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> > > > -- 
> > > > 1.8.3.1

[Qemu-devel] [PATCH 7/9] acpi: replace opencoded notify codes with named values

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c             | 4 ++--
 include/hw/acpi/acpi_gen_utils.h | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0f6202d..a33d130 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -840,10 +840,10 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             build_append_namestring(method, "BNUM");
             build_append_namestring(method, "DVNT");
             build_append_namestring(method, "PCIU");
-            build_append_int(method, 1); /* Device Check */
+            build_append_int(method, ACPI_DEV_CHK);
             build_append_namestring(method, "DVNT");
             build_append_namestring(method, "PCID");
-            build_append_int(method, 3); /* Eject Request */
+            build_append_int(method, ACPI_DEV_EJ);
         }
 
         /* Notify about child bus events in any case */
diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
index fd50625..ce76dc1 100644
--- a/include/hw/acpi/acpi_gen_utils.h
+++ b/include/hw/acpi/acpi_gen_utils.h
@@ -5,6 +5,12 @@
 #include <glib.h>
 #include "qemu/compiler.h"
 
+/* ACPI 5.0: table "Device Object Notification Values" */
+enum {
+    ACPI_DEV_CHK = 1,
+    ACPI_DEV_EJ = 3,
+};
+
 GArray *build_alloc_array(void);
 void build_free_array(GArray *array);
 void build_prepend_byte(GArray *array, uint8_t val);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 7/9] acpi: replace opencoded notify codes with named values

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:06PM +0000, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

I'm not sure this makes sense for these constants.

Device Check seems more readable than ACPI_DEV_CHK.
If your object here is readability, please do not
abbreviate.

Generally ability to match spec names exactly is
what made me prefer code comments to enums for one-off constants.
Looking up "Device Check" works in any spec version,
I don't have to dig up the exact one, find table by name,
break my eyes trying to locate the correct line in
a huge table.
Just text search, and the correct line is highlighted.

Why look in another spec version you might ask?
Well it's definitely helpful to understand legacy
guest quirks.


> ---
>  hw/i386/acpi-build.c             | 4 ++--
>  include/hw/acpi/acpi_gen_utils.h | 6 ++++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 0f6202d..a33d130 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -840,10 +840,10 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>              build_append_namestring(method, "BNUM");
>              build_append_namestring(method, "DVNT");
>              build_append_namestring(method, "PCIU");
> -            build_append_int(method, 1); /* Device Check */
> +            build_append_int(method, ACPI_DEV_CHK);
>              build_append_namestring(method, "DVNT");
>              build_append_namestring(method, "PCID");
> -            build_append_int(method, 3); /* Eject Request */
> +            build_append_int(method, ACPI_DEV_EJ);
>          }
>  
>          /* Notify about child bus events in any case */
> diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> index fd50625..ce76dc1 100644
> --- a/include/hw/acpi/acpi_gen_utils.h
> +++ b/include/hw/acpi/acpi_gen_utils.h
> @@ -5,6 +5,12 @@
>  #include <glib.h>
>  #include "qemu/compiler.h"
>  
> +/* ACPI 5.0: table "Device Object Notification Values" */
> +enum {
> +    ACPI_DEV_CHK = 1,
> +    ACPI_DEV_EJ = 3,
> +};
> +
>  GArray *build_alloc_array(void);
>  void build_free_array(GArray *array);
>  void build_prepend_byte(GArray *array, uint8_t val);
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 7/9] acpi: replace opencoded notify codes with named values

[Igor Mammedov]

On Mon, 8 Dec 2014 22:54:03 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 08, 2014 at 04:08:06PM +0000, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> 
> I'm not sure this makes sense for these constants.
> 
> Device Check seems more readable than ACPI_DEV_CHK.
> If your object here is readability, please do not
> abbreviate.
> 
> Generally ability to match spec names exactly is
> what made me prefer code comments to enums for one-off constants.
> Looking up "Device Check" works in any spec version,
> I don't have to dig up the exact one, find table by name,
> break my eyes trying to locate the correct line in
> a huge table.
> Just text search, and the correct line is highlighted.
> 
> Why look in another spec version you might ask?
> Well it's definitely helpful to understand legacy
> guest quirks.
> 
> 

sure, lets drop it.

> > ---
> >  hw/i386/acpi-build.c             | 4 ++--
> >  include/hw/acpi/acpi_gen_utils.h | 6 ++++++
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 0f6202d..a33d130 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -840,10 +840,10 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >              build_append_namestring(method, "BNUM");
> >              build_append_namestring(method, "DVNT");
> >              build_append_namestring(method, "PCIU");
> > -            build_append_int(method, 1); /* Device Check */
> > +            build_append_int(method, ACPI_DEV_CHK);
> >              build_append_namestring(method, "DVNT");
> >              build_append_namestring(method, "PCID");
> > -            build_append_int(method, 3); /* Eject Request */
> > +            build_append_int(method, ACPI_DEV_EJ);
> >          }
> >  
> >          /* Notify about child bus events in any case */
> > diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > index fd50625..ce76dc1 100644
> > --- a/include/hw/acpi/acpi_gen_utils.h
> > +++ b/include/hw/acpi/acpi_gen_utils.h
> > @@ -5,6 +5,12 @@
> >  #include <glib.h>
> >  #include "qemu/compiler.h"
> >  
> > +/* ACPI 5.0: table "Device Object Notification Values" */
> > +enum {
> > +    ACPI_DEV_CHK = 1,
> > +    ACPI_DEV_EJ = 3,
> > +};
> > +
> >  GArray *build_alloc_array(void);
> >  void build_free_array(GArray *array);
> >  void build_prepend_byte(GArray *array, uint8_t val);
> > -- 
> > 1.8.3.1
> 

[Qemu-devel] [PATCH 8/9] acpi: drop min-bytes in build_package()

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/acpi_gen_utils.c         | 14 ++++----------
 hw/i386/acpi-build.c             | 13 ++++++-------
 include/hw/acpi/acpi_gen_utils.h |  4 ++--
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
index 31e2694..ff25012 100644
--- a/hw/acpi/acpi_gen_utils.c
+++ b/hw/acpi/acpi_gen_utils.c
@@ -145,7 +145,7 @@ enum {
     PACKAGE_LENGTH_4BYTE_SHIFT = 20,
 };
 
-void build_prepend_package_length(GArray *package, unsigned min_bytes)
+void build_prepend_package_length(GArray *package)
 {
     uint8_t byte;
     unsigned length = package->len;
@@ -161,11 +161,6 @@ void build_prepend_package_length(GArray *package, unsigned min_bytes)
         length_bytes = 4;
     }
 
-    /* Force length to at least min_bytes.
-     * This wastes memory but that's how bios did it.
-     */
-    length_bytes = MAX(length_bytes, min_bytes);
-
     /* PkgLength is the length of the inclusive length of the data. */
     length += length_bytes;
 
@@ -198,15 +193,15 @@ void build_prepend_package_length(GArray *package, unsigned min_bytes)
     build_prepend_byte(package, byte);
 }
 
-void build_package(GArray *package, uint8_t op, unsigned min_bytes)
+void build_package(GArray *package, uint8_t op)
 {
-    build_prepend_package_length(package, min_bytes);
+    build_prepend_package_length(package);
     build_prepend_byte(package, op);
 }
 
 void build_extop_package(GArray *package, uint8_t op)
 {
-    build_package(package, op, 1);
+    build_package(package, op);
     build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
 }
 
@@ -250,4 +245,3 @@ void build_append_int(GArray *table, uint32_t value)
         build_append_value(table, value, 4);
     }
 }
-
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a33d130..97ff245 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -283,7 +283,7 @@ static void build_append_and_cleanup_method(GArray *device, GArray *method)
 {
     uint8_t op = 0x14; /* MethodOp */
 
-    build_package(method, op, 0);
+    build_package(method, op);
 
     build_append_array(device, method);
     build_free_array(method);
@@ -304,7 +304,7 @@ static void build_append_notify_target_ifequal(GArray *method,
     build_append_byte(notify, 0x69); /* Arg1Op */
 
     /* Pack it up */
-    build_package(notify, op, 1);
+    build_package(notify, op);
 
     build_append_array(method, notify);
 
@@ -817,7 +817,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             build_append_byte(notify, 0x69); /* Arg1Op */
 
             /* Pack it up */
-            build_package(notify, op, 0);
+            build_package(notify, op);
 
             build_append_array(method, notify);
 
@@ -858,7 +858,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         if (bus->parent_dev) {
             build_extop_package(bus_table, op);
         } else {
-            build_package(bus_table, op, 0);
+            build_package(bus_table, op);
         }
 
         /* Append our bus description to parent table */
@@ -981,7 +981,7 @@ build_ssdt(GArray *table_data, GArray *linker,
                 build_append_byte(package, b);
             }
 
-            build_package(package, op, 2);
+            build_package(package, op);
             build_append_array(sb_scope, package);
             build_free_array(package);
         }
@@ -1029,8 +1029,7 @@ build_ssdt(GArray *table_data, GArray *linker,
             build_append_array(sb_scope, hotplug_state.device_table);
             build_pci_bus_state_cleanup(&hotplug_state);
         }
-
-        build_package(sb_scope, op, 3);
+        build_package(sb_scope, op);
         build_append_array(table_data, sb_scope);
         build_free_array(sb_scope);
     }
diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
index ce76dc1..91c9a55 100644
--- a/include/hw/acpi/acpi_gen_utils.h
+++ b/include/hw/acpi/acpi_gen_utils.h
@@ -20,8 +20,8 @@ void build_append_array(GArray *array, GArray *val);
 void GCC_FMT_ATTR(2, 3)
 build_append_namestring(GArray *array, const char *format, ...);
 
-void build_prepend_package_length(GArray *package, unsigned min_bytes);
-void build_package(GArray *package, uint8_t op, unsigned min_bytes);
+void build_prepend_package_length(GArray *package);
+void build_package(GArray *package, uint8_t op);
 void build_append_value(GArray *table, uint32_t value, int size);
 void build_append_int(GArray *table, uint32_t value);
 void build_extop_package(GArray *package, uint8_t op);
-- 
1.8.3.1

[Qemu-devel] [PATCH 9/9] pc: acpi-build: replace recursive PCI bus tree generation with loop based

[Igor Mammedov]

it replaces PCI tree structure in SSDT with a set of scopes
describing each PCI bus as a separate scope with a child devices.
It makes code easier to follow and a little bit smaller.

In addition it makes simplier to convert current template
patching approach to completely dynamically generated SSDT
without dependency on IASL, which would allow to drop
template binary blobs from source tree.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 362 +++++++++++++++++++++++----------------------------
 1 file changed, 165 insertions(+), 197 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 97ff245..7606a73 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -641,196 +641,186 @@ static void acpi_set_pci_info(void)
     }
 }
 
-static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
-                                     AcpiBuildPciBusHotplugState *parent,
-                                     bool pcihp_bridge_en)
+static char *pci_bus_get_scope_name(PCIBus *bus)
 {
-    state->parent = parent;
-    state->device_table = build_alloc_array();
-    state->notify_table = build_alloc_array();
-    state->pcihp_bridge_en = pcihp_bridge_en;
-}
+    char *name = NULL;
+    char *last = name;
 
-static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
-{
-    build_free_array(state->device_table);
-    build_free_array(state->notify_table);
-}
+    while (bus->parent_dev) {
+        last = name;
+        name = g_strdup_printf("%s.S%.02X_",
+                               name ? name : "", bus->parent_dev->devfn);
+        g_free(last);
 
-static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
-{
-    AcpiBuildPciBusHotplugState *parent = parent_state;
-    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
+        bus = bus->parent_dev->bus;
+    }
 
-    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
+    last = name;
+    name = g_strdup_printf("PCI0%s", name ? name : "");
+    g_free(last);
 
-    return child;
+    return name;
 }
 
-static void build_pci_bus_end(PCIBus *bus, void *bus_state)
+static GQueue *get_pci_bus_list(PCIBus *bus, bool pcihp_bridge_en)
 {
-    AcpiBuildPciBusHotplugState *child = bus_state;
-    AcpiBuildPciBusHotplugState *parent = child->parent;
-    GArray *bus_table = build_alloc_array();
-    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
-    uint8_t op;
-    int i;
-    QObject *bsel;
-    GArray *method;
-    bool bus_hotplug_support = false;
-
-    /*
-     * Skip bridge subtree creation if bridge hotplug is disabled
-     * to make acpi tables compatible with legacy machine types.
-     */
-    if (!child->pcihp_bridge_en && bus->parent_dev) {
-        return;
-    }
-
-    if (bus->parent_dev) {
-        op = 0x82; /* DeviceOp */
-        build_append_namestring(bus_table, "S%.02X",
-                             bus->parent_dev->devfn);
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "_SUN");
-        build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "_ADR");
-        build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
-                           PCI_FUNC(bus->parent_dev->devfn), 4);
-    } else {
-        op = 0x10; /* ScopeOp */;
-        build_append_namestring(bus_table, "PCI0");
-    }
-
-    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
-    if (bsel) {
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "BSEL");
-        build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
-        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
-    } else {
-        /* No bsel - no slots are hot-pluggable */
-        memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
+    GQueue *bus_list = g_queue_new();
+    GQueue *tree_walk = g_queue_new();
+
+    /* build BUS list */
+    g_queue_push_tail(tree_walk, bus);
+    while (!g_queue_is_empty(tree_walk)) {
+        PCIBus *sec;
+        PCIBus *parent = g_queue_pop_tail(tree_walk);
+
+        g_queue_push_tail(bus_list, parent);
+        if (!pcihp_bridge_en) {
+            break;
+        }
+        QLIST_FOREACH(sec, &parent->child, sibling) {
+            g_queue_push_tail(tree_walk, sec);
+        }
     }
+    g_queue_free(tree_walk);
+    return bus_list;
+}
 
-    memset(slot_device_present, 0x00, sizeof slot_device_present);
-    memset(slot_device_system, 0x00, sizeof slot_device_present);
-    memset(slot_device_vga, 0x00, sizeof slot_device_vga);
-    memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
-
-    for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
-        DeviceClass *dc;
-        PCIDeviceClass *pc;
-        PCIDevice *pdev = bus->devices[i];
-        int slot = PCI_SLOT(i);
-        bool bridge_in_acpi;
-
-        if (!pdev) {
-            continue;
+static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
+                                         bool pcihp_bridge_en)
+{
+    GQueue *bus_list = get_pci_bus_list(bus, pcihp_bridge_en);
+
+    while (!g_queue_is_empty(bus_list)) {
+        DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
+        DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
+        DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
+        DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
+        DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
+        GArray *bus_table = build_alloc_array();
+        PCIBus *bus = g_queue_pop_head(bus_list);
+        GArray *method;
+        QObject *bsel;
+        PCIBus *sec;
+        int i;
+
+        char *scope_name = pci_bus_get_scope_name(bus);
+        build_append_namestring(bus_table, "%s", scope_name);
+        g_free(scope_name);
+
+        bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
+                                           NULL);
+        if (bsel) {
+            build_append_byte(bus_table, 0x08); /* NameOp */
+            build_append_namestring(bus_table, "BSEL");
+            build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
+            memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
+        } else {
+            /* No bsel - no slots are hot-pluggable */
+            memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
         }
 
-        set_bit(slot, slot_device_present);
-        pc = PCI_DEVICE_GET_CLASS(pdev);
-        dc = DEVICE_GET_CLASS(pdev);
+        memset(slot_device_present, 0x00, sizeof slot_device_present);
+        memset(slot_device_system, 0x00, sizeof slot_device_present);
+        memset(slot_device_vga, 0x00, sizeof slot_device_vga);
+        memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
 
-        /* When hotplug for bridges is enabled, bridges are
-         * described in ACPI separately (see build_pci_bus_end).
-         * In this case they aren't themselves hot-pluggable.
-         */
-        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
+        for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
+            DeviceClass *dc;
+            PCIDeviceClass *pc;
+            PCIDevice *pdev = bus->devices[i];
+            int slot = PCI_SLOT(i);
 
-        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
-            set_bit(slot, slot_device_system);
-        }
+            if (!pdev) {
+                continue;
+            }
 
-        if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
-            set_bit(slot, slot_device_vga);
+            set_bit(slot, slot_device_present);
+            pc = PCI_DEVICE_GET_CLASS(pdev);
+            dc = DEVICE_GET_CLASS(pdev);
 
-            if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
-                set_bit(slot, slot_device_qxl);
+            if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
+                set_bit(slot, slot_device_system);
             }
-        }
 
-        if (!dc->hotpluggable || pc->is_bridge) {
-            clear_bit(slot, slot_hotplug_enable);
-        }
-    }
+            if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
+                set_bit(slot, slot_device_vga);
 
-    /* Append Device object for each slot */
-    for (i = 0; i < PCI_SLOT_MAX; i++) {
-        bool can_eject = test_bit(i, slot_hotplug_enable);
-        bool present = test_bit(i, slot_device_present);
-        bool vga = test_bit(i, slot_device_vga);
-        bool qxl = test_bit(i, slot_device_qxl);
-        bool system = test_bit(i, slot_device_system);
-        if (can_eject) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIHP_SIZEOF);
-            memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
-            patch_pcihp(i, pcihp);
-            bus_hotplug_support = true;
-        } else if (qxl) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIQXL_SIZEOF);
-            memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
-            patch_pciqxl(i, pcihp);
-        } else if (vga) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIVGA_SIZEOF);
-            memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
-            patch_pcivga(i, pcihp);
-        } else if (system) {
-            /* Nothing to do: system devices are in DSDT or in SSDT above. */
-        } else if (present) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCINOHP_SIZEOF);
-            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
-            patch_pcinohp(i, pcihp);
-        }
-    }
+                if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
+                    set_bit(slot, slot_device_qxl);
+                }
+            }
 
-    if (bsel) {
-        method = build_alloc_method("DVNT", 2);
+            if (!dc->hotpluggable || pc->is_bridge) {
+                clear_bit(slot, slot_hotplug_enable);
+            }
+        }
 
+        /* Append Device object for each slot */
         for (i = 0; i < PCI_SLOT_MAX; i++) {
-            GArray *notify;
-            uint8_t op;
-
-            if (!test_bit(i, slot_hotplug_enable)) {
-                continue;
+            bool can_eject = test_bit(i, slot_hotplug_enable);
+            bool present = test_bit(i, slot_device_present);
+            bool vga = test_bit(i, slot_device_vga);
+            bool qxl = test_bit(i, slot_device_qxl);
+            bool system = test_bit(i, slot_device_system);
+            if (can_eject) {
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIHP_SIZEOF);
+                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
+                patch_pcihp(i, pcihp);
+            } else if (qxl) {
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIQXL_SIZEOF);
+                memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
+                patch_pciqxl(i, pcihp);
+            } else if (vga) {
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIVGA_SIZEOF);
+                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
+                patch_pcivga(i, pcihp);
+            } else if (system) {
+                /* Nothing to do: system devices are in DSDT or in SSDT above */
+            } else if (present) {
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCINOHP_SIZEOF);
+                memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
+                patch_pcinohp(i, pcihp);
             }
+        }
 
-            notify = build_alloc_array();
-            op = 0xA0; /* IfOp */
-
-            build_append_byte(notify, 0x7B); /* AndOp */
-            build_append_byte(notify, 0x68); /* Arg0Op */
-            build_append_int(notify, 0x1U << i);
-            build_append_byte(notify, 0x00); /* NullName */
-            build_append_byte(notify, 0x86); /* NotifyOp */
-            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
-            build_append_byte(notify, 0x69); /* Arg1Op */
-
-            /* Pack it up */
-            build_package(notify, op);
-
-            build_append_array(method, notify);
+        if (bsel) {
+            method = build_alloc_method("DVNT", 2);
+
+            for (i = 0; i < PCI_SLOT_MAX; i++) {
+                GArray *notify;
+                uint8_t op;
+
+                if (!test_bit(i, slot_hotplug_enable)) {
+                    continue;
+                }
+
+                notify = build_alloc_array();
+                op = 0xA0; /* IfOp */
+
+                build_append_byte(notify, 0x7B); /* AndOp */
+                build_append_byte(notify, 0x68); /* Arg0Op */
+                build_append_int(notify, 0x1U << i);
+                build_append_byte(notify, 0x00); /* NullName */
+                build_append_byte(notify, 0x86); /* NotifyOp */
+                build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
+                build_append_byte(notify, 0x69); /* Arg1Op */
+
+                /* Pack it up */
+                build_package(notify, op);
+                build_append_array(method, notify);
+                build_free_array(notify);
+            }
 
-            build_free_array(notify);
+            build_append_and_cleanup_method(bus_table, method);
         }
 
-        build_append_and_cleanup_method(bus_table, method);
-    }
-
-    /* Append PCNT method to notify about events on local and child buses.
-     * Add unconditionally for root since DSDT expects it.
-     */
-    if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
+        /* Append PCNT method to notify about events on local and child buses.
+         * Add unconditionally for root since DSDT expects it.
+         */
         method = build_alloc_method("PCNT", 0);
 
         /* If bus supports hotplug select it and notify about local events */
@@ -847,36 +837,20 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         }
 
         /* Notify about child bus events in any case */
-        build_append_array(method, child->notify_table);
-
-        build_append_and_cleanup_method(bus_table, method);
-
-        /* Append description of child buses */
-        build_append_array(bus_table, child->device_table);
-
-        /* Pack it up */
-        if (bus->parent_dev) {
-            build_extop_package(bus_table, op);
-        } else {
-            build_package(bus_table, op);
+        if (pcihp_bridge_en) {
+            QLIST_FOREACH(sec, &bus->child, sibling) {
+                build_append_namestring(method, "^S%.02X.PCNT",
+                                        sec->parent_dev->devfn);
+            }
         }
 
-        /* Append our bus description to parent table */
-        build_append_array(parent->device_table, bus_table);
+        build_append_and_cleanup_method(bus_table, method);
 
-        /* Also tell parent how to notify us, invoking PCNT method.
-         * At the moment this is not needed for root as we have a single root.
-         */
-        if (bus->parent_dev) {
-            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
-                                 bus->parent_dev->devfn);
-        }
+        build_package(bus_table, 0x10); /* ScopeOp */
+        build_append_array(parent_scope, bus_table);
+        build_free_array(bus_table);
     }
-
-    qobject_decref(bsel);
-    build_free_array(bus_table);
-    build_pci_bus_state_cleanup(child);
-    g_free(child);
+    g_queue_free(bus_list);
 }
 
 static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
@@ -1008,7 +982,6 @@ build_ssdt(GArray *table_data, GArray *linker,
         }
 
         {
-            AcpiBuildPciBusHotplugState hotplug_state;
             Object *pci_host;
             PCIBus *bus = NULL;
             bool ambiguous;
@@ -1018,16 +991,11 @@ build_ssdt(GArray *table_data, GArray *linker,
                 bus = PCI_HOST_BRIDGE(pci_host)->bus;
             }
 
-            build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
-
             if (bus) {
                 /* Scan all PCI buses. Generate tables to support hotplug. */
-                pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
-                                             build_pci_bus_end, &hotplug_state);
+                build_append_pci_bus_devices(sb_scope, bus,
+                                             pm->pcihp_bridge_en);
             }
-
-            build_append_array(sb_scope, hotplug_state.device_table);
-            build_pci_bus_state_cleanup(&hotplug_state);
         }
         build_package(sb_scope, op);
         build_append_array(table_data, sb_scope);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 9/9] pc: acpi-build: replace recursive PCI bus tree generation with loop based

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:08:08PM +0000, Igor Mammedov wrote:
> it replaces PCI tree structure in SSDT with a set of scopes
> describing each PCI bus as a separate scope with a child devices.
> It makes code easier to follow and a little bit smaller.
> 
> In addition it makes simplier to convert current template
> patching approach to completely dynamically generated SSDT
> without dependency on IASL, which would allow to drop
> template binary blobs from source tree.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c | 362 +++++++++++++++++++++++----------------------------
>  1 file changed, 165 insertions(+), 197 deletions(-)

I like it that your patch makes code smaller and simpler,
but I think we do need to generate hierarchical AML.

I think this can still be done with some modifications to
the logic you have here.

Basically current code does all work in build_pci_bus_end.
It follows that you can do the same by simply walking
the list in reverse order.





> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 97ff245..7606a73 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -641,196 +641,186 @@ static void acpi_set_pci_info(void)
>      }
>  }
>  
> -static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
> -                                     AcpiBuildPciBusHotplugState *parent,
> -                                     bool pcihp_bridge_en)
> +static char *pci_bus_get_scope_name(PCIBus *bus)
>  {
> -    state->parent = parent;
> -    state->device_table = build_alloc_array();
> -    state->notify_table = build_alloc_array();
> -    state->pcihp_bridge_en = pcihp_bridge_en;
> -}
> +    char *name = NULL;
> +    char *last = name;
>  
> -static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
> -{
> -    build_free_array(state->device_table);
> -    build_free_array(state->notify_table);
> -}
> +    while (bus->parent_dev) {
> +        last = name;
> +        name = g_strdup_printf("%s.S%.02X_",
> +                               name ? name : "", bus->parent_dev->devfn);
> +        g_free(last);
>  
> -static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
> -{
> -    AcpiBuildPciBusHotplugState *parent = parent_state;
> -    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
> +        bus = bus->parent_dev->bus;
> +    }
>  
> -    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
> +    last = name;
> +    name = g_strdup_printf("PCI0%s", name ? name : "");
> +    g_free(last);
>  
> -    return child;
> +    return name;
>  }
>  
> -static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> +static GQueue *get_pci_bus_list(PCIBus *bus, bool pcihp_bridge_en)
>  {
> -    AcpiBuildPciBusHotplugState *child = bus_state;
> -    AcpiBuildPciBusHotplugState *parent = child->parent;
> -    GArray *bus_table = build_alloc_array();
> -    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> -    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> -    DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> -    DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> -    DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> -    uint8_t op;
> -    int i;
> -    QObject *bsel;
> -    GArray *method;
> -    bool bus_hotplug_support = false;
> -
> -    /*
> -     * Skip bridge subtree creation if bridge hotplug is disabled
> -     * to make acpi tables compatible with legacy machine types.
> -     */
> -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> -        return;
> -    }
> -
> -    if (bus->parent_dev) {
> -        op = 0x82; /* DeviceOp */
> -        build_append_namestring(bus_table, "S%.02X",
> -                             bus->parent_dev->devfn);
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_namestring(bus_table, "_SUN");
> -        build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_namestring(bus_table, "_ADR");
> -        build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> -                           PCI_FUNC(bus->parent_dev->devfn), 4);
> -    } else {
> -        op = 0x10; /* ScopeOp */;
> -        build_append_namestring(bus_table, "PCI0");
> -    }
> -
> -    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> -    if (bsel) {
> -        build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_namestring(bus_table, "BSEL");
> -        build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> -        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> -    } else {
> -        /* No bsel - no slots are hot-pluggable */
> -        memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
> +    GQueue *bus_list = g_queue_new();
> +    GQueue *tree_walk = g_queue_new();
> +
> +    /* build BUS list */
> +    g_queue_push_tail(tree_walk, bus);
> +    while (!g_queue_is_empty(tree_walk)) {
> +        PCIBus *sec;
> +        PCIBus *parent = g_queue_pop_tail(tree_walk);
> +
> +        g_queue_push_tail(bus_list, parent);
> +        if (!pcihp_bridge_en) {
> +            break;
> +        }
> +        QLIST_FOREACH(sec, &parent->child, sibling) {
> +            g_queue_push_tail(tree_walk, sec);
> +        }
>      }
> +    g_queue_free(tree_walk);
> +    return bus_list;
> +}
>  
> -    memset(slot_device_present, 0x00, sizeof slot_device_present);
> -    memset(slot_device_system, 0x00, sizeof slot_device_present);
> -    memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> -    memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
> -
> -    for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> -        DeviceClass *dc;
> -        PCIDeviceClass *pc;
> -        PCIDevice *pdev = bus->devices[i];
> -        int slot = PCI_SLOT(i);
> -        bool bridge_in_acpi;
> -
> -        if (!pdev) {
> -            continue;
> +static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
> +                                         bool pcihp_bridge_en)
> +{
> +    GQueue *bus_list = get_pci_bus_list(bus, pcihp_bridge_en);
> +
> +    while (!g_queue_is_empty(bus_list)) {
> +        DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> +        DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> +        DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> +        DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> +        DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> +        GArray *bus_table = build_alloc_array();
> +        PCIBus *bus = g_queue_pop_head(bus_list);
> +        GArray *method;
> +        QObject *bsel;
> +        PCIBus *sec;
> +        int i;
> +
> +        char *scope_name = pci_bus_get_scope_name(bus);
> +        build_append_namestring(bus_table, "%s", scope_name);
> +        g_free(scope_name);
> +
> +        bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> +                                           NULL);
> +        if (bsel) {
> +            build_append_byte(bus_table, 0x08); /* NameOp */
> +            build_append_namestring(bus_table, "BSEL");
> +            build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> +            memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> +        } else {
> +            /* No bsel - no slots are hot-pluggable */
> +            memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
>          }
>  
> -        set_bit(slot, slot_device_present);
> -        pc = PCI_DEVICE_GET_CLASS(pdev);
> -        dc = DEVICE_GET_CLASS(pdev);
> +        memset(slot_device_present, 0x00, sizeof slot_device_present);
> +        memset(slot_device_system, 0x00, sizeof slot_device_present);
> +        memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> +        memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
>  
> -        /* When hotplug for bridges is enabled, bridges are
> -         * described in ACPI separately (see build_pci_bus_end).
> -         * In this case they aren't themselves hot-pluggable.
> -         */
> -        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
> +        for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> +            DeviceClass *dc;
> +            PCIDeviceClass *pc;
> +            PCIDevice *pdev = bus->devices[i];
> +            int slot = PCI_SLOT(i);
>  
> -        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
> -            set_bit(slot, slot_device_system);
> -        }
> +            if (!pdev) {
> +                continue;
> +            }
>  
> -        if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> -            set_bit(slot, slot_device_vga);
> +            set_bit(slot, slot_device_present);
> +            pc = PCI_DEVICE_GET_CLASS(pdev);
> +            dc = DEVICE_GET_CLASS(pdev);
>  
> -            if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> -                set_bit(slot, slot_device_qxl);
> +            if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
> +                set_bit(slot, slot_device_system);
>              }
> -        }
>  
> -        if (!dc->hotpluggable || pc->is_bridge) {
> -            clear_bit(slot, slot_hotplug_enable);
> -        }
> -    }
> +            if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> +                set_bit(slot, slot_device_vga);
>  
> -    /* Append Device object for each slot */
> -    for (i = 0; i < PCI_SLOT_MAX; i++) {
> -        bool can_eject = test_bit(i, slot_hotplug_enable);
> -        bool present = test_bit(i, slot_device_present);
> -        bool vga = test_bit(i, slot_device_vga);
> -        bool qxl = test_bit(i, slot_device_qxl);
> -        bool system = test_bit(i, slot_device_system);
> -        if (can_eject) {
> -            void *pcihp = acpi_data_push(bus_table,
> -                                         ACPI_PCIHP_SIZEOF);
> -            memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> -            patch_pcihp(i, pcihp);
> -            bus_hotplug_support = true;
> -        } else if (qxl) {
> -            void *pcihp = acpi_data_push(bus_table,
> -                                         ACPI_PCIQXL_SIZEOF);
> -            memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> -            patch_pciqxl(i, pcihp);
> -        } else if (vga) {
> -            void *pcihp = acpi_data_push(bus_table,
> -                                         ACPI_PCIVGA_SIZEOF);
> -            memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> -            patch_pcivga(i, pcihp);
> -        } else if (system) {
> -            /* Nothing to do: system devices are in DSDT or in SSDT above. */
> -        } else if (present) {
> -            void *pcihp = acpi_data_push(bus_table,
> -                                         ACPI_PCINOHP_SIZEOF);
> -            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> -            patch_pcinohp(i, pcihp);
> -        }
> -    }
> +                if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> +                    set_bit(slot, slot_device_qxl);
> +                }
> +            }
>  
> -    if (bsel) {
> -        method = build_alloc_method("DVNT", 2);
> +            if (!dc->hotpluggable || pc->is_bridge) {
> +                clear_bit(slot, slot_hotplug_enable);
> +            }
> +        }
>  
> +        /* Append Device object for each slot */
>          for (i = 0; i < PCI_SLOT_MAX; i++) {
> -            GArray *notify;
> -            uint8_t op;
> -
> -            if (!test_bit(i, slot_hotplug_enable)) {
> -                continue;
> +            bool can_eject = test_bit(i, slot_hotplug_enable);
> +            bool present = test_bit(i, slot_device_present);
> +            bool vga = test_bit(i, slot_device_vga);
> +            bool qxl = test_bit(i, slot_device_qxl);
> +            bool system = test_bit(i, slot_device_system);
> +            if (can_eject) {
> +                void *pcihp = acpi_data_push(bus_table,
> +                                             ACPI_PCIHP_SIZEOF);
> +                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> +                patch_pcihp(i, pcihp);
> +            } else if (qxl) {
> +                void *pcihp = acpi_data_push(bus_table,
> +                                             ACPI_PCIQXL_SIZEOF);
> +                memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> +                patch_pciqxl(i, pcihp);
> +            } else if (vga) {
> +                void *pcihp = acpi_data_push(bus_table,
> +                                             ACPI_PCIVGA_SIZEOF);
> +                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> +                patch_pcivga(i, pcihp);
> +            } else if (system) {
> +                /* Nothing to do: system devices are in DSDT or in SSDT above */
> +            } else if (present) {
> +                void *pcihp = acpi_data_push(bus_table,
> +                                             ACPI_PCINOHP_SIZEOF);
> +                memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> +                patch_pcinohp(i, pcihp);
>              }
> +        }
>  
> -            notify = build_alloc_array();
> -            op = 0xA0; /* IfOp */
> -
> -            build_append_byte(notify, 0x7B); /* AndOp */
> -            build_append_byte(notify, 0x68); /* Arg0Op */
> -            build_append_int(notify, 0x1U << i);
> -            build_append_byte(notify, 0x00); /* NullName */
> -            build_append_byte(notify, 0x86); /* NotifyOp */
> -            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> -            build_append_byte(notify, 0x69); /* Arg1Op */
> -
> -            /* Pack it up */
> -            build_package(notify, op);
> -
> -            build_append_array(method, notify);
> +        if (bsel) {
> +            method = build_alloc_method("DVNT", 2);
> +
> +            for (i = 0; i < PCI_SLOT_MAX; i++) {
> +                GArray *notify;
> +                uint8_t op;
> +
> +                if (!test_bit(i, slot_hotplug_enable)) {
> +                    continue;
> +                }
> +
> +                notify = build_alloc_array();
> +                op = 0xA0; /* IfOp */
> +
> +                build_append_byte(notify, 0x7B); /* AndOp */
> +                build_append_byte(notify, 0x68); /* Arg0Op */
> +                build_append_int(notify, 0x1U << i);
> +                build_append_byte(notify, 0x00); /* NullName */
> +                build_append_byte(notify, 0x86); /* NotifyOp */
> +                build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> +                build_append_byte(notify, 0x69); /* Arg1Op */
> +
> +                /* Pack it up */
> +                build_package(notify, op);
> +                build_append_array(method, notify);
> +                build_free_array(notify);
> +            }
>  
> -            build_free_array(notify);
> +            build_append_and_cleanup_method(bus_table, method);
>          }
>  
> -        build_append_and_cleanup_method(bus_table, method);
> -    }
> -
> -    /* Append PCNT method to notify about events on local and child buses.
> -     * Add unconditionally for root since DSDT expects it.
> -     */
> -    if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
> +        /* Append PCNT method to notify about events on local and child buses.
> +         * Add unconditionally for root since DSDT expects it.
> +         */
>          method = build_alloc_method("PCNT", 0);
>  
>          /* If bus supports hotplug select it and notify about local events */
> @@ -847,36 +837,20 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>          }
>  
>          /* Notify about child bus events in any case */
> -        build_append_array(method, child->notify_table);
> -
> -        build_append_and_cleanup_method(bus_table, method);
> -
> -        /* Append description of child buses */
> -        build_append_array(bus_table, child->device_table);
> -
> -        /* Pack it up */
> -        if (bus->parent_dev) {
> -            build_extop_package(bus_table, op);
> -        } else {
> -            build_package(bus_table, op);
> +        if (pcihp_bridge_en) {
> +            QLIST_FOREACH(sec, &bus->child, sibling) {
> +                build_append_namestring(method, "^S%.02X.PCNT",
> +                                        sec->parent_dev->devfn);
> +            }
>          }
>  
> -        /* Append our bus description to parent table */
> -        build_append_array(parent->device_table, bus_table);
> +        build_append_and_cleanup_method(bus_table, method);
>  
> -        /* Also tell parent how to notify us, invoking PCNT method.
> -         * At the moment this is not needed for root as we have a single root.
> -         */
> -        if (bus->parent_dev) {
> -            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> -                                 bus->parent_dev->devfn);
> -        }
> +        build_package(bus_table, 0x10); /* ScopeOp */
> +        build_append_array(parent_scope, bus_table);
> +        build_free_array(bus_table);
>      }
> -
> -    qobject_decref(bsel);
> -    build_free_array(bus_table);
> -    build_pci_bus_state_cleanup(child);
> -    g_free(child);
> +    g_queue_free(bus_list);
>  }
>  
>  static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
> @@ -1008,7 +982,6 @@ build_ssdt(GArray *table_data, GArray *linker,
>          }
>  
>          {
> -            AcpiBuildPciBusHotplugState hotplug_state;
>              Object *pci_host;
>              PCIBus *bus = NULL;
>              bool ambiguous;
> @@ -1018,16 +991,11 @@ build_ssdt(GArray *table_data, GArray *linker,
>                  bus = PCI_HOST_BRIDGE(pci_host)->bus;
>              }
>  
> -            build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
> -
>              if (bus) {
>                  /* Scan all PCI buses. Generate tables to support hotplug. */
> -                pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
> -                                             build_pci_bus_end, &hotplug_state);
> +                build_append_pci_bus_devices(sb_scope, bus,
> +                                             pm->pcihp_bridge_en);
>              }
> -
> -            build_append_array(sb_scope, hotplug_state.device_table);
> -            build_pci_bus_state_cleanup(&hotplug_state);
>          }
>          build_package(sb_scope, op);
>          build_append_array(table_data, sb_scope);
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 9/9] pc: acpi-build: replace recursive PCI bus tree generation with loop based

[Igor Mammedov]

On Mon, 8 Dec 2014 22:43:24 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 08, 2014 at 04:08:08PM +0000, Igor Mammedov wrote:
> > it replaces PCI tree structure in SSDT with a set of scopes
> > describing each PCI bus as a separate scope with a child devices.
> > It makes code easier to follow and a little bit smaller.
> > 
> > In addition it makes simplier to convert current template
> > patching approach to completely dynamically generated SSDT
> > without dependency on IASL, which would allow to drop
> > template binary blobs from source tree.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/i386/acpi-build.c | 362 +++++++++++++++++++++++----------------------------
> >  1 file changed, 165 insertions(+), 197 deletions(-)
> 
> I like it that your patch makes code smaller and simpler,
> but I think we do need to generate hierarchical AML.
If only reason for hierarchical AML is table size then
I don't see how scopes could possibly double the table size,
Compared to tree version scope adds only:

 ~5 bytes for package + NamePath(~[0-2] + 4 * tree depth)

Which is almost nothing compared to slots description
per a bridge.
One needs to configure insanely deep bridge nesting
for scope cost to significantly affect table size, which
doesn't sound as practical thing to do anyway except of
testing limitation on how many nested bridges QEMU would
stomach.

> 
> I think this can still be done with some modifications to
> the logic you have here.
> 
> Basically current code does all work in build_pci_bus_end.
> It follows that you can do the same by simply walking
> the list in reverse order.
I've thought about it already, but tree creation means
creating temporary contexts for each scope, copying them
to parent context, keeping track of contexts to build
correct notifiers. It's basically the same or even worse
as current code just other way around.

And all that makes code quite hard to follow and
maintain, that is the main reason to drop recursion in
favor of simple flat scope sets. And it makes easier to
switch from AML+template patching to ASL API later
and hide from user direct access to table array, forcing
him to work only with ASL constructs when building table.

> 
> 
> 
> 
> 
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 97ff245..7606a73 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -641,196 +641,186 @@ static void acpi_set_pci_info(void)
> >      }
> >  }
> >  
> > -static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
> > -                                     AcpiBuildPciBusHotplugState *parent,
> > -                                     bool pcihp_bridge_en)
> > +static char *pci_bus_get_scope_name(PCIBus *bus)
> >  {
> > -    state->parent = parent;
> > -    state->device_table = build_alloc_array();
> > -    state->notify_table = build_alloc_array();
> > -    state->pcihp_bridge_en = pcihp_bridge_en;
> > -}
> > +    char *name = NULL;
> > +    char *last = name;
> >  
> > -static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
> > -{
> > -    build_free_array(state->device_table);
> > -    build_free_array(state->notify_table);
> > -}
> > +    while (bus->parent_dev) {
> > +        last = name;
> > +        name = g_strdup_printf("%s.S%.02X_",
> > +                               name ? name : "", bus->parent_dev->devfn);
> > +        g_free(last);
> >  
> > -static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
> > -{
> > -    AcpiBuildPciBusHotplugState *parent = parent_state;
> > -    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
> > +        bus = bus->parent_dev->bus;
> > +    }
> >  
> > -    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
> > +    last = name;
> > +    name = g_strdup_printf("PCI0%s", name ? name : "");
> > +    g_free(last);
> >  
> > -    return child;
> > +    return name;
> >  }
> >  
> > -static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > +static GQueue *get_pci_bus_list(PCIBus *bus, bool pcihp_bridge_en)
> >  {
> > -    AcpiBuildPciBusHotplugState *child = bus_state;
> > -    AcpiBuildPciBusHotplugState *parent = child->parent;
> > -    GArray *bus_table = build_alloc_array();
> > -    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> > -    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> > -    DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> > -    DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> > -    DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> > -    uint8_t op;
> > -    int i;
> > -    QObject *bsel;
> > -    GArray *method;
> > -    bool bus_hotplug_support = false;
> > -
> > -    /*
> > -     * Skip bridge subtree creation if bridge hotplug is disabled
> > -     * to make acpi tables compatible with legacy machine types.
> > -     */
> > -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> > -        return;
> > -    }
> > -
> > -    if (bus->parent_dev) {
> > -        op = 0x82; /* DeviceOp */
> > -        build_append_namestring(bus_table, "S%.02X",
> > -                             bus->parent_dev->devfn);
> > -        build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_namestring(bus_table, "_SUN");
> > -        build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> > -        build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_namestring(bus_table, "_ADR");
> > -        build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> > -                           PCI_FUNC(bus->parent_dev->devfn), 4);
> > -    } else {
> > -        op = 0x10; /* ScopeOp */;
> > -        build_append_namestring(bus_table, "PCI0");
> > -    }
> > -
> > -    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> > -    if (bsel) {
> > -        build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_namestring(bus_table, "BSEL");
> > -        build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> > -        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> > -    } else {
> > -        /* No bsel - no slots are hot-pluggable */
> > -        memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
> > +    GQueue *bus_list = g_queue_new();
> > +    GQueue *tree_walk = g_queue_new();
> > +
> > +    /* build BUS list */
> > +    g_queue_push_tail(tree_walk, bus);
> > +    while (!g_queue_is_empty(tree_walk)) {
> > +        PCIBus *sec;
> > +        PCIBus *parent = g_queue_pop_tail(tree_walk);
> > +
> > +        g_queue_push_tail(bus_list, parent);
> > +        if (!pcihp_bridge_en) {
> > +            break;
> > +        }
> > +        QLIST_FOREACH(sec, &parent->child, sibling) {
> > +            g_queue_push_tail(tree_walk, sec);
> > +        }
> >      }
> > +    g_queue_free(tree_walk);
> > +    return bus_list;
> > +}
> >  
> > -    memset(slot_device_present, 0x00, sizeof slot_device_present);
> > -    memset(slot_device_system, 0x00, sizeof slot_device_present);
> > -    memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> > -    memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
> > -
> > -    for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> > -        DeviceClass *dc;
> > -        PCIDeviceClass *pc;
> > -        PCIDevice *pdev = bus->devices[i];
> > -        int slot = PCI_SLOT(i);
> > -        bool bridge_in_acpi;
> > -
> > -        if (!pdev) {
> > -            continue;
> > +static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
> > +                                         bool pcihp_bridge_en)
> > +{
> > +    GQueue *bus_list = get_pci_bus_list(bus, pcihp_bridge_en);
> > +
> > +    while (!g_queue_is_empty(bus_list)) {
> > +        DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> > +        DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> > +        DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> > +        DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> > +        DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> > +        GArray *bus_table = build_alloc_array();
> > +        PCIBus *bus = g_queue_pop_head(bus_list);
> > +        GArray *method;
> > +        QObject *bsel;
> > +        PCIBus *sec;
> > +        int i;
> > +
> > +        char *scope_name = pci_bus_get_scope_name(bus);
> > +        build_append_namestring(bus_table, "%s", scope_name);
> > +        g_free(scope_name);
> > +
> > +        bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> > +                                           NULL);
> > +        if (bsel) {
> > +            build_append_byte(bus_table, 0x08); /* NameOp */
> > +            build_append_namestring(bus_table, "BSEL");
> > +            build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> > +            memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> > +        } else {
> > +            /* No bsel - no slots are hot-pluggable */
> > +            memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
> >          }
> >  
> > -        set_bit(slot, slot_device_present);
> > -        pc = PCI_DEVICE_GET_CLASS(pdev);
> > -        dc = DEVICE_GET_CLASS(pdev);
> > +        memset(slot_device_present, 0x00, sizeof slot_device_present);
> > +        memset(slot_device_system, 0x00, sizeof slot_device_present);
> > +        memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> > +        memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
> >  
> > -        /* When hotplug for bridges is enabled, bridges are
> > -         * described in ACPI separately (see build_pci_bus_end).
> > -         * In this case they aren't themselves hot-pluggable.
> > -         */
> > -        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
> > +        for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> > +            DeviceClass *dc;
> > +            PCIDeviceClass *pc;
> > +            PCIDevice *pdev = bus->devices[i];
> > +            int slot = PCI_SLOT(i);
> >  
> > -        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
> > -            set_bit(slot, slot_device_system);
> > -        }
> > +            if (!pdev) {
> > +                continue;
> > +            }
> >  
> > -        if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> > -            set_bit(slot, slot_device_vga);
> > +            set_bit(slot, slot_device_present);
> > +            pc = PCI_DEVICE_GET_CLASS(pdev);
> > +            dc = DEVICE_GET_CLASS(pdev);
> >  
> > -            if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> > -                set_bit(slot, slot_device_qxl);
> > +            if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
> > +                set_bit(slot, slot_device_system);
> >              }
> > -        }
> >  
> > -        if (!dc->hotpluggable || pc->is_bridge) {
> > -            clear_bit(slot, slot_hotplug_enable);
> > -        }
> > -    }
> > +            if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> > +                set_bit(slot, slot_device_vga);
> >  
> > -    /* Append Device object for each slot */
> > -    for (i = 0; i < PCI_SLOT_MAX; i++) {
> > -        bool can_eject = test_bit(i, slot_hotplug_enable);
> > -        bool present = test_bit(i, slot_device_present);
> > -        bool vga = test_bit(i, slot_device_vga);
> > -        bool qxl = test_bit(i, slot_device_qxl);
> > -        bool system = test_bit(i, slot_device_system);
> > -        if (can_eject) {
> > -            void *pcihp = acpi_data_push(bus_table,
> > -                                         ACPI_PCIHP_SIZEOF);
> > -            memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> > -            patch_pcihp(i, pcihp);
> > -            bus_hotplug_support = true;
> > -        } else if (qxl) {
> > -            void *pcihp = acpi_data_push(bus_table,
> > -                                         ACPI_PCIQXL_SIZEOF);
> > -            memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> > -            patch_pciqxl(i, pcihp);
> > -        } else if (vga) {
> > -            void *pcihp = acpi_data_push(bus_table,
> > -                                         ACPI_PCIVGA_SIZEOF);
> > -            memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> > -            patch_pcivga(i, pcihp);
> > -        } else if (system) {
> > -            /* Nothing to do: system devices are in DSDT or in SSDT above. */
> > -        } else if (present) {
> > -            void *pcihp = acpi_data_push(bus_table,
> > -                                         ACPI_PCINOHP_SIZEOF);
> > -            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> > -            patch_pcinohp(i, pcihp);
> > -        }
> > -    }
> > +                if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> > +                    set_bit(slot, slot_device_qxl);
> > +                }
> > +            }
> >  
> > -    if (bsel) {
> > -        method = build_alloc_method("DVNT", 2);
> > +            if (!dc->hotpluggable || pc->is_bridge) {
> > +                clear_bit(slot, slot_hotplug_enable);
> > +            }
> > +        }
> >  
> > +        /* Append Device object for each slot */
> >          for (i = 0; i < PCI_SLOT_MAX; i++) {
> > -            GArray *notify;
> > -            uint8_t op;
> > -
> > -            if (!test_bit(i, slot_hotplug_enable)) {
> > -                continue;
> > +            bool can_eject = test_bit(i, slot_hotplug_enable);
> > +            bool present = test_bit(i, slot_device_present);
> > +            bool vga = test_bit(i, slot_device_vga);
> > +            bool qxl = test_bit(i, slot_device_qxl);
> > +            bool system = test_bit(i, slot_device_system);
> > +            if (can_eject) {
> > +                void *pcihp = acpi_data_push(bus_table,
> > +                                             ACPI_PCIHP_SIZEOF);
> > +                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> > +                patch_pcihp(i, pcihp);
> > +            } else if (qxl) {
> > +                void *pcihp = acpi_data_push(bus_table,
> > +                                             ACPI_PCIQXL_SIZEOF);
> > +                memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> > +                patch_pciqxl(i, pcihp);
> > +            } else if (vga) {
> > +                void *pcihp = acpi_data_push(bus_table,
> > +                                             ACPI_PCIVGA_SIZEOF);
> > +                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> > +                patch_pcivga(i, pcihp);
> > +            } else if (system) {
> > +                /* Nothing to do: system devices are in DSDT or in SSDT above */
> > +            } else if (present) {
> > +                void *pcihp = acpi_data_push(bus_table,
> > +                                             ACPI_PCINOHP_SIZEOF);
> > +                memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> > +                patch_pcinohp(i, pcihp);
> >              }
> > +        }
> >  
> > -            notify = build_alloc_array();
> > -            op = 0xA0; /* IfOp */
> > -
> > -            build_append_byte(notify, 0x7B); /* AndOp */
> > -            build_append_byte(notify, 0x68); /* Arg0Op */
> > -            build_append_int(notify, 0x1U << i);
> > -            build_append_byte(notify, 0x00); /* NullName */
> > -            build_append_byte(notify, 0x86); /* NotifyOp */
> > -            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> > -            build_append_byte(notify, 0x69); /* Arg1Op */
> > -
> > -            /* Pack it up */
> > -            build_package(notify, op);
> > -
> > -            build_append_array(method, notify);
> > +        if (bsel) {
> > +            method = build_alloc_method("DVNT", 2);
> > +
> > +            for (i = 0; i < PCI_SLOT_MAX; i++) {
> > +                GArray *notify;
> > +                uint8_t op;
> > +
> > +                if (!test_bit(i, slot_hotplug_enable)) {
> > +                    continue;
> > +                }
> > +
> > +                notify = build_alloc_array();
> > +                op = 0xA0; /* IfOp */
> > +
> > +                build_append_byte(notify, 0x7B); /* AndOp */
> > +                build_append_byte(notify, 0x68); /* Arg0Op */
> > +                build_append_int(notify, 0x1U << i);
> > +                build_append_byte(notify, 0x00); /* NullName */
> > +                build_append_byte(notify, 0x86); /* NotifyOp */
> > +                build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> > +                build_append_byte(notify, 0x69); /* Arg1Op */
> > +
> > +                /* Pack it up */
> > +                build_package(notify, op);
> > +                build_append_array(method, notify);
> > +                build_free_array(notify);
> > +            }
> >  
> > -            build_free_array(notify);
> > +            build_append_and_cleanup_method(bus_table, method);
> >          }
> >  
> > -        build_append_and_cleanup_method(bus_table, method);
> > -    }
> > -
> > -    /* Append PCNT method to notify about events on local and child buses.
> > -     * Add unconditionally for root since DSDT expects it.
> > -     */
> > -    if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
> > +        /* Append PCNT method to notify about events on local and child buses.
> > +         * Add unconditionally for root since DSDT expects it.
> > +         */
> >          method = build_alloc_method("PCNT", 0);
> >  
> >          /* If bus supports hotplug select it and notify about local events */
> > @@ -847,36 +837,20 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >          }
> >  
> >          /* Notify about child bus events in any case */
> > -        build_append_array(method, child->notify_table);
> > -
> > -        build_append_and_cleanup_method(bus_table, method);
> > -
> > -        /* Append description of child buses */
> > -        build_append_array(bus_table, child->device_table);
> > -
> > -        /* Pack it up */
> > -        if (bus->parent_dev) {
> > -            build_extop_package(bus_table, op);
> > -        } else {
> > -            build_package(bus_table, op);
> > +        if (pcihp_bridge_en) {
> > +            QLIST_FOREACH(sec, &bus->child, sibling) {
> > +                build_append_namestring(method, "^S%.02X.PCNT",
> > +                                        sec->parent_dev->devfn);
> > +            }
> >          }
> >  
> > -        /* Append our bus description to parent table */
> > -        build_append_array(parent->device_table, bus_table);
> > +        build_append_and_cleanup_method(bus_table, method);
> >  
> > -        /* Also tell parent how to notify us, invoking PCNT method.
> > -         * At the moment this is not needed for root as we have a single root.
> > -         */
> > -        if (bus->parent_dev) {
> > -            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> > -                                 bus->parent_dev->devfn);
> > -        }
> > +        build_package(bus_table, 0x10); /* ScopeOp */
> > +        build_append_array(parent_scope, bus_table);
> > +        build_free_array(bus_table);
> >      }
> > -
> > -    qobject_decref(bsel);
> > -    build_free_array(bus_table);
> > -    build_pci_bus_state_cleanup(child);
> > -    g_free(child);
> > +    g_queue_free(bus_list);
> >  }
> >  
> >  static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
> > @@ -1008,7 +982,6 @@ build_ssdt(GArray *table_data, GArray *linker,
> >          }
> >  
> >          {
> > -            AcpiBuildPciBusHotplugState hotplug_state;
> >              Object *pci_host;
> >              PCIBus *bus = NULL;
> >              bool ambiguous;
> > @@ -1018,16 +991,11 @@ build_ssdt(GArray *table_data, GArray *linker,
> >                  bus = PCI_HOST_BRIDGE(pci_host)->bus;
> >              }
> >  
> > -            build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
> > -
> >              if (bus) {
> >                  /* Scan all PCI buses. Generate tables to support hotplug. */
> > -                pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
> > -                                             build_pci_bus_end, &hotplug_state);
> > +                build_append_pci_bus_devices(sb_scope, bus,
> > +                                             pm->pcihp_bridge_en);
> >              }
> > -
> > -            build_append_array(sb_scope, hotplug_state.device_table);
> > -            build_pci_bus_state_cleanup(&hotplug_state);
> >          }
> >          build_package(sb_scope, op);
> >          build_append_array(table_data, sb_scope);
> > -- 
> > 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 9/9] pc: acpi-build: replace recursive PCI bus tree generation with loop based

[Michael S. Tsirkin]

On Tue, Dec 09, 2014 at 03:01:00PM +0100, Igor Mammedov wrote:
> On Mon, 8 Dec 2014 22:43:24 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Dec 08, 2014 at 04:08:08PM +0000, Igor Mammedov wrote:
> > > it replaces PCI tree structure in SSDT with a set of scopes
> > > describing each PCI bus as a separate scope with a child devices.
> > > It makes code easier to follow and a little bit smaller.
> > > 
> > > In addition it makes simplier to convert current template
> > > patching approach to completely dynamically generated SSDT
> > > without dependency on IASL, which would allow to drop
> > > template binary blobs from source tree.
> > > 
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > ---
> > >  hw/i386/acpi-build.c | 362 +++++++++++++++++++++++----------------------------
> > >  1 file changed, 165 insertions(+), 197 deletions(-)
> > 
> > I like it that your patch makes code smaller and simpler,
> > but I think we do need to generate hierarchical AML.
> If only reason for hierarchical AML is table size then
> I don't see how scopes could possibly double the table size,
> Compared to tree version scope adds only:
> 
>  ~5 bytes for package + NamePath(~[0-2] + 4 * tree depth)

No, tree depth squared since you generate this per bridge.

> Which is almost nothing compared to slots description
> per a bridge.
> One needs to configure insanely deep bridge nesting
> for scope cost to significantly affect table size, which
> doesn't sound as practical thing to do anyway except of
> testing limitation on how many nested bridges QEMU would
> stomach.

pci supports up to 256 nested bridges.
So that's 256K which isn't the end of the world but
not negligeable for a bios.

> > 
> > I think this can still be done with some modifications to
> > the logic you have here.
> > 
> > Basically current code does all work in build_pci_bus_end.
> > It follows that you can do the same by simply walking
> > the list in reverse order.
> I've thought about it already, but tree creation means
> creating temporary contexts for each scope, copying them
> to parent context, keeping track of contexts to build
> correct notifiers. It's basically the same or even worse
> as current code just other way around.

Well, the way I see it, the main simplification is
that you can pass data from child to parent explicitly.

Maybe I'm not explaining this properly, I'll try to
find the time to send a patch for comparison.

> And all that makes code quite hard to follow and
> maintain, that is the main reason to drop recursion in
> favor of simple flat scope sets.

I agree the approach of unrolling the tree, then walking the list has
advantages, but if it's forcing a specific AML structure then that
really means it's too limited.

> And it makes easier to
> switch from AML+template patching to ASL API later
> and hide from user direct access to table array, forcing
> him to work only with ASL constructs when building table.

I don't necessarily see how these two ideas are related.

> > 
> > 
> > 
> > 
> > 
> > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > > index 97ff245..7606a73 100644
> > > --- a/hw/i386/acpi-build.c
> > > +++ b/hw/i386/acpi-build.c
> > > @@ -641,196 +641,186 @@ static void acpi_set_pci_info(void)
> > >      }
> > >  }
> > >  
> > > -static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
> > > -                                     AcpiBuildPciBusHotplugState *parent,
> > > -                                     bool pcihp_bridge_en)
> > > +static char *pci_bus_get_scope_name(PCIBus *bus)
> > >  {
> > > -    state->parent = parent;
> > > -    state->device_table = build_alloc_array();
> > > -    state->notify_table = build_alloc_array();
> > > -    state->pcihp_bridge_en = pcihp_bridge_en;
> > > -}
> > > +    char *name = NULL;
> > > +    char *last = name;
> > >  
> > > -static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
> > > -{
> > > -    build_free_array(state->device_table);
> > > -    build_free_array(state->notify_table);
> > > -}
> > > +    while (bus->parent_dev) {
> > > +        last = name;
> > > +        name = g_strdup_printf("%s.S%.02X_",
> > > +                               name ? name : "", bus->parent_dev->devfn);
> > > +        g_free(last);
> > >  
> > > -static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
> > > -{
> > > -    AcpiBuildPciBusHotplugState *parent = parent_state;
> > > -    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
> > > +        bus = bus->parent_dev->bus;
> > > +    }
> > >  
> > > -    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
> > > +    last = name;
> > > +    name = g_strdup_printf("PCI0%s", name ? name : "");
> > > +    g_free(last);
> > >  
> > > -    return child;
> > > +    return name;
> > >  }
> > >  
> > > -static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > > +static GQueue *get_pci_bus_list(PCIBus *bus, bool pcihp_bridge_en)
> > >  {
> > > -    AcpiBuildPciBusHotplugState *child = bus_state;
> > > -    AcpiBuildPciBusHotplugState *parent = child->parent;
> > > -    GArray *bus_table = build_alloc_array();
> > > -    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> > > -    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> > > -    DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> > > -    DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> > > -    DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> > > -    uint8_t op;
> > > -    int i;
> > > -    QObject *bsel;
> > > -    GArray *method;
> > > -    bool bus_hotplug_support = false;
> > > -
> > > -    /*
> > > -     * Skip bridge subtree creation if bridge hotplug is disabled
> > > -     * to make acpi tables compatible with legacy machine types.
> > > -     */
> > > -    if (!child->pcihp_bridge_en && bus->parent_dev) {
> > > -        return;
> > > -    }
> > > -
> > > -    if (bus->parent_dev) {
> > > -        op = 0x82; /* DeviceOp */
> > > -        build_append_namestring(bus_table, "S%.02X",
> > > -                             bus->parent_dev->devfn);
> > > -        build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_namestring(bus_table, "_SUN");
> > > -        build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> > > -        build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_namestring(bus_table, "_ADR");
> > > -        build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> > > -                           PCI_FUNC(bus->parent_dev->devfn), 4);
> > > -    } else {
> > > -        op = 0x10; /* ScopeOp */;
> > > -        build_append_namestring(bus_table, "PCI0");
> > > -    }
> > > -
> > > -    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> > > -    if (bsel) {
> > > -        build_append_byte(bus_table, 0x08); /* NameOp */
> > > -        build_append_namestring(bus_table, "BSEL");
> > > -        build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> > > -        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> > > -    } else {
> > > -        /* No bsel - no slots are hot-pluggable */
> > > -        memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
> > > +    GQueue *bus_list = g_queue_new();
> > > +    GQueue *tree_walk = g_queue_new();
> > > +
> > > +    /* build BUS list */
> > > +    g_queue_push_tail(tree_walk, bus);
> > > +    while (!g_queue_is_empty(tree_walk)) {
> > > +        PCIBus *sec;
> > > +        PCIBus *parent = g_queue_pop_tail(tree_walk);
> > > +
> > > +        g_queue_push_tail(bus_list, parent);
> > > +        if (!pcihp_bridge_en) {
> > > +            break;
> > > +        }
> > > +        QLIST_FOREACH(sec, &parent->child, sibling) {
> > > +            g_queue_push_tail(tree_walk, sec);
> > > +        }
> > >      }
> > > +    g_queue_free(tree_walk);
> > > +    return bus_list;
> > > +}
> > >  
> > > -    memset(slot_device_present, 0x00, sizeof slot_device_present);
> > > -    memset(slot_device_system, 0x00, sizeof slot_device_present);
> > > -    memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> > > -    memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
> > > -
> > > -    for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> > > -        DeviceClass *dc;
> > > -        PCIDeviceClass *pc;
> > > -        PCIDevice *pdev = bus->devices[i];
> > > -        int slot = PCI_SLOT(i);
> > > -        bool bridge_in_acpi;
> > > -
> > > -        if (!pdev) {
> > > -            continue;
> > > +static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
> > > +                                         bool pcihp_bridge_en)
> > > +{
> > > +    GQueue *bus_list = get_pci_bus_list(bus, pcihp_bridge_en);
> > > +
> > > +    while (!g_queue_is_empty(bus_list)) {
> > > +        DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> > > +        DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> > > +        DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> > > +        DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> > > +        DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> > > +        GArray *bus_table = build_alloc_array();
> > > +        PCIBus *bus = g_queue_pop_head(bus_list);
> > > +        GArray *method;
> > > +        QObject *bsel;
> > > +        PCIBus *sec;
> > > +        int i;
> > > +
> > > +        char *scope_name = pci_bus_get_scope_name(bus);
> > > +        build_append_namestring(bus_table, "%s", scope_name);
> > > +        g_free(scope_name);
> > > +
> > > +        bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
> > > +                                           NULL);
> > > +        if (bsel) {
> > > +            build_append_byte(bus_table, 0x08); /* NameOp */
> > > +            build_append_namestring(bus_table, "BSEL");
> > > +            build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> > > +            memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> > > +        } else {
> > > +            /* No bsel - no slots are hot-pluggable */
> > > +            memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
> > >          }
> > >  
> > > -        set_bit(slot, slot_device_present);
> > > -        pc = PCI_DEVICE_GET_CLASS(pdev);
> > > -        dc = DEVICE_GET_CLASS(pdev);
> > > +        memset(slot_device_present, 0x00, sizeof slot_device_present);
> > > +        memset(slot_device_system, 0x00, sizeof slot_device_present);
> > > +        memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> > > +        memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
> > >  
> > > -        /* When hotplug for bridges is enabled, bridges are
> > > -         * described in ACPI separately (see build_pci_bus_end).
> > > -         * In this case they aren't themselves hot-pluggable.
> > > -         */
> > > -        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
> > > +        for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> > > +            DeviceClass *dc;
> > > +            PCIDeviceClass *pc;
> > > +            PCIDevice *pdev = bus->devices[i];
> > > +            int slot = PCI_SLOT(i);
> > >  
> > > -        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
> > > -            set_bit(slot, slot_device_system);
> > > -        }
> > > +            if (!pdev) {
> > > +                continue;
> > > +            }
> > >  
> > > -        if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> > > -            set_bit(slot, slot_device_vga);
> > > +            set_bit(slot, slot_device_present);
> > > +            pc = PCI_DEVICE_GET_CLASS(pdev);
> > > +            dc = DEVICE_GET_CLASS(pdev);
> > >  
> > > -            if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> > > -                set_bit(slot, slot_device_qxl);
> > > +            if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
> > > +                set_bit(slot, slot_device_system);
> > >              }
> > > -        }
> > >  
> > > -        if (!dc->hotpluggable || pc->is_bridge) {
> > > -            clear_bit(slot, slot_hotplug_enable);
> > > -        }
> > > -    }
> > > +            if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> > > +                set_bit(slot, slot_device_vga);
> > >  
> > > -    /* Append Device object for each slot */
> > > -    for (i = 0; i < PCI_SLOT_MAX; i++) {
> > > -        bool can_eject = test_bit(i, slot_hotplug_enable);
> > > -        bool present = test_bit(i, slot_device_present);
> > > -        bool vga = test_bit(i, slot_device_vga);
> > > -        bool qxl = test_bit(i, slot_device_qxl);
> > > -        bool system = test_bit(i, slot_device_system);
> > > -        if (can_eject) {
> > > -            void *pcihp = acpi_data_push(bus_table,
> > > -                                         ACPI_PCIHP_SIZEOF);
> > > -            memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> > > -            patch_pcihp(i, pcihp);
> > > -            bus_hotplug_support = true;
> > > -        } else if (qxl) {
> > > -            void *pcihp = acpi_data_push(bus_table,
> > > -                                         ACPI_PCIQXL_SIZEOF);
> > > -            memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> > > -            patch_pciqxl(i, pcihp);
> > > -        } else if (vga) {
> > > -            void *pcihp = acpi_data_push(bus_table,
> > > -                                         ACPI_PCIVGA_SIZEOF);
> > > -            memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> > > -            patch_pcivga(i, pcihp);
> > > -        } else if (system) {
> > > -            /* Nothing to do: system devices are in DSDT or in SSDT above. */
> > > -        } else if (present) {
> > > -            void *pcihp = acpi_data_push(bus_table,
> > > -                                         ACPI_PCINOHP_SIZEOF);
> > > -            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> > > -            patch_pcinohp(i, pcihp);
> > > -        }
> > > -    }
> > > +                if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> > > +                    set_bit(slot, slot_device_qxl);
> > > +                }
> > > +            }
> > >  
> > > -    if (bsel) {
> > > -        method = build_alloc_method("DVNT", 2);
> > > +            if (!dc->hotpluggable || pc->is_bridge) {
> > > +                clear_bit(slot, slot_hotplug_enable);
> > > +            }
> > > +        }
> > >  
> > > +        /* Append Device object for each slot */
> > >          for (i = 0; i < PCI_SLOT_MAX; i++) {
> > > -            GArray *notify;
> > > -            uint8_t op;
> > > -
> > > -            if (!test_bit(i, slot_hotplug_enable)) {
> > > -                continue;
> > > +            bool can_eject = test_bit(i, slot_hotplug_enable);
> > > +            bool present = test_bit(i, slot_device_present);
> > > +            bool vga = test_bit(i, slot_device_vga);
> > > +            bool qxl = test_bit(i, slot_device_qxl);
> > > +            bool system = test_bit(i, slot_device_system);
> > > +            if (can_eject) {
> > > +                void *pcihp = acpi_data_push(bus_table,
> > > +                                             ACPI_PCIHP_SIZEOF);
> > > +                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> > > +                patch_pcihp(i, pcihp);
> > > +            } else if (qxl) {
> > > +                void *pcihp = acpi_data_push(bus_table,
> > > +                                             ACPI_PCIQXL_SIZEOF);
> > > +                memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> > > +                patch_pciqxl(i, pcihp);
> > > +            } else if (vga) {
> > > +                void *pcihp = acpi_data_push(bus_table,
> > > +                                             ACPI_PCIVGA_SIZEOF);
> > > +                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> > > +                patch_pcivga(i, pcihp);
> > > +            } else if (system) {
> > > +                /* Nothing to do: system devices are in DSDT or in SSDT above */
> > > +            } else if (present) {
> > > +                void *pcihp = acpi_data_push(bus_table,
> > > +                                             ACPI_PCINOHP_SIZEOF);
> > > +                memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> > > +                patch_pcinohp(i, pcihp);
> > >              }
> > > +        }
> > >  
> > > -            notify = build_alloc_array();
> > > -            op = 0xA0; /* IfOp */
> > > -
> > > -            build_append_byte(notify, 0x7B); /* AndOp */
> > > -            build_append_byte(notify, 0x68); /* Arg0Op */
> > > -            build_append_int(notify, 0x1U << i);
> > > -            build_append_byte(notify, 0x00); /* NullName */
> > > -            build_append_byte(notify, 0x86); /* NotifyOp */
> > > -            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> > > -            build_append_byte(notify, 0x69); /* Arg1Op */
> > > -
> > > -            /* Pack it up */
> > > -            build_package(notify, op);
> > > -
> > > -            build_append_array(method, notify);
> > > +        if (bsel) {
> > > +            method = build_alloc_method("DVNT", 2);
> > > +
> > > +            for (i = 0; i < PCI_SLOT_MAX; i++) {
> > > +                GArray *notify;
> > > +                uint8_t op;
> > > +
> > > +                if (!test_bit(i, slot_hotplug_enable)) {
> > > +                    continue;
> > > +                }
> > > +
> > > +                notify = build_alloc_array();
> > > +                op = 0xA0; /* IfOp */
> > > +
> > > +                build_append_byte(notify, 0x7B); /* AndOp */
> > > +                build_append_byte(notify, 0x68); /* Arg0Op */
> > > +                build_append_int(notify, 0x1U << i);
> > > +                build_append_byte(notify, 0x00); /* NullName */
> > > +                build_append_byte(notify, 0x86); /* NotifyOp */
> > > +                build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> > > +                build_append_byte(notify, 0x69); /* Arg1Op */
> > > +
> > > +                /* Pack it up */
> > > +                build_package(notify, op);
> > > +                build_append_array(method, notify);
> > > +                build_free_array(notify);
> > > +            }
> > >  
> > > -            build_free_array(notify);
> > > +            build_append_and_cleanup_method(bus_table, method);
> > >          }
> > >  
> > > -        build_append_and_cleanup_method(bus_table, method);
> > > -    }
> > > -
> > > -    /* Append PCNT method to notify about events on local and child buses.
> > > -     * Add unconditionally for root since DSDT expects it.
> > > -     */
> > > -    if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
> > > +        /* Append PCNT method to notify about events on local and child buses.
> > > +         * Add unconditionally for root since DSDT expects it.
> > > +         */
> > >          method = build_alloc_method("PCNT", 0);
> > >  
> > >          /* If bus supports hotplug select it and notify about local events */
> > > @@ -847,36 +837,20 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > >          }
> > >  
> > >          /* Notify about child bus events in any case */
> > > -        build_append_array(method, child->notify_table);
> > > -
> > > -        build_append_and_cleanup_method(bus_table, method);
> > > -
> > > -        /* Append description of child buses */
> > > -        build_append_array(bus_table, child->device_table);
> > > -
> > > -        /* Pack it up */
> > > -        if (bus->parent_dev) {
> > > -            build_extop_package(bus_table, op);
> > > -        } else {
> > > -            build_package(bus_table, op);
> > > +        if (pcihp_bridge_en) {
> > > +            QLIST_FOREACH(sec, &bus->child, sibling) {
> > > +                build_append_namestring(method, "^S%.02X.PCNT",
> > > +                                        sec->parent_dev->devfn);
> > > +            }
> > >          }
> > >  
> > > -        /* Append our bus description to parent table */
> > > -        build_append_array(parent->device_table, bus_table);
> > > +        build_append_and_cleanup_method(bus_table, method);
> > >  
> > > -        /* Also tell parent how to notify us, invoking PCNT method.
> > > -         * At the moment this is not needed for root as we have a single root.
> > > -         */
> > > -        if (bus->parent_dev) {
> > > -            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> > > -                                 bus->parent_dev->devfn);
> > > -        }
> > > +        build_package(bus_table, 0x10); /* ScopeOp */
> > > +        build_append_array(parent_scope, bus_table);
> > > +        build_free_array(bus_table);
> > >      }
> > > -
> > > -    qobject_decref(bsel);
> > > -    build_free_array(bus_table);
> > > -    build_pci_bus_state_cleanup(child);
> > > -    g_free(child);
> > > +    g_queue_free(bus_list);
> > >  }
> > >  
> > >  static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
> > > @@ -1008,7 +982,6 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >          }
> > >  
> > >          {
> > > -            AcpiBuildPciBusHotplugState hotplug_state;
> > >              Object *pci_host;
> > >              PCIBus *bus = NULL;
> > >              bool ambiguous;
> > > @@ -1018,16 +991,11 @@ build_ssdt(GArray *table_data, GArray *linker,
> > >                  bus = PCI_HOST_BRIDGE(pci_host)->bus;
> > >              }
> > >  
> > > -            build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
> > > -
> > >              if (bus) {
> > >                  /* Scan all PCI buses. Generate tables to support hotplug. */
> > > -                pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
> > > -                                             build_pci_bus_end, &hotplug_state);
> > > +                build_append_pci_bus_devices(sb_scope, bus,
> > > +                                             pm->pcihp_bridge_en);
> > >              }
> > > -
> > > -            build_append_array(sb_scope, hotplug_state.device_table);
> > > -            build_pci_bus_state_cleanup(&hotplug_state);
> > >          }
> > >          build_package(sb_scope, op);
> > >          build_append_array(table_data, sb_scope);
> > > -- 
> > > 1.8.3.1
> > 

[Qemu-devel] [PATCH V2 9/9] pc: acpi-build: simplify PCI bus tree generation

[Igor Mammedov]

it basicaly does the same as original approach,
* just without bus/notify tables tracking (less obscure)
  which is easier to follow.
* drops unnecessary loops and bitmaps,
  creating devices and notification method in the same loop.
* saves us ~100LOC

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 262 ++++++++++++++++-----------------------------------
 1 file changed, 80 insertions(+), 182 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a615cd1..6b0e983 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -95,7 +95,6 @@ typedef struct AcpiPmInfo {
 typedef struct AcpiMiscInfo {
     bool has_hpet;
     bool has_tpm;
-    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
     const unsigned char *dsdt_code;
     unsigned dsdt_size;
     uint16_t pvpanic_port;
@@ -641,242 +640,147 @@ static void acpi_set_pci_info(void)
     }
 }
 
-static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
-                                     AcpiBuildPciBusHotplugState *parent,
-                                     bool pcihp_bridge_en)
+static void build_append_pcihp_notify(GArray *method, int slot)
 {
-    state->parent = parent;
-    state->device_table = build_alloc_array();
-    state->notify_table = build_alloc_array();
-    state->pcihp_bridge_en = pcihp_bridge_en;
-}
+    GArray *notify;
 
-static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
-{
-    build_free_array(state->device_table);
-    build_free_array(state->notify_table);
+    notify = build_alloc_array();
+    build_append_byte(notify, 0x7B); /* AndOp */
+    build_append_byte(notify, 0x68); /* Arg0Op */
+    build_append_int(notify, 0x1U << slot);
+    build_append_byte(notify, 0x00); /* NullName */
+    build_append_byte(notify, 0x86); /* NotifyOp */
+    build_append_namestring(notify, "S%.02X", PCI_DEVFN(slot, 0));
+    build_append_byte(notify, 0x69); /* Arg1Op */
+
+    /* Pack it up */
+    build_package(notify, 0xA0 /* IfOp */);
+    build_append_array(method, notify);
+    build_free_array(notify);
 }
 
-static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
+static void build_append_pcihp_dev(GArray *table, int slot)
 {
-    AcpiBuildPciBusHotplugState *parent = parent_state;
-    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
-
-    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
+    void *pcihp = acpi_data_push(table, ACPI_PCINOHP_SIZEOF);
 
-    return child;
+    memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
+    patch_pcinohp(slot, pcihp);
 }
 
-static void build_pci_bus_end(PCIBus *bus, void *bus_state)
+static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
+                                         bool pcihp_bridge_en)
 {
-    AcpiBuildPciBusHotplugState *child = bus_state;
-    AcpiBuildPciBusHotplugState *parent = child->parent;
     GArray *bus_table = build_alloc_array();
-    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
-    uint8_t op;
-    int i;
+    GArray *method = NULL;
     QObject *bsel;
-    GArray *method;
-    bool bus_hotplug_support = false;
-
-    /*
-     * Skip bridge subtree creation if bridge hotplug is disabled
-     * to make acpi tables compatible with legacy machine types.
-     */
-    if (!child->pcihp_bridge_en && bus->parent_dev) {
-        return;
-    }
+    PCIBus *sec;
+    int i;
 
     if (bus->parent_dev) {
-        op = 0x82; /* DeviceOp */
-        build_append_namestring(bus_table, "S%.02X",
-                             bus->parent_dev->devfn);
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "_SUN");
-        build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "_ADR");
-        build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
-                           PCI_FUNC(bus->parent_dev->devfn), 4);
+        build_append_namestring(bus_table, "S%.02X_", bus->parent_dev->devfn);
     } else {
-        op = 0x10; /* ScopeOp */;
         build_append_namestring(bus_table, "PCI0");
     }
 
-    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
+    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
+                                       NULL);
     if (bsel) {
         build_append_byte(bus_table, 0x08); /* NameOp */
         build_append_namestring(bus_table, "BSEL");
         build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
-        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
-    } else {
-        /* No bsel - no slots are hot-pluggable */
-        memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
+        method = build_alloc_method("DVNT", 2);
     }
 
-    memset(slot_device_present, 0x00, sizeof slot_device_present);
-    memset(slot_device_system, 0x00, sizeof slot_device_present);
-    memset(slot_device_vga, 0x00, sizeof slot_device_vga);
-    memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
-
     for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
         DeviceClass *dc;
         PCIDeviceClass *pc;
         PCIDevice *pdev = bus->devices[i];
         int slot = PCI_SLOT(i);
-        bool bridge_in_acpi;
 
         if (!pdev) {
+            build_append_pcihp_dev(bus_table, slot);
+            if (method) {
+                build_append_pcihp_notify(method, slot);
+            }
             continue;
         }
 
-        set_bit(slot, slot_device_present);
         pc = PCI_DEVICE_GET_CLASS(pdev);
         dc = DEVICE_GET_CLASS(pdev);
 
-        /* When hotplug for bridges is enabled, bridges are
-         * described in ACPI separately (see build_pci_bus_end).
-         * In this case they aren't themselves hot-pluggable.
-         */
-        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
-
-        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
-            set_bit(slot, slot_device_system);
+        if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
+            continue;
         }
 
         if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
-            set_bit(slot, slot_device_vga);
 
             if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
-                set_bit(slot, slot_device_qxl);
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIQXL_SIZEOF);
+                      memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
+                      patch_pciqxl(slot, pcihp);
+            } else {
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIVGA_SIZEOF);
+                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
+                patch_pcivga(slot, pcihp);
             }
-        }
-
-        if (!dc->hotpluggable || pc->is_bridge) {
-            clear_bit(slot, slot_hotplug_enable);
-        }
-    }
-
-    /* Append Device object for each slot */
-    for (i = 0; i < PCI_SLOT_MAX; i++) {
-        bool can_eject = test_bit(i, slot_hotplug_enable);
-        bool present = test_bit(i, slot_device_present);
-        bool vga = test_bit(i, slot_device_vga);
-        bool qxl = test_bit(i, slot_device_qxl);
-        bool system = test_bit(i, slot_device_system);
-        if (can_eject) {
+        } else if (dc->hotpluggable && !pc->is_bridge) {
             void *pcihp = acpi_data_push(bus_table,
                                          ACPI_PCIHP_SIZEOF);
             memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
-            patch_pcihp(i, pcihp);
-            bus_hotplug_support = true;
-        } else if (qxl) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIQXL_SIZEOF);
-            memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
-            patch_pciqxl(i, pcihp);
-        } else if (vga) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIVGA_SIZEOF);
-            memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
-            patch_pcivga(i, pcihp);
-        } else if (system) {
-            /* Nothing to do: system devices are in DSDT or in SSDT above. */
-        } else if (present) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCINOHP_SIZEOF);
-            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
-            patch_pcinohp(i, pcihp);
-        }
-    }
-
-    if (bsel) {
-        method = build_alloc_method("DVNT", 2);
-
-        for (i = 0; i < PCI_SLOT_MAX; i++) {
-            GArray *notify;
-            uint8_t op;
+            patch_pcihp(slot, pcihp);
 
-            if (!test_bit(i, slot_hotplug_enable)) {
-                continue;
+            if (method) {
+                build_append_pcihp_notify(method, slot);
             }
+        } else {
+            build_append_pcihp_dev(bus_table, slot);
 
-            notify = build_alloc_array();
-            op = 0xA0; /* IfOp */
-
-            build_append_byte(notify, 0x7B); /* AndOp */
-            build_append_byte(notify, 0x68); /* Arg0Op */
-            build_append_int(notify, 0x1U << i);
-            build_append_byte(notify, 0x00); /* NullName */
-            build_append_byte(notify, 0x86); /* NotifyOp */
-            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
-            build_append_byte(notify, 0x69); /* Arg1Op */
-
-            /* Pack it up */
-            build_package(notify, op);
-
-            build_append_array(method, notify);
+            if (pc->is_bridge) {
+                PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
 
-            build_free_array(notify);
+                build_append_pci_bus_devices(bus_table, sec_bus,
+                                             pcihp_bridge_en);
+            }
         }
+    }
 
+    if (method) {
         build_append_and_cleanup_method(bus_table, method);
     }
 
     /* Append PCNT method to notify about events on local and child buses.
      * Add unconditionally for root since DSDT expects it.
      */
-    if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
-        method = build_alloc_method("PCNT", 0);
-
-        /* If bus supports hotplug select it and notify about local events */
-        if (bsel) {
-            build_append_byte(method, 0x70); /* StoreOp */
-            build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
-            build_append_namestring(method, "BNUM");
-            build_append_namestring(method, "DVNT");
-            build_append_namestring(method, "PCIU");
-            build_append_int(method, 1); /* Device Check */
-            build_append_namestring(method, "DVNT");
-            build_append_namestring(method, "PCID");
-            build_append_int(method, 3); /* Eject Request */
-        }
-
-        /* Notify about child bus events in any case */
-        build_append_array(method, child->notify_table);
-
-        build_append_and_cleanup_method(bus_table, method);
-
-        /* Append description of child buses */
-        build_append_array(bus_table, child->device_table);
+    method = build_alloc_method("PCNT", 0);
 
-        /* Pack it up */
-        if (bus->parent_dev) {
-            build_extop_package(bus_table, op);
-        } else {
-            build_package(bus_table, op);
-        }
-
-        /* Append our bus description to parent table */
-        build_append_array(parent->device_table, bus_table);
+    /* If bus supports hotplug select it and notify about local events */
+    if (bsel) {
+        build_append_byte(method, 0x70); /* StoreOp */
+        build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
+        build_append_namestring(method, "BNUM");
+        build_append_namestring(method, "DVNT");
+        build_append_namestring(method, "PCIU");
+        build_append_int(method, 1); /* Device Check */
+        build_append_namestring(method, "DVNT");
+        build_append_namestring(method, "PCID");
+        build_append_int(method, 3); /* Eject Request */
+    }
 
-        /* Also tell parent how to notify us, invoking PCNT method.
-         * At the moment this is not needed for root as we have a single root.
-         */
-        if (bus->parent_dev) {
-            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
-                                 bus->parent_dev->devfn);
+    /* Notify about child bus events in any case */
+    if (pcihp_bridge_en) {
+        QLIST_FOREACH(sec, &bus->child, sibling) {
+            build_append_namestring(method, "^S%.02X.PCNT",
+                                    sec->parent_dev->devfn);
         }
     }
 
-    qobject_decref(bsel);
+    build_append_and_cleanup_method(bus_table, method);
+
+    build_package(bus_table, 0x10); /* ScopeOp */
+    build_append_array(parent_scope, bus_table);
     build_free_array(bus_table);
-    build_pci_bus_state_cleanup(child);
-    g_free(child);
 }
 
 static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
@@ -1008,7 +912,6 @@ build_ssdt(GArray *table_data, GArray *linker,
         }
 
         {
-            AcpiBuildPciBusHotplugState hotplug_state;
             Object *pci_host;
             PCIBus *bus = NULL;
             bool ambiguous;
@@ -1018,16 +921,11 @@ build_ssdt(GArray *table_data, GArray *linker,
                 bus = PCI_HOST_BRIDGE(pci_host)->bus;
             }
 
-            build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
-
             if (bus) {
                 /* Scan all PCI buses. Generate tables to support hotplug. */
-                pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
-                                             build_pci_bus_end, &hotplug_state);
+                build_append_pci_bus_devices(sb_scope, bus,
+                                             pm->pcihp_bridge_en);
             }
-
-            build_append_array(sb_scope, hotplug_state.device_table);
-            build_pci_bus_state_cleanup(&hotplug_state);
         }
         build_package(sb_scope, op);
         build_append_array(table_data, sb_scope);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/9] pc: acpi: various fixes and cleanups

[Michael S. Tsirkin]

On Mon, Dec 08, 2014 at 04:07:59PM +0000, Igor Mammedov wrote:
> this series is an early attempt to shave off a bunch of
> not directly related patches from already big dynamic
> AML series (although it's dependency for it)
> 
> main target of this series is:
> 
> pc: acpi-build: replace recursive PCI bus tree generation with loop
>     based
> 
> A simplified PCI description generation, which replaces tree
> structure with a set of ACPI scopes and allows later easily replace
> template patching with direct AML generation with further
> simplification.

I considered this early on.
Unfortunately using scopes almost doubles the
ACPI size for deep hierarchies.

I'm fine with flattening the generation code
but I think we are better off keeping the AML hierarchical.

More on the patch itself.


> Tested: on XPsp3 to WS2012R2 and REHL6/7 guests.
> 
> Git tree for testing:
>  https://github.com/imammedo/qemu/commits/acpi_pci_gen_simplification
> 
> 
> Igor Mammedov (9):
>   pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled
>   pc: acpi: decribe bridge device as not hotpluggable
>   pc: acpi-build: cleanup AcpiPmInfo initialization
>   acpi: build_append_nameseg(): add padding if necessary
>   acpi: move generic aml building helpers into dedictated file
>   acpi: add build_append_namestring() helper
>   acpi: replace opencoded notify codes with named values
>   acpi: drop min-bytes in build_package()
>   pc: acpi-build: replace recursive PCI bus tree generation with loop
>     based
> 
>  hw/acpi/Makefile.objs             |   1 +
>  hw/acpi/acpi_gen_utils.c          | 247 +++++++++++++++++
>  hw/i386/acpi-build.c              | 561 +++++++++++++-------------------------
>  hw/i386/acpi-dsdt-cpu-hotplug.dsl |   1 +
>  include/hw/acpi/acpi_gen_utils.h  |  29 ++
>  5 files changed, 462 insertions(+), 377 deletions(-)
>  create mode 100644 hw/acpi/acpi_gen_utils.c
>  create mode 100644 include/hw/acpi/acpi_gen_utils.h
> 
> -- 
> 1.8.3.1