| Message ID | 1425567322-8337-11-git-send-email-marcel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > --- > hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 78 insertions(+) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index e5709e8..f0401d2 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > aml_append(parent_scope, method); > } > > +static Aml *build_prt(void) > +{ > + Aml *method, *pkg, *if_ctx, *while_ctx; > + > + method = aml_method("_PRT", 0); > + > + aml_append(method, aml_store(aml_package(128), aml_local(0))); > + aml_append(method, aml_store(aml_int(0), aml_local(1))); > + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); > + { > + aml_append(while_ctx, > + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); > + aml_append(while_ctx, > + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), > + aml_local(3))); > + > + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); > + { > + pkg = aml_package(4); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_name("LNKD")); > + aml_append(pkg, aml_int(0)); > + aml_append(if_ctx, aml_store(pkg, aml_local(4))); > + } > + aml_append(while_ctx, if_ctx); > + > + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); > + { > + pkg = aml_package(4); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_name("LNKA")); > + aml_append(pkg, aml_int(0)); > + aml_append(if_ctx, aml_store(pkg, aml_local(4))); > + } > + aml_append(while_ctx, if_ctx); > + > + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); > + { > + pkg = aml_package(4); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_name("LNKB")); > + aml_append(pkg, aml_int(0)); > + aml_append(if_ctx, aml_store(pkg, aml_local(4))); > + } > + aml_append(while_ctx, if_ctx); > + > + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); > + { > + pkg = aml_package(4); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_name("LNKC")); > + aml_append(pkg, aml_int(0)); > + aml_append(if_ctx, aml_store(pkg, aml_local(4))); > + } > + aml_append(while_ctx, if_ctx); > + > + aml_append(while_ctx, > + aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), > + aml_int(0xFFFF)), > + aml_index(aml_local(4), aml_int(0)))); > + aml_append(while_ctx, > + aml_store(aml_and(aml_local(1), aml_int(3)), > + aml_index(aml_local(4), aml_int(1)))); > + aml_append(while_ctx, > + aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); > + aml_append(while_ctx, aml_increment(aml_local(1))); > + } > + aml_append(method, while_ctx); > + aml_append(method, aml_return(aml_local(0))); > + > + return method; > +} > + Pls improve readability of this code using comments, sub-functions and local variables. > static void > build_ssdt(GArray *table_data, GArray *linker, > AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, > @@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, > aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); > aml_append(dev, > aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); > + aml_append(dev, build_prt()); > aml_append(scope, dev); > aml_append(ssdt, scope); > } > -- > 2.1.0
On 03/05/2015 09:52 PM, Michael S. Tsirkin wrote: > On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >> --- >> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 78 insertions(+) >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index e5709e8..f0401d2 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, >> aml_append(parent_scope, method); >> } >> >> +static Aml *build_prt(void) >> +{ >> + Aml *method, *pkg, *if_ctx, *while_ctx; >> + >> + method = aml_method("_PRT", 0); >> + >> + aml_append(method, aml_store(aml_package(128), aml_local(0))); >> + aml_append(method, aml_store(aml_int(0), aml_local(1))); >> + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); >> + { >> + aml_append(while_ctx, >> + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); >> + aml_append(while_ctx, >> + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), >> + aml_local(3))); >> + >> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); >> + { >> + pkg = aml_package(4); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_name("LNKD")); >> + aml_append(pkg, aml_int(0)); >> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >> + } >> + aml_append(while_ctx, if_ctx); >> + >> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); >> + { >> + pkg = aml_package(4); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_name("LNKA")); >> + aml_append(pkg, aml_int(0)); >> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >> + } >> + aml_append(while_ctx, if_ctx); >> + >> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); >> + { >> + pkg = aml_package(4); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_name("LNKB")); >> + aml_append(pkg, aml_int(0)); >> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >> + } >> + aml_append(while_ctx, if_ctx); >> + >> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); >> + { >> + pkg = aml_package(4); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_name("LNKC")); >> + aml_append(pkg, aml_int(0)); >> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >> + } >> + aml_append(while_ctx, if_ctx); >> + >> + aml_append(while_ctx, >> + aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), >> + aml_int(0xFFFF)), >> + aml_index(aml_local(4), aml_int(0)))); >> + aml_append(while_ctx, >> + aml_store(aml_and(aml_local(1), aml_int(3)), >> + aml_index(aml_local(4), aml_int(1)))); >> + aml_append(while_ctx, >> + aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); >> + aml_append(while_ctx, aml_increment(aml_local(1))); >> + } >> + aml_append(method, while_ctx); >> + aml_append(method, aml_return(aml_local(0))); >> + >> + return method; >> +} >> + > > Pls improve readability of this code using comments, sub-functions and > local variables. It is the exact "copy" of the static aml code we had, witch by itself wasn't so nice, so it is so much I can do. However, I'll try to improve it. Thanks, Marcel > > >> static void >> build_ssdt(GArray *table_data, GArray *linker, >> AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, >> @@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, >> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); >> aml_append(dev, >> aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); >> + aml_append(dev, build_prt()); >> aml_append(scope, dev); >> aml_append(ssdt, scope); >> } >> -- >> 2.1.0
On Thu, Mar 05, 2015 at 11:55:34PM +0200, Marcel Apfelbaum wrote: > On 03/05/2015 09:52 PM, Michael S. Tsirkin wrote: > >On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: > >>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > >>--- > >> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 78 insertions(+) > >> > >>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > >>index e5709e8..f0401d2 100644 > >>--- a/hw/i386/acpi-build.c > >>+++ b/hw/i386/acpi-build.c > >>@@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > >> aml_append(parent_scope, method); > >> } > >> > >>+static Aml *build_prt(void) > >>+{ > >>+ Aml *method, *pkg, *if_ctx, *while_ctx; > >>+ > >>+ method = aml_method("_PRT", 0); > >>+ > >>+ aml_append(method, aml_store(aml_package(128), aml_local(0))); > >>+ aml_append(method, aml_store(aml_int(0), aml_local(1))); > >>+ while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); > >>+ { > >>+ aml_append(while_ctx, > >>+ aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); > >>+ aml_append(while_ctx, > >>+ aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), > >>+ aml_local(3))); > >>+ > >>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); > >>+ { > >>+ pkg = aml_package(4); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_name("LNKD")); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>+ } > >>+ aml_append(while_ctx, if_ctx); > >>+ > >>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); > >>+ { > >>+ pkg = aml_package(4); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_name("LNKA")); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>+ } > >>+ aml_append(while_ctx, if_ctx); > >>+ > >>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); > >>+ { > >>+ pkg = aml_package(4); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_name("LNKB")); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>+ } > >>+ aml_append(while_ctx, if_ctx); > >>+ > >>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); > >>+ { > >>+ pkg = aml_package(4); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_name("LNKC")); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>+ } > >>+ aml_append(while_ctx, if_ctx); > >>+ > >>+ aml_append(while_ctx, > >>+ aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), > >>+ aml_int(0xFFFF)), > >>+ aml_index(aml_local(4), aml_int(0)))); > >>+ aml_append(while_ctx, > >>+ aml_store(aml_and(aml_local(1), aml_int(3)), > >>+ aml_index(aml_local(4), aml_int(1)))); > >>+ aml_append(while_ctx, > >>+ aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); > >>+ aml_append(while_ctx, aml_increment(aml_local(1))); > >>+ } > >>+ aml_append(method, while_ctx); > >>+ aml_append(method, aml_return(aml_local(0))); > >>+ > >>+ return method; > >>+} > >>+ > > > >Pls improve readability of this code using comments, sub-functions and > >local variables. > It is the exact "copy" of the static aml code we had, witch by itself > wasn't so nice, so it is so much I can do. That one has *some* comments at least. But yes, we can do better. Duplication of code is also a problem. > However, I'll try to improve it. > > Thanks, > Marcel > > > > > > > >> static void > >> build_ssdt(GArray *table_data, GArray *linker, > >> AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, > >>@@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, > >> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); > >> aml_append(dev, > >> aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); > >>+ aml_append(dev, build_prt()); > >> aml_append(scope, dev); > >> aml_append(ssdt, scope); > >> } > >>-- > >>2.1.0
On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> some ideas for cleaning this up. there's more here btw. > --- > hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 78 insertions(+) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index e5709e8..f0401d2 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > aml_append(parent_scope, method); > } > > +static Aml *build_prt(void) > +{ > + Aml *method, *pkg, *if_ctx, *while_ctx; > + > + method = aml_method("_PRT", 0); > + > + aml_append(method, aml_store(aml_package(128), aml_local(0))); > + aml_append(method, aml_store(aml_int(0), aml_local(1))); > + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); > + { > + aml_append(while_ctx, > + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); > + aml_append(while_ctx, > + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), > + aml_local(3))); As an example, you can have Aml *i = aml_local(1); Aml *mask = aml_local(2); > + > + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); > + { > + pkg = aml_package(4); Make pkg local here? > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_name("LNKD")); > + aml_append(pkg, aml_int(0)); > + aml_append(if_ctx, aml_store(pkg, aml_local(4))); > + } > + aml_append(while_ctx, if_ctx); > + > + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); > + { > + pkg = aml_package(4); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_name("LNKA")); > + aml_append(pkg, aml_int(0)); > + aml_append(if_ctx, aml_store(pkg, aml_local(4))); > + } > + aml_append(while_ctx, if_ctx); > + > + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); > + { > + pkg = aml_package(4); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_name("LNKB")); > + aml_append(pkg, aml_int(0)); > + aml_append(if_ctx, aml_store(pkg, aml_local(4))); > + } > + aml_append(while_ctx, if_ctx); > + > + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); > + { > + pkg = aml_package(4); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_int(0)); > + aml_append(pkg, aml_name("LNKC")); > + aml_append(pkg, aml_int(0)); > + aml_append(if_ctx, aml_store(pkg, aml_local(4))); > + } > + aml_append(while_ctx, if_ctx); > + > + aml_append(while_ctx, > + aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), > + aml_int(0xFFFF)), > + aml_index(aml_local(4), aml_int(0)))); > + aml_append(while_ctx, > + aml_store(aml_and(aml_local(1), aml_int(3)), > + aml_index(aml_local(4), aml_int(1)))); > + aml_append(while_ctx, > + aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); > + aml_append(while_ctx, aml_increment(aml_local(1))); > + } > + aml_append(method, while_ctx); > + aml_append(method, aml_return(aml_local(0))); > + > + return method; > +} > + > static void > build_ssdt(GArray *table_data, GArray *linker, > AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, > @@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, > aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); > aml_append(dev, > aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); > + aml_append(dev, build_prt()); > aml_append(scope, dev); > aml_append(ssdt, scope); > } > -- > 2.1.0
On 03/08/2015 12:46 PM, Michael S. Tsirkin wrote: > On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > some ideas for cleaning this up. > there's more here btw. > >> --- >> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 78 insertions(+) >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index e5709e8..f0401d2 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, >> aml_append(parent_scope, method); >> } >> >> +static Aml *build_prt(void) >> +{ >> + Aml *method, *pkg, *if_ctx, *while_ctx; >> + >> + method = aml_method("_PRT", 0); >> + >> + aml_append(method, aml_store(aml_package(128), aml_local(0))); >> + aml_append(method, aml_store(aml_int(0), aml_local(1))); >> + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); >> + { >> + aml_append(while_ctx, >> + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); >> + aml_append(while_ctx, >> + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), >> + aml_local(3))); > > > As an example, you can have > > Aml *i = aml_local(1); > Aml *mask = aml_local(2); Hi Michael, Thank you for the tip, the implementation is ready and I'll submit it right away. BTW, I found a bug in aml_equal implementation during re-testing, so it was worth it! Thanks, Marcel > > >> + >> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); >> + { >> + pkg = aml_package(4); > > Make pkg local here? > >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_name("LNKD")); >> + aml_append(pkg, aml_int(0)); >> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >> + } >> + aml_append(while_ctx, if_ctx); >> + >> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); >> + { >> + pkg = aml_package(4); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_name("LNKA")); >> + aml_append(pkg, aml_int(0)); >> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >> + } >> + aml_append(while_ctx, if_ctx); >> + >> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); >> + { >> + pkg = aml_package(4); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_name("LNKB")); >> + aml_append(pkg, aml_int(0)); >> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >> + } >> + aml_append(while_ctx, if_ctx); >> + >> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); >> + { >> + pkg = aml_package(4); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_int(0)); >> + aml_append(pkg, aml_name("LNKC")); >> + aml_append(pkg, aml_int(0)); >> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >> + } >> + aml_append(while_ctx, if_ctx); >> + >> + aml_append(while_ctx, >> + aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), >> + aml_int(0xFFFF)), >> + aml_index(aml_local(4), aml_int(0)))); >> + aml_append(while_ctx, >> + aml_store(aml_and(aml_local(1), aml_int(3)), >> + aml_index(aml_local(4), aml_int(1)))); >> + aml_append(while_ctx, >> + aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); >> + aml_append(while_ctx, aml_increment(aml_local(1))); >> + } >> + aml_append(method, while_ctx); >> + aml_append(method, aml_return(aml_local(0))); >> + >> + return method; >> +} >> + >> static void >> build_ssdt(GArray *table_data, GArray *linker, >> AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, >> @@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, >> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); >> aml_append(dev, >> aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); >> + aml_append(dev, build_prt()); >> aml_append(scope, dev); >> aml_append(ssdt, scope); >> } >> -- >> 2.1.0
On Sun, Mar 08, 2015 at 01:04:07PM +0200, Marcel Apfelbaum wrote: > On 03/08/2015 12:46 PM, Michael S. Tsirkin wrote: > >On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: > >>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > > > >some ideas for cleaning this up. > >there's more here btw. > > > >>--- > >> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 78 insertions(+) > >> > >>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > >>index e5709e8..f0401d2 100644 > >>--- a/hw/i386/acpi-build.c > >>+++ b/hw/i386/acpi-build.c > >>@@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > >> aml_append(parent_scope, method); > >> } > >> > >>+static Aml *build_prt(void) > >>+{ > >>+ Aml *method, *pkg, *if_ctx, *while_ctx; > >>+ > >>+ method = aml_method("_PRT", 0); > >>+ > >>+ aml_append(method, aml_store(aml_package(128), aml_local(0))); > >>+ aml_append(method, aml_store(aml_int(0), aml_local(1))); > >>+ while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); > >>+ { > >>+ aml_append(while_ctx, > >>+ aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); > >>+ aml_append(while_ctx, > >>+ aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), > >>+ aml_local(3))); > > > > > >As an example, you can have > > > >Aml *i = aml_local(1); > >Aml *mask = aml_local(2); > Hi Michael, > > Thank you for the tip, the implementation is ready > and I'll submit it right away. So, one thing that's problematic here is that expected files need to be examined manually. How about a three-stage approach: 1. move _PRT out from DSDT - e.g. to a separate ssdt, update expected files. One way to do this is to first revert commit 4ec8d2b3f54dd1dcd9e2a80e529feff4e2603288 Author: Igor Mammedov <imammedo@redhat.com> pc: acpi-build: drop remaining ssdt_misc template 2. rewrite the new SSDT in C, produce an otherwise identical code. make check will catch errors 3. reuse code from (2) for extra roots. > BTW, I found a bug in aml_equal implementation during re-testing, > so it was worth it! > > Thanks, > Marcel > > > > > > >>+ > >>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); > >>+ { > >>+ pkg = aml_package(4); > > > >Make pkg local here? > > > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_name("LNKD")); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>+ } > >>+ aml_append(while_ctx, if_ctx); > >>+ > >>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); > >>+ { > >>+ pkg = aml_package(4); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_name("LNKA")); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>+ } > >>+ aml_append(while_ctx, if_ctx); > >>+ > >>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); > >>+ { > >>+ pkg = aml_package(4); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_name("LNKB")); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>+ } > >>+ aml_append(while_ctx, if_ctx); > >>+ > >>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); > >>+ { > >>+ pkg = aml_package(4); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(pkg, aml_name("LNKC")); > >>+ aml_append(pkg, aml_int(0)); > >>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>+ } > >>+ aml_append(while_ctx, if_ctx); > >>+ > >>+ aml_append(while_ctx, > >>+ aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), > >>+ aml_int(0xFFFF)), > >>+ aml_index(aml_local(4), aml_int(0)))); > >>+ aml_append(while_ctx, > >>+ aml_store(aml_and(aml_local(1), aml_int(3)), > >>+ aml_index(aml_local(4), aml_int(1)))); > >>+ aml_append(while_ctx, > >>+ aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); > >>+ aml_append(while_ctx, aml_increment(aml_local(1))); > >>+ } > >>+ aml_append(method, while_ctx); > >>+ aml_append(method, aml_return(aml_local(0))); > >>+ > >>+ return method; > >>+} > >>+ > >> static void > >> build_ssdt(GArray *table_data, GArray *linker, > >> AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, > >>@@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, > >> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); > >> aml_append(dev, > >> aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); > >>+ aml_append(dev, build_prt()); > >> aml_append(scope, dev); > >> aml_append(ssdt, scope); > >> } > >>-- > >>2.1.0
On 03/08/2015 04:47 PM, Michael S. Tsirkin wrote: > On Sun, Mar 08, 2015 at 01:04:07PM +0200, Marcel Apfelbaum wrote: >> On 03/08/2015 12:46 PM, Michael S. Tsirkin wrote: >>> On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: >>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>> >>> some ideas for cleaning this up. >>> there's more here btw. >>> >>>> --- >>>> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 78 insertions(+) >>>> >>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>>> index e5709e8..f0401d2 100644 >>>> --- a/hw/i386/acpi-build.c >>>> +++ b/hw/i386/acpi-build.c >>>> @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, >>>> aml_append(parent_scope, method); >>>> } >>>> >>>> +static Aml *build_prt(void) >>>> +{ >>>> + Aml *method, *pkg, *if_ctx, *while_ctx; >>>> + >>>> + method = aml_method("_PRT", 0); >>>> + >>>> + aml_append(method, aml_store(aml_package(128), aml_local(0))); >>>> + aml_append(method, aml_store(aml_int(0), aml_local(1))); >>>> + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); >>>> + { >>>> + aml_append(while_ctx, >>>> + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); >>>> + aml_append(while_ctx, >>>> + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), >>>> + aml_local(3))); >>> >>> >>> As an example, you can have >>> >>> Aml *i = aml_local(1); >>> Aml *mask = aml_local(2); >> Hi Michael, >> >> Thank you for the tip, the implementation is ready >> and I'll submit it right away. > > So, one thing that's problematic here is that > expected files need to be examined manually. Only if pxb-device is added, otherwise they remain the same. So by default, make check remains unchanged. > How about a three-stage approach: > > 1. move _PRT out from DSDT - e.g. to a separate ssdt, > update expected files. > One way to do this is to first revert > commit 4ec8d2b3f54dd1dcd9e2a80e529feff4e2603288 > Author: Igor Mammedov <imammedo@redhat.com> > pc: acpi-build: drop remaining ssdt_misc template > 2. rewrite the new SSDT in C, > produce an otherwise identical code. > make check will catch errors > 3. reuse code from (2) for extra roots. I am aware of this and I was planning to do that on top of this series. Four reasons for this: 1. As stated before, it does not affect make check because this code affects the ACPI table only if pxb-device is present. 2. The _PRT is not *exactly* the same, bus 0's _PRT has an extra "if" for the power-management device than needs needs SCI 3. The series is already big, I prefer attacking this as a new enhancement: "Dynamically create bus 0 _PRT" 4. QEMU 2.3 is approaching, I don't want o diverge now Thanks, Marcel > > >> BTW, I found a bug in aml_equal implementation during re-testing, >> so it was worth it! >> >> Thanks, >> Marcel >> >>> >>> >>>> + >>>> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); >>>> + { >>>> + pkg = aml_package(4); >>> >>> Make pkg local here? >>> >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(pkg, aml_name("LNKD")); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >>>> + } >>>> + aml_append(while_ctx, if_ctx); >>>> + >>>> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); >>>> + { >>>> + pkg = aml_package(4); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(pkg, aml_name("LNKA")); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >>>> + } >>>> + aml_append(while_ctx, if_ctx); >>>> + >>>> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); >>>> + { >>>> + pkg = aml_package(4); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(pkg, aml_name("LNKB")); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >>>> + } >>>> + aml_append(while_ctx, if_ctx); >>>> + >>>> + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); >>>> + { >>>> + pkg = aml_package(4); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(pkg, aml_name("LNKC")); >>>> + aml_append(pkg, aml_int(0)); >>>> + aml_append(if_ctx, aml_store(pkg, aml_local(4))); >>>> + } >>>> + aml_append(while_ctx, if_ctx); >>>> + >>>> + aml_append(while_ctx, >>>> + aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), >>>> + aml_int(0xFFFF)), >>>> + aml_index(aml_local(4), aml_int(0)))); >>>> + aml_append(while_ctx, >>>> + aml_store(aml_and(aml_local(1), aml_int(3)), >>>> + aml_index(aml_local(4), aml_int(1)))); >>>> + aml_append(while_ctx, >>>> + aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); >>>> + aml_append(while_ctx, aml_increment(aml_local(1))); >>>> + } >>>> + aml_append(method, while_ctx); >>>> + aml_append(method, aml_return(aml_local(0))); >>>> + >>>> + return method; >>>> +} >>>> + >>>> static void >>>> build_ssdt(GArray *table_data, GArray *linker, >>>> AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, >>>> @@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, >>>> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); >>>> aml_append(dev, >>>> aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); >>>> + aml_append(dev, build_prt()); >>>> aml_append(scope, dev); >>>> aml_append(ssdt, scope); >>>> } >>>> -- >>>> 2.1.0
On Sun, Mar 08, 2015 at 05:22:43PM +0200, Marcel Apfelbaum wrote: > On 03/08/2015 04:47 PM, Michael S. Tsirkin wrote: > >On Sun, Mar 08, 2015 at 01:04:07PM +0200, Marcel Apfelbaum wrote: > >>On 03/08/2015 12:46 PM, Michael S. Tsirkin wrote: > >>>On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: > >>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > >>> > >>>some ideas for cleaning this up. > >>>there's more here btw. > >>> > >>>>--- > >>>> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 78 insertions(+) > >>>> > >>>>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > >>>>index e5709e8..f0401d2 100644 > >>>>--- a/hw/i386/acpi-build.c > >>>>+++ b/hw/i386/acpi-build.c > >>>>@@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > >>>> aml_append(parent_scope, method); > >>>> } > >>>> > >>>>+static Aml *build_prt(void) > >>>>+{ > >>>>+ Aml *method, *pkg, *if_ctx, *while_ctx; > >>>>+ > >>>>+ method = aml_method("_PRT", 0); > >>>>+ > >>>>+ aml_append(method, aml_store(aml_package(128), aml_local(0))); > >>>>+ aml_append(method, aml_store(aml_int(0), aml_local(1))); > >>>>+ while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); > >>>>+ { > >>>>+ aml_append(while_ctx, > >>>>+ aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); > >>>>+ aml_append(while_ctx, > >>>>+ aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), > >>>>+ aml_local(3))); > >>> > >>> > >>>As an example, you can have > >>> > >>>Aml *i = aml_local(1); > >>>Aml *mask = aml_local(2); > >>Hi Michael, > >> > >>Thank you for the tip, the implementation is ready > >>and I'll submit it right away. > > > >So, one thing that's problematic here is that > >expected files need to be examined manually. > Only if pxb-device is added, otherwise they remain the same. > So by default, make check remains unchanged. > >How about a three-stage approach: > > > >1. move _PRT out from DSDT - e.g. to a separate ssdt, > > update expected files. > > One way to do this is to first revert > > commit 4ec8d2b3f54dd1dcd9e2a80e529feff4e2603288 > > Author: Igor Mammedov <imammedo@redhat.com> > > pc: acpi-build: drop remaining ssdt_misc template > >2. rewrite the new SSDT in C, > > produce an otherwise identical code. > > make check will catch errors > >3. reuse code from (2) for extra roots. > > I am aware of this and I was planning to do that on top of this series. > Four reasons for this: > 1. As stated before, it does not affect make check because this code > affects the ACPI table only if pxb-device is present. > 2. The _PRT is not *exactly* the same, bus 0's _PRT has an extra "if" for > the power-management device than needs needs SCI > 3. The series is already big, I prefer attacking this as a new enhancement: > "Dynamically create bus 0 _PRT" > 4. QEMU 2.3 is approaching, I don't want o diverge now > > > Thanks, > Marcel Meanwhile 2.3 will have two almost identical copies of _PRT? I don't think that's a good idea. > > > > > >>BTW, I found a bug in aml_equal implementation during re-testing, > >>so it was worth it! > >> > >>Thanks, > >>Marcel > >> > >>> > >>> > >>>>+ > >>>>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); > >>>>+ { > >>>>+ pkg = aml_package(4); > >>> > >>>Make pkg local here? > >>> > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(pkg, aml_name("LNKD")); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>>>+ } > >>>>+ aml_append(while_ctx, if_ctx); > >>>>+ > >>>>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); > >>>>+ { > >>>>+ pkg = aml_package(4); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(pkg, aml_name("LNKA")); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>>>+ } > >>>>+ aml_append(while_ctx, if_ctx); > >>>>+ > >>>>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); > >>>>+ { > >>>>+ pkg = aml_package(4); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(pkg, aml_name("LNKB")); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>>>+ } > >>>>+ aml_append(while_ctx, if_ctx); > >>>>+ > >>>>+ if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); > >>>>+ { > >>>>+ pkg = aml_package(4); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(pkg, aml_name("LNKC")); > >>>>+ aml_append(pkg, aml_int(0)); > >>>>+ aml_append(if_ctx, aml_store(pkg, aml_local(4))); > >>>>+ } > >>>>+ aml_append(while_ctx, if_ctx); > >>>>+ > >>>>+ aml_append(while_ctx, > >>>>+ aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), > >>>>+ aml_int(0xFFFF)), > >>>>+ aml_index(aml_local(4), aml_int(0)))); > >>>>+ aml_append(while_ctx, > >>>>+ aml_store(aml_and(aml_local(1), aml_int(3)), > >>>>+ aml_index(aml_local(4), aml_int(1)))); > >>>>+ aml_append(while_ctx, > >>>>+ aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); > >>>>+ aml_append(while_ctx, aml_increment(aml_local(1))); > >>>>+ } > >>>>+ aml_append(method, while_ctx); > >>>>+ aml_append(method, aml_return(aml_local(0))); > >>>>+ > >>>>+ return method; > >>>>+} > >>>>+ > >>>> static void > >>>> build_ssdt(GArray *table_data, GArray *linker, > >>>> AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, > >>>>@@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, > >>>> aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); > >>>> aml_append(dev, > >>>> aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); > >>>>+ aml_append(dev, build_prt()); > >>>> aml_append(scope, dev); > >>>> aml_append(ssdt, scope); > >>>> } > >>>>-- > >>>>2.1.0
On 03/08/2015 05:27 PM, Michael S. Tsirkin wrote: > On Sun, Mar 08, 2015 at 05:22:43PM +0200, Marcel Apfelbaum wrote: >> On 03/08/2015 04:47 PM, Michael S. Tsirkin wrote: >>> On Sun, Mar 08, 2015 at 01:04:07PM +0200, Marcel Apfelbaum wrote: >>>> On 03/08/2015 12:46 PM, Michael S. Tsirkin wrote: >>>>> On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: >>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>>>> >>>>> some ideas for cleaning this up. >>>>> there's more here btw. >>>>> >>>>>> --- >>>>>> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 78 insertions(+) >>>>>> >>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>>>>> index e5709e8..f0401d2 100644 >>>>>> --- a/hw/i386/acpi-build.c >>>>>> +++ b/hw/i386/acpi-build.c >>>>>> @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, >>>>>> aml_append(parent_scope, method); >>>>>> } >>>>>> >>>>>> +static Aml *build_prt(void) >>>>>> +{ >>>>>> + Aml *method, *pkg, *if_ctx, *while_ctx; >>>>>> + >>>>>> + method = aml_method("_PRT", 0); >>>>>> + >>>>>> + aml_append(method, aml_store(aml_package(128), aml_local(0))); >>>>>> + aml_append(method, aml_store(aml_int(0), aml_local(1))); >>>>>> + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); >>>>>> + { >>>>>> + aml_append(while_ctx, >>>>>> + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); >>>>>> + aml_append(while_ctx, >>>>>> + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), >>>>>> + aml_local(3))); >>>>> >>>>> >>>>> As an example, you can have >>>>> >>>>> Aml *i = aml_local(1); >>>>> Aml *mask = aml_local(2); >>>> Hi Michael, >>>> >>>> Thank you for the tip, the implementation is ready >>>> and I'll submit it right away. >>> >>> So, one thing that's problematic here is that >>> expected files need to be examined manually. >> Only if pxb-device is added, otherwise they remain the same. >> So by default, make check remains unchanged. >>> How about a three-stage approach: >>> >>> 1. move _PRT out from DSDT - e.g. to a separate ssdt, >>> update expected files. >>> One way to do this is to first revert >>> commit 4ec8d2b3f54dd1dcd9e2a80e529feff4e2603288 >>> Author: Igor Mammedov <imammedo@redhat.com> >>> pc: acpi-build: drop remaining ssdt_misc template >>> 2. rewrite the new SSDT in C, >>> produce an otherwise identical code. >>> make check will catch errors >>> 3. reuse code from (2) for extra roots. >> >> I am aware of this and I was planning to do that on top of this series. >> Four reasons for this: >> 1. As stated before, it does not affect make check because this code >> affects the ACPI table only if pxb-device is present. >> 2. The _PRT is not *exactly* the same, bus 0's _PRT has an extra "if" for >> the power-management device than needs needs SCI >> 3. The series is already big, I prefer attacking this as a new enhancement: >> "Dynamically create bus 0 _PRT" >> 4. QEMU 2.3 is approaching, I don't want o diverge now >> >> >> Thanks, >> Marcel > > Meanwhile 2.3 will have two almost identical copies of _PRT? > I don't think that's a good idea. The are 2 *different* _PRTs for 2 different hw components. The look the same only because I *preferred* them to look almost the same, I could implement it differently. There is actually nothing common about 1. PIIX host bridge _PRT implemented statically 2. pcb-device _PRT implemented in code. What is true is the series makes easier to implement dynamic _PRT for any device, including PIIX host bridge. It doesn't mean we have two. Actually we need to think about using the same code, because changing it would influence both devices. I really don't think they are related. Thanks, Marcel > >>> [...]
On Sun, Mar 08, 2015 at 05:37:25PM +0200, Marcel Apfelbaum wrote: > On 03/08/2015 05:27 PM, Michael S. Tsirkin wrote: > >On Sun, Mar 08, 2015 at 05:22:43PM +0200, Marcel Apfelbaum wrote: > >>On 03/08/2015 04:47 PM, Michael S. Tsirkin wrote: > >>>On Sun, Mar 08, 2015 at 01:04:07PM +0200, Marcel Apfelbaum wrote: > >>>>On 03/08/2015 12:46 PM, Michael S. Tsirkin wrote: > >>>>>On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: > >>>>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> > >>>>> > >>>>>some ideas for cleaning this up. > >>>>>there's more here btw. > >>>>> > >>>>>>--- > >>>>>> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > >>>>>> 1 file changed, 78 insertions(+) > >>>>>> > >>>>>>diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > >>>>>>index e5709e8..f0401d2 100644 > >>>>>>--- a/hw/i386/acpi-build.c > >>>>>>+++ b/hw/i386/acpi-build.c > >>>>>>@@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, > >>>>>> aml_append(parent_scope, method); > >>>>>> } > >>>>>> > >>>>>>+static Aml *build_prt(void) > >>>>>>+{ > >>>>>>+ Aml *method, *pkg, *if_ctx, *while_ctx; > >>>>>>+ > >>>>>>+ method = aml_method("_PRT", 0); > >>>>>>+ > >>>>>>+ aml_append(method, aml_store(aml_package(128), aml_local(0))); > >>>>>>+ aml_append(method, aml_store(aml_int(0), aml_local(1))); > >>>>>>+ while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); > >>>>>>+ { > >>>>>>+ aml_append(while_ctx, > >>>>>>+ aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); > >>>>>>+ aml_append(while_ctx, > >>>>>>+ aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), > >>>>>>+ aml_local(3))); > >>>>> > >>>>> > >>>>>As an example, you can have > >>>>> > >>>>>Aml *i = aml_local(1); > >>>>>Aml *mask = aml_local(2); > >>>>Hi Michael, > >>>> > >>>>Thank you for the tip, the implementation is ready > >>>>and I'll submit it right away. > >>> > >>>So, one thing that's problematic here is that > >>>expected files need to be examined manually. > >>Only if pxb-device is added, otherwise they remain the same. > >>So by default, make check remains unchanged. > >>>How about a three-stage approach: > >>> > >>>1. move _PRT out from DSDT - e.g. to a separate ssdt, > >>> update expected files. > >>> One way to do this is to first revert > >>> commit 4ec8d2b3f54dd1dcd9e2a80e529feff4e2603288 > >>> Author: Igor Mammedov <imammedo@redhat.com> > >>> pc: acpi-build: drop remaining ssdt_misc template > >>>2. rewrite the new SSDT in C, > >>> produce an otherwise identical code. > >>> make check will catch errors > >>>3. reuse code from (2) for extra roots. > >> > >>I am aware of this and I was planning to do that on top of this series. > >>Four reasons for this: > >> 1. As stated before, it does not affect make check because this code > >> affects the ACPI table only if pxb-device is present. > >> 2. The _PRT is not *exactly* the same, bus 0's _PRT has an extra "if" for > >> the power-management device than needs needs SCI > >> 3. The series is already big, I prefer attacking this as a new enhancement: > >> "Dynamically create bus 0 _PRT" > >> 4. QEMU 2.3 is approaching, I don't want o diverge now > >> > >> > >>Thanks, > >>Marcel > > > >Meanwhile 2.3 will have two almost identical copies of _PRT? > >I don't think that's a good idea. > The are 2 *different* _PRTs for 2 different hw components. > The look the same only because I *preferred* them to look almost the same, > I could implement it differently. > There is actually nothing common about > 1. PIIX host bridge _PRT implemented statically > 2. pcb-device _PRT implemented in code. > > What is true is the series makes easier to implement dynamic _PRT > for any device, including PIIX host bridge. It doesn't mean we have two. > > Actually we need to think about using the same code, because changing it > would influence both devices. > > I really don't think they are related. > Thanks, > Marcel Come on, the same argument would make us duplicate almost any block of code. Copy-pasting always seems safer and faster than reusing, until you have to fix a bug in multiple places. > > > > >>> > [...]
On 03/08/2015 06:03 PM, Michael S. Tsirkin wrote: > On Sun, Mar 08, 2015 at 05:37:25PM +0200, Marcel Apfelbaum wrote: >> On 03/08/2015 05:27 PM, Michael S. Tsirkin wrote: >>> On Sun, Mar 08, 2015 at 05:22:43PM +0200, Marcel Apfelbaum wrote: >>>> On 03/08/2015 04:47 PM, Michael S. Tsirkin wrote: >>>>> On Sun, Mar 08, 2015 at 01:04:07PM +0200, Marcel Apfelbaum wrote: >>>>>> On 03/08/2015 12:46 PM, Michael S. Tsirkin wrote: >>>>>>> On Thu, Mar 05, 2015 at 04:55:08PM +0200, Marcel Apfelbaum wrote: >>>>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> >>>>>>> >>>>>>> some ideas for cleaning this up. >>>>>>> there's more here btw. >>>>>>> >>>>>>>> --- >>>>>>>> hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 1 file changed, 78 insertions(+) >>>>>>>> >>>>>>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>>>>>>> index e5709e8..f0401d2 100644 >>>>>>>> --- a/hw/i386/acpi-build.c >>>>>>>> +++ b/hw/i386/acpi-build.c >>>>>>>> @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, >>>>>>>> aml_append(parent_scope, method); >>>>>>>> } >>>>>>>> >>>>>>>> +static Aml *build_prt(void) >>>>>>>> +{ >>>>>>>> + Aml *method, *pkg, *if_ctx, *while_ctx; >>>>>>>> + >>>>>>>> + method = aml_method("_PRT", 0); >>>>>>>> + >>>>>>>> + aml_append(method, aml_store(aml_package(128), aml_local(0))); >>>>>>>> + aml_append(method, aml_store(aml_int(0), aml_local(1))); >>>>>>>> + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); >>>>>>>> + { >>>>>>>> + aml_append(while_ctx, >>>>>>>> + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); >>>>>>>> + aml_append(while_ctx, >>>>>>>> + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), >>>>>>>> + aml_local(3))); >>>>>>> >>>>>>> >>>>>>> As an example, you can have >>>>>>> >>>>>>> Aml *i = aml_local(1); >>>>>>> Aml *mask = aml_local(2); >>>>>> Hi Michael, >>>>>> >>>>>> Thank you for the tip, the implementation is ready >>>>>> and I'll submit it right away. >>>>> >>>>> So, one thing that's problematic here is that >>>>> expected files need to be examined manually. >>>> Only if pxb-device is added, otherwise they remain the same. >>>> So by default, make check remains unchanged. >>>>> How about a three-stage approach: >>>>> >>>>> 1. move _PRT out from DSDT - e.g. to a separate ssdt, >>>>> update expected files. >>>>> One way to do this is to first revert >>>>> commit 4ec8d2b3f54dd1dcd9e2a80e529feff4e2603288 >>>>> Author: Igor Mammedov <imammedo@redhat.com> >>>>> pc: acpi-build: drop remaining ssdt_misc template >>>>> 2. rewrite the new SSDT in C, >>>>> produce an otherwise identical code. >>>>> make check will catch errors >>>>> 3. reuse code from (2) for extra roots. >>>> >>>> I am aware of this and I was planning to do that on top of this series. >>>> Four reasons for this: >>>> 1. As stated before, it does not affect make check because this code >>>> affects the ACPI table only if pxb-device is present. >>>> 2. The _PRT is not *exactly* the same, bus 0's _PRT has an extra "if" for >>>> the power-management device than needs needs SCI >>>> 3. The series is already big, I prefer attacking this as a new enhancement: >>>> "Dynamically create bus 0 _PRT" >>>> 4. QEMU 2.3 is approaching, I don't want o diverge now >>>> >>>> >>>> Thanks, >>>> Marcel >>> >>> Meanwhile 2.3 will have two almost identical copies of _PRT? >>> I don't think that's a good idea. >> The are 2 *different* _PRTs for 2 different hw components. >> The look the same only because I *preferred* them to look almost the same, >> I could implement it differently. >> There is actually nothing common about >> 1. PIIX host bridge _PRT implemented statically >> 2. pcb-device _PRT implemented in code. >> >> What is true is the series makes easier to implement dynamic _PRT >> for any device, including PIIX host bridge. It doesn't mean we have two. >> >> Actually we need to think about using the same code, because changing it >> would influence both devices. >> >> I really don't think they are related. >> Thanks, >> Marcel > > > Come on, the same argument would make us duplicate almost > any block of code. Copy-pasting always seems safer and faster > than reusing, until you have to fix a bug in multiple places. Completely agree, but this is not the case. Here we have symmetric ways to solve this: 1. As I do: _PRT for PXB, then reuse it for piix host-bridge PXB code. 2. First the _PRT of piix host-bridge, then reuse it for PXB _PRT And we still make the series longer and more complex. Thanks, Marcel > >> >>> >>>>> >> [...]
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index e5709e8..f0401d2 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -664,6 +664,83 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus, aml_append(parent_scope, method); } +static Aml *build_prt(void) +{ + Aml *method, *pkg, *if_ctx, *while_ctx; + + method = aml_method("_PRT", 0); + + aml_append(method, aml_store(aml_package(128), aml_local(0))); + aml_append(method, aml_store(aml_int(0), aml_local(1))); + while_ctx = aml_while(aml_lless(aml_local(1), aml_int(128))); + { + aml_append(while_ctx, + aml_store(aml_shiftright(aml_local(1), aml_int(2)), aml_local(2))); + aml_append(while_ctx, + aml_store(aml_and(aml_add(aml_local(1), aml_local(2)), aml_int(3)), + aml_local(3))); + + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(0))); + { + pkg = aml_package(4); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_name("LNKD")); + aml_append(pkg, aml_int(0)); + aml_append(if_ctx, aml_store(pkg, aml_local(4))); + } + aml_append(while_ctx, if_ctx); + + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(1))); + { + pkg = aml_package(4); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_name("LNKA")); + aml_append(pkg, aml_int(0)); + aml_append(if_ctx, aml_store(pkg, aml_local(4))); + } + aml_append(while_ctx, if_ctx); + + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(2))); + { + pkg = aml_package(4); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_name("LNKB")); + aml_append(pkg, aml_int(0)); + aml_append(if_ctx, aml_store(pkg, aml_local(4))); + } + aml_append(while_ctx, if_ctx); + + if_ctx = aml_if(aml_equal(aml_local(3), aml_int(3))); + { + pkg = aml_package(4); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_int(0)); + aml_append(pkg, aml_name("LNKC")); + aml_append(pkg, aml_int(0)); + aml_append(if_ctx, aml_store(pkg, aml_local(4))); + } + aml_append(while_ctx, if_ctx); + + aml_append(while_ctx, + aml_store(aml_or(aml_shiftleft(aml_local(2), aml_int(16)), + aml_int(0xFFFF)), + aml_index(aml_local(4), aml_int(0)))); + aml_append(while_ctx, + aml_store(aml_and(aml_local(1), aml_int(3)), + aml_index(aml_local(4), aml_int(1)))); + aml_append(while_ctx, + aml_store(aml_local(4), aml_index(aml_local(0), aml_local(1)))); + aml_append(while_ctx, aml_increment(aml_local(1))); + } + aml_append(method, while_ctx); + aml_append(method, aml_return(aml_local(0))); + + return method; +} + static void build_ssdt(GArray *table_data, GArray *linker, AcpiCpuInfo *cpu, AcpiPmInfo *pm, AcpiMiscInfo *misc, @@ -708,6 +785,7 @@ build_ssdt(GArray *table_data, GArray *linker, aml_append(dev, aml_name_decl("_HID", aml_string("PNP0A03"))); aml_append(dev, aml_name_decl("_BBN", aml_int((uint8_t)bus_info->bus))); + aml_append(dev, build_prt()); aml_append(scope, dev); aml_append(ssdt, scope); }
Signed-off-by: Marcel Apfelbaum <marcel@redhat.com> --- hw/i386/acpi-build.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+)