Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation
On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote: > On Wed, 25 Jul 2018 15:44:37 +0300 > "Michael S. Tsirkin" wrote: > > > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote: > > > On Tue, 10 Jul 2018 03:01:30 +0300 > > > "Michael S. Tsirkin" wrote: > > > > > > > Now that basic support for guest CPU PM is upstream, I started looking > > > > for making it migrateable. Since a VM can be migrated between different > > > > hosts, PM info needs to change each time with move the VM to a different > > > > host. > > > Considering notification is async, so there will be a window when > > > guest will be using old Cstates on new host. What will happen if > > > machine is migrated to host that doesn't support a Cstate > > > that was used on source when guest was started? > > > > My testing shows mwait with a wrong hint works, presumably it just uses > > a lot of power. > > > > > > This adds infrastructure - based on Load/Unload - to support exactly > > > > that: QEMU generates AML (changing it on migration) and stores it in > > > > reserved memory, OSPM loads _CST from there on demand. > > > Cool and very tempting idea but I have 2 worries about this approach: > > > 1. How well does it work with Windows based guests? > > >(I've tried something similar but generating new AML from AML itself > > > to get rid of our long if/else chains there to make up Notify target > > > name. > > > I ditched it (unfortunately I don't recall why) ) > > > > After trying it, I can tell you why - it's a horrid mess of > > unreadable code, with arbitrary restraints on package > > length etc. > in my case it was only 4 character NameString, but Windows probably > wasn't happy or just ignored it. > Considering recent development (TPM series) it might have been issue > with SYSTEM_MEMORY not working properly if used as byte buffer field. > > Even if it's an unreadable mess it's still stable mess and very constrained > one within a single firmware blob that came from source. That's exectly the argument we had for keeping ACPI generation in bios. It's just not an interface that scales. > Hence it's more preferable than split brain in this series. > > But I don't think we even need dynamic AML for _CST usecase at all, > existing cpuhp interface should work just fine for it and should be > simpler as all infrastructure is already there. Not sure I get what you mean. Could you post a patch? > > > 2. (probably biggest issue) Loading dynamically generated AML > > >basically would make all AML code ABI, so that static AML > > >in RAM of old QEMU version would match dynamic generated > > >one on target side with new QEMU (/me generalizing approach beyond > > > _CST support). > > >I'd try to keep our AML version less as much as possible > > >and go this route only if there is no other way to do it. > > > > That's a good point, thanks for bringing this up! > > > > So it seems that we will need to define the ABI, yes. All AML code is > > an over-statement though, there are specific entry points > > we must maintain, right? > Well, I'm rather unsure what and where could break, > hence I'm afraid of a new beast and it probably won't be easy > to convince me that ABI would be able to keep things manageable > and stable. > Considering big amount of AML code that we already have > I'm not confident eye balling during review and testing the latest > firmware/qemu would be sufficient as we suddenly would have exploded > test matrix where firmware in use is a mixed from old/and new parts. Well there's one exported method so far and it relies on one container device in static table set which does not sound too hard to keep stable. Given how simple the dynamic table is, how about just checking it with a unit test? It is literally a single return statement. If it returns a valid package, that is all that we care about. I can write some firmware to test that constraint. > > And that in the end isn't fundamentally different from the ABIs > > mandated by the ACPI spec. > > > > So I have these ideas to try to mitigate the issues: > > - document the ABI (like we have in the ACPI spec) > > - use a specific prefix for all external calls (like _ for ACPI spec ones) > > - use a specific (different) prefix for all internal loaded calls (like > > [A-Z] for non-ACPI spec ones) > ACPI spec horrible mess and bad example (even committee isn't able to keep it > consistent) > but it's necessary evil (compared to no spec at all). > Adding ABI requirement will complicate coding/reviewing for > already complex AML compared to current non ABI way. > Hence from maintainability POV we should stay away from this approach > if there is a viable alternative. I agree. Not that I see one. > > > > Thoughts? > > > > > > This is a partially functional prototype. > > > > > > > > What works: > > > > loading _CST dynamically and reporting it to OSPM > > > > > > > > What doesn't: > > > > detec
Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation
On Thu, 26 Jul 2018 19:09:22 +0300 "Michael S. Tsirkin" wrote: > On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote: > > On Wed, 25 Jul 2018 15:44:37 +0300 > > "Michael S. Tsirkin" wrote: > > > > > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote: > > > > On Tue, 10 Jul 2018 03:01:30 +0300 > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > Now that basic support for guest CPU PM is upstream, I started looking > > > > > for making it migrateable. Since a VM can be migrated between > > > > > different > > > > > hosts, PM info needs to change each time with move the VM to a > > > > > different > > > > > host. > > > > Considering notification is async, so there will be a window when > > > > guest will be using old Cstates on new host. What will happen if > > > > machine is migrated to host that doesn't support a Cstate > > > > that was used on source when guest was started? > > > > > > My testing shows mwait with a wrong hint works, presumably it just uses > > > a lot of power. > > > > > > > > This adds infrastructure - based on Load/Unload - to support exactly > > > > > that: QEMU generates AML (changing it on migration) and stores it in > > > > > reserved memory, OSPM loads _CST from there on demand. > > > > Cool and very tempting idea but I have 2 worries about this approach: > > > > 1. How well does it work with Windows based guests? > > > >(I've tried something similar but generating new AML from AML itself > > > > to get rid of our long if/else chains there to make up Notify > > > > target name. > > > > I ditched it (unfortunately I don't recall why) ) > > > > > > After trying it, I can tell you why - it's a horrid mess of > > > unreadable code, with arbitrary restraints on package > > > length etc. > > in my case it was only 4 character NameString, but Windows probably > > wasn't happy or just ignored it. > > Considering recent development (TPM series) it might have been issue > > with SYSTEM_MEMORY not working properly if used as byte buffer field. > > > > Even if it's an unreadable mess it's still stable mess and very constrained > > one within a single firmware blob that came from source. > > That's exectly the argument we had for keeping ACPI > generation in bios. It's just not an interface that scales. > > > Hence it's more preferable than split brain in this series. > > > > But I don't think we even need dynamic AML for _CST usecase at all, > > existing cpuhp interface should work just fine for it and should be > > simpler as all infrastructure is already there. > > Not sure I get what you mean. Could you post a patch? > > > > > 2. (probably biggest issue) Loading dynamically generated AML > > > >basically would make all AML code ABI, so that static AML > > > >in RAM of old QEMU version would match dynamic generated > > > >one on target side with new QEMU (/me generalizing approach beyond > > > > _CST support). > > > >I'd try to keep our AML version less as much as possible > > > >and go this route only if there is no other way to do it. > > > > > > That's a good point, thanks for bringing this up! > > > > > > So it seems that we will need to define the ABI, yes. All AML code is > > > an over-statement though, there are specific entry points > > > we must maintain, right? > > Well, I'm rather unsure what and where could break, > > hence I'm afraid of a new beast and it probably won't be easy > > to convince me that ABI would be able to keep things manageable > > and stable. > > Considering big amount of AML code that we already have > > I'm not confident eye balling during review and testing the latest > > firmware/qemu would be sufficient as we suddenly would have exploded > > test matrix where firmware in use is a mixed from old/and new parts. > > Well there's one exported method so far and it relies on one container > device in static table set which does not sound too hard to keep stable. > > Given how simple the dynamic table is, how about just checking it > with a unit test? It is literally a single return statement. > If it returns a valid package, that is all that we > care about. I can write some firmware to test that constraint. > > > > > And that in the end isn't fundamentally different from the ABIs > > > mandated by the ACPI spec. > > > > > > So I have these ideas to try to mitigate the issues: > > > - document the ABI (like we have in the ACPI spec) > > > - use a specific prefix for all external calls (like _ for ACPI spec ones) > > > - use a specific (different) prefix for all internal loaded calls (like > > > [A-Z] for non-ACPI spec ones) > > ACPI spec horrible mess and bad example (even committee isn't able to keep > > it consistent) > > but it's necessary evil (compared to no spec at all). > > Adding ABI requirement will complicate coding/reviewing for > > already complex AML compared to current non ABI way. > > Hence from maintainability POV we shoul
Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation
On Thu, Aug 02, 2018 at 11:18:08AM +0200, Igor Mammedov wrote: > On Thu, 26 Jul 2018 19:09:22 +0300 > "Michael S. Tsirkin" wrote: > > > On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote: > > > On Wed, 25 Jul 2018 15:44:37 +0300 > > > "Michael S. Tsirkin" wrote: > > > > > > > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote: > > > > > On Tue, 10 Jul 2018 03:01:30 +0300 > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > Now that basic support for guest CPU PM is upstream, I started > > > > > > looking > > > > > > for making it migrateable. Since a VM can be migrated between > > > > > > different > > > > > > hosts, PM info needs to change each time with move the VM to a > > > > > > different > > > > > > host. > > > > > Considering notification is async, so there will be a window when > > > > > guest will be using old Cstates on new host. What will happen if > > > > > machine is migrated to host that doesn't support a Cstate > > > > > that was used on source when guest was started? > > > > > > > > My testing shows mwait with a wrong hint works, presumably it just uses > > > > a lot of power. > > > > > > > > > > This adds infrastructure - based on Load/Unload - to support exactly > > > > > > that: QEMU generates AML (changing it on migration) and stores it in > > > > > > reserved memory, OSPM loads _CST from there on demand. > > > > > Cool and very tempting idea but I have 2 worries about this approach: > > > > > 1. How well does it work with Windows based guests? > > > > >(I've tried something similar but generating new AML from AML > > > > > itself > > > > > to get rid of our long if/else chains there to make up Notify > > > > > target name. > > > > > I ditched it (unfortunately I don't recall why) ) > > > > > > > > After trying it, I can tell you why - it's a horrid mess of > > > > unreadable code, with arbitrary restraints on package > > > > length etc. > > > in my case it was only 4 character NameString, but Windows probably > > > wasn't happy or just ignored it. > > > Considering recent development (TPM series) it might have been issue > > > with SYSTEM_MEMORY not working properly if used as byte buffer field. > > > > > > Even if it's an unreadable mess it's still stable mess and very > > > constrained > > > one within a single firmware blob that came from source. > > > > That's exectly the argument we had for keeping ACPI > > generation in bios. It's just not an interface that scales. > > > > > Hence it's more preferable than split brain in this series. > > > > > > But I don't think we even need dynamic AML for _CST usecase at all, > > > existing cpuhp interface should work just fine for it and should be > > > simpler as all infrastructure is already there. > > > > Not sure I get what you mean. Could you post a patch? > > > > > > > 2. (probably biggest issue) Loading dynamically generated AML > > > > >basically would make all AML code ABI, so that static AML > > > > >in RAM of old QEMU version would match dynamic generated > > > > >one on target side with new QEMU (/me generalizing approach beyond > > > > > _CST support). > > > > >I'd try to keep our AML version less as much as possible > > > > >and go this route only if there is no other way to do it. > > > > > > > > That's a good point, thanks for bringing this up! > > > > > > > > So it seems that we will need to define the ABI, yes. All AML code is > > > > an over-statement though, there are specific entry points > > > > we must maintain, right? > > > Well, I'm rather unsure what and where could break, > > > hence I'm afraid of a new beast and it probably won't be easy > > > to convince me that ABI would be able to keep things manageable > > > and stable. > > > Considering big amount of AML code that we already have > > > I'm not confident eye balling during review and testing the latest > > > firmware/qemu would be sufficient as we suddenly would have exploded > > > test matrix where firmware in use is a mixed from old/and new parts. > > > > Well there's one exported method so far and it relies on one container > > device in static table set which does not sound too hard to keep stable. > > > > Given how simple the dynamic table is, how about just checking it > > with a unit test? It is literally a single return statement. > > If it returns a valid package, that is all that we > > care about. I can write some firmware to test that constraint. > > > > > > > > And that in the end isn't fundamentally different from the ABIs > > > > mandated by the ACPI spec. > > > > > > > > So I have these ideas to try to mitigate the issues: > > > > - document the ABI (like we have in the ACPI spec) > > > > - use a specific prefix for all external calls (like _ for ACPI spec > > > > ones) > > > > - use a specific (different) prefix for all internal loaded calls (like > > > > [A-Z] for non-ACPI spec ones) > > > ACPI spec horrible m
Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation
On Thu, 2 Aug 2018 11:18:08 +0200 Igor Mammedov wrote: > On Thu, 26 Jul 2018 19:09:22 +0300 > "Michael S. Tsirkin" wrote: > > > On Wed, Jul 25, 2018 at 05:53:35PM +0200, Igor Mammedov wrote: > > > On Wed, 25 Jul 2018 15:44:37 +0300 > > > "Michael S. Tsirkin" wrote: > > > > > > > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote: > > > > > On Tue, 10 Jul 2018 03:01:30 +0300 > > > > > "Michael S. Tsirkin" wrote: > > > > > > > > > > > Now that basic support for guest CPU PM is upstream, I started > > > > > > looking > > > > > > for making it migrateable. Since a VM can be migrated between > > > > > > different > > > > > > hosts, PM info needs to change each time with move the VM to a > > > > > > different > > > > > > host. > > > > > Considering notification is async, so there will be a window when > > > > > guest will be using old Cstates on new host. What will happen if > > > > > machine is migrated to host that doesn't support a Cstate > > > > > that was used on source when guest was started? > > > > > > > > My testing shows mwait with a wrong hint works, presumably it just uses > > > > a lot of power. > > > > > > > > > > This adds infrastructure - based on Load/Unload - to support exactly > > > > > > that: QEMU generates AML (changing it on migration) and stores it in > > > > > > reserved memory, OSPM loads _CST from there on demand. > > > > > Cool and very tempting idea but I have 2 worries about this approach: > > > > > 1. How well does it work with Windows based guests? > > > > >(I've tried something similar but generating new AML from AML > > > > > itself > > > > > to get rid of our long if/else chains there to make up Notify > > > > > target name. > > > > > I ditched it (unfortunately I don't recall why) ) > > > > > > > > After trying it, I can tell you why - it's a horrid mess of > > > > unreadable code, with arbitrary restraints on package > > > > length etc. > > > in my case it was only 4 character NameString, but Windows probably > > > wasn't happy or just ignored it. > > > Considering recent development (TPM series) it might have been issue > > > with SYSTEM_MEMORY not working properly if used as byte buffer field. > > > > > > Even if it's an unreadable mess it's still stable mess and very > > > constrained > > > one within a single firmware blob that came from source. > > > > That's exectly the argument we had for keeping ACPI > > generation in bios. It's just not an interface that scales. > > > > > Hence it's more preferable than split brain in this series. > > > > > > But I don't think we even need dynamic AML for _CST usecase at all, > > > existing cpuhp interface should work just fine for it and should be > > > simpler as all infrastructure is already there. > > > > Not sure I get what you mean. Could you post a patch? > > > > > > > 2. (probably biggest issue) Loading dynamically generated AML > > > > >basically would make all AML code ABI, so that static AML > > > > >in RAM of old QEMU version would match dynamic generated > > > > >one on target side with new QEMU (/me generalizing approach beyond > > > > > _CST support). > > > > >I'd try to keep our AML version less as much as possible > > > > >and go this route only if there is no other way to do it. > > > > > > > > That's a good point, thanks for bringing this up! > > > > > > > > So it seems that we will need to define the ABI, yes. All AML code is > > > > an over-statement though, there are specific entry points > > > > we must maintain, right? > > > Well, I'm rather unsure what and where could break, > > > hence I'm afraid of a new beast and it probably won't be easy > > > to convince me that ABI would be able to keep things manageable > > > and stable. > > > Considering big amount of AML code that we already have > > > I'm not confident eye balling during review and testing the latest > > > firmware/qemu would be sufficient as we suddenly would have exploded > > > test matrix where firmware in use is a mixed from old/and new parts. > > > > Well there's one exported method so far and it relies on one container > > device in static table set which does not sound too hard to keep stable. > > > > Given how simple the dynamic table is, how about just checking it > > with a unit test? It is literally a single return statement. > > If it returns a valid package, that is all that we > > care about. I can write some firmware to test that constraint. > > > > > > > > And that in the end isn't fundamentally different from the ABIs > > > > mandated by the ACPI spec. > > > > > > > > So I have these ideas to try to mitigate the issues: > > > > - document the ABI (like we have in the ACPI spec) > > > > - use a specific prefix for all external calls (like _ for ACPI spec > > > > ones) > > > > - use a specific (different) prefix for all internal loaded calls (like > > > > [A-Z] for non-ACPI spec ones
Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 2018071024.542612-1-...@redhat.com Subject: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu t [tag update] patchew/20180626135035.133432-1-vsement...@virtuozzo.com -> patchew/20180626135035.133432-1-vsement...@virtuozzo.com * [new tag] patchew/2018071024.542612-1-...@redhat.com -> patchew/2018071024.542612-1-...@redhat.com Switched to a new branch 'test' eea669947b pc: HACK: acpi: tie in _CST object to Processor 51455e6143 acpi: aml generation for _CST be172c3f6f acpi: init header without linking 8986b8080b acpi: export acpi_checksum 3c7a978139 acpi: aml_load/aml_unload 4fc698863e acpi: generalize aml_package / aml_varpackage 75d0818dd8 acpi: aml: add aml_register() === OUTPUT BEGIN === Checking PATCH 1/7: acpi: aml: add aml_register()... Checking PATCH 2/7: acpi: generalize aml_package / aml_varpackage... Checking PATCH 3/7: acpi: aml_load/aml_unload... Checking PATCH 4/7: acpi: export acpi_checksum... Checking PATCH 5/7: acpi: init header without linking... Checking PATCH 6/7: acpi: aml generation for _CST... WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #41: new file mode 100644 ERROR: trailing whitespace #139: FILE: hw/acpi/cst.c:94: + * a good idea for future extensions. $ ERROR: do not use C99 // comments #150: FILE: hw/acpi/cst.c:105: +//table_data->data[cstp_offset] = 0x8; /* hack */ WARNING: line over 80 characters #172: FILE: hw/acpi/cst.c:127: +acpi_init_header(dyn_ssdt_hdr, "SSDT", dyn_ssdt->buf->len, 1, NULL, "DYNSSDT"); WARNING: line over 80 characters #185: FILE: hw/acpi/cst.c:140: +static void cst_ioport_write(void *opaque, hwaddr addr, uint64_t data, unsigned size) WARNING: line over 80 characters #214: FILE: hw/acpi/cst.c:169: +memory_region_init_io(&cst_mr, NULL, &cst_ops, NULL, "cst-update-request", 4); total: 2 errors, 4 warnings, 187 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 7/7: pc: HACK: acpi: tie in _CST object to Processor... === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation
On Tue, 10 Jul 2018 03:01:30 +0300 "Michael S. Tsirkin" wrote: > Now that basic support for guest CPU PM is upstream, I started looking > for making it migrateable. Since a VM can be migrated between different > hosts, PM info needs to change each time with move the VM to a different > host. Considering notification is async, so there will be a window when guest will be using old Cstates on new host. What will happen if machine is migrated to host that doesn't support a Cstate that was used on source when guest was started? > This adds infrastructure - based on Load/Unload - to support exactly > that: QEMU generates AML (changing it on migration) and stores it in > reserved memory, OSPM loads _CST from there on demand. Cool and very tempting idea but I have 2 worries about this approach: 1. How well does it work with Windows based guests? (I've tried something similar but generating new AML from AML itself to get rid of our long if/else chains there to make up Notify target name. I ditched it (unfortunately I don't recall why) ) 2. (probably biggest issue) Loading dynamically generated AML basically would make all AML code ABI, so that static AML in RAM of old QEMU version would match dynamic generated one on target side with new QEMU (/me generalizing approach beyond _CST support). I'd try to keep our AML version less as much as possible and go this route only if there is no other way to do it. > This is a partially functional prototype. > > What works: > loading _CST dynamically and reporting it to OSPM > > What doesn't: > detecting host configuration and generating correct _CST package > notifying guest about the change to trigger _CST re-evaluation > disabling mwait/halt exists as appropriate > > Michael S. Tsirkin (6): > acpi: aml: add aml_register() > acpi: generalize aml_package / aml_varpackage > acpi: aml_load/aml_unload > acpi: export acpi_checksum > acpi: init header without linking > acpi: aml generation for _CST > pc: HACK: acpi: tie in _CST object to Processor > > include/hw/acpi/acpi.h | 2 + > include/hw/acpi/aml-build.h | 14 ++- > include/hw/acpi/cst.h | 8 ++ > include/hw/i386/pc.h| 5 ++ > hw/acpi/aml-build.c | 81 ++--- > hw/acpi/core.c | 2 +- > hw/acpi/cpu.c | 5 ++ > hw/acpi/cpu_hotplug.c | 11 +-- > hw/acpi/cst.c | 173 > hw/arm/virt-acpi-build.c| 2 +- > hw/i386/acpi-build.c| 10 ++- > hw/acpi/Makefile.objs | 2 +- > 12 files changed, 290 insertions(+), 25 deletions(-) > create mode 100644 include/hw/acpi/cst.h > create mode 100644 hw/acpi/cst.c >
Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation
On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote: > On Tue, 10 Jul 2018 03:01:30 +0300 > "Michael S. Tsirkin" wrote: > > > Now that basic support for guest CPU PM is upstream, I started looking > > for making it migrateable. Since a VM can be migrated between different > > hosts, PM info needs to change each time with move the VM to a different > > host. > Considering notification is async, so there will be a window when > guest will be using old Cstates on new host. What will happen if > machine is migrated to host that doesn't support a Cstate > that was used on source when guest was started? My testing shows mwait with a wrong hint works, presumably it just uses a lot of power. > > This adds infrastructure - based on Load/Unload - to support exactly > > that: QEMU generates AML (changing it on migration) and stores it in > > reserved memory, OSPM loads _CST from there on demand. > Cool and very tempting idea but I have 2 worries about this approach: > 1. How well does it work with Windows based guests? >(I've tried something similar but generating new AML from AML itself > to get rid of our long if/else chains there to make up Notify target name. > I ditched it (unfortunately I don't recall why) ) After trying it, I can tell you why - it's a horrid mess of unreadable code, with arbitrary restraints on package length etc. > 2. (probably biggest issue) Loading dynamically generated AML >basically would make all AML code ABI, so that static AML >in RAM of old QEMU version would match dynamic generated >one on target side with new QEMU (/me generalizing approach beyond _CST > support). >I'd try to keep our AML version less as much as possible >and go this route only if there is no other way to do it. That's a good point, thanks for bringing this up! So it seems that we will need to define the ABI, yes. All AML code is an over-statement though, there are specific entry points we must maintain, right? And that in the end isn't fundamentally different from the ABIs mandated by the ACPI spec. So I have these ideas to try to mitigate the issues: - document the ABI (like we have in the ACPI spec) - use a specific prefix for all external calls (like _ for ACPI spec ones) - use a specific (different) prefix for all internal loaded calls (like [A-Z] for non-ACPI spec ones) Thoughts? > > This is a partially functional prototype. > > > > What works: > > loading _CST dynamically and reporting it to OSPM > > > > What doesn't: > > detecting host configuration and generating correct _CST package > > notifying guest about the change to trigger _CST re-evaluation > > disabling mwait/halt exists as appropriate > > > > Michael S. Tsirkin (6): > > acpi: aml: add aml_register() > > acpi: generalize aml_package / aml_varpackage > > acpi: aml_load/aml_unload > > acpi: export acpi_checksum > > acpi: init header without linking > > acpi: aml generation for _CST > > pc: HACK: acpi: tie in _CST object to Processor > > > > include/hw/acpi/acpi.h | 2 + > > include/hw/acpi/aml-build.h | 14 ++- > > include/hw/acpi/cst.h | 8 ++ > > include/hw/i386/pc.h| 5 ++ > > hw/acpi/aml-build.c | 81 ++--- > > hw/acpi/core.c | 2 +- > > hw/acpi/cpu.c | 5 ++ > > hw/acpi/cpu_hotplug.c | 11 +-- > > hw/acpi/cst.c | 173 > > hw/arm/virt-acpi-build.c| 2 +- > > hw/i386/acpi-build.c| 10 ++- > > hw/acpi/Makefile.objs | 2 +- > > 12 files changed, 290 insertions(+), 25 deletions(-) > > create mode 100644 include/hw/acpi/cst.h > > create mode 100644 hw/acpi/cst.c > >
Re: [Qemu-devel] [PATCH hack dontapply v2 0/7] Dynamic _CST generation
On Wed, 25 Jul 2018 15:44:37 +0300 "Michael S. Tsirkin" wrote: > On Wed, Jul 25, 2018 at 02:32:11PM +0200, Igor Mammedov wrote: > > On Tue, 10 Jul 2018 03:01:30 +0300 > > "Michael S. Tsirkin" wrote: > > > > > Now that basic support for guest CPU PM is upstream, I started looking > > > for making it migrateable. Since a VM can be migrated between different > > > hosts, PM info needs to change each time with move the VM to a different > > > host. > > Considering notification is async, so there will be a window when > > guest will be using old Cstates on new host. What will happen if > > machine is migrated to host that doesn't support a Cstate > > that was used on source when guest was started? > > My testing shows mwait with a wrong hint works, presumably it just uses > a lot of power. > > > > This adds infrastructure - based on Load/Unload - to support exactly > > > that: QEMU generates AML (changing it on migration) and stores it in > > > reserved memory, OSPM loads _CST from there on demand. > > Cool and very tempting idea but I have 2 worries about this approach: > > 1. How well does it work with Windows based guests? > >(I've tried something similar but generating new AML from AML itself > > to get rid of our long if/else chains there to make up Notify target > > name. > > I ditched it (unfortunately I don't recall why) ) > > After trying it, I can tell you why - it's a horrid mess of > unreadable code, with arbitrary restraints on package > length etc. in my case it was only 4 character NameString, but Windows probably wasn't happy or just ignored it. Considering recent development (TPM series) it might have been issue with SYSTEM_MEMORY not working properly if used as byte buffer field. Even if it's an unreadable mess it's still stable mess and very constrained one within a single firmware blob that came from source. Hence it's more preferable than split brain in this series. But I don't think we even need dynamic AML for _CST usecase at all, existing cpuhp interface should work just fine for it and should be simpler as all infrastructure is already there. > > 2. (probably biggest issue) Loading dynamically generated AML > >basically would make all AML code ABI, so that static AML > >in RAM of old QEMU version would match dynamic generated > >one on target side with new QEMU (/me generalizing approach beyond _CST > > support). > >I'd try to keep our AML version less as much as possible > >and go this route only if there is no other way to do it. > > That's a good point, thanks for bringing this up! > > So it seems that we will need to define the ABI, yes. All AML code is > an over-statement though, there are specific entry points > we must maintain, right? Well, I'm rather unsure what and where could break, hence I'm afraid of a new beast and it probably won't be easy to convince me that ABI would be able to keep things manageable and stable. Considering big amount of AML code that we already have I'm not confident eye balling during review and testing the latest firmware/qemu would be sufficient as we suddenly would have exploded test matrix where firmware in use is a mixed from old/and new parts. > And that in the end isn't fundamentally different from the ABIs > mandated by the ACPI spec. > > So I have these ideas to try to mitigate the issues: > - document the ABI (like we have in the ACPI spec) > - use a specific prefix for all external calls (like _ for ACPI spec ones) > - use a specific (different) prefix for all internal loaded calls (like > [A-Z] for non-ACPI spec ones) ACPI spec horrible mess and bad example (even committee isn't able to keep it consistent) but it's necessary evil (compared to no spec at all). Adding ABI requirement will complicate coding/reviewing for already complex AML compared to current non ABI way. Hence from maintainability POV we should stay away from this approach if there is a viable alternative. > > Thoughts? > > > > This is a partially functional prototype. > > > > > > What works: > > > loading _CST dynamically and reporting it to OSPM > > > > > > What doesn't: > > > detecting host configuration and generating correct _CST package > > > notifying guest about the change to trigger _CST re-evaluation > > > disabling mwait/halt exists as appropriate > > > > > > Michael S. Tsirkin (6): > > > acpi: aml: add aml_register() > > > acpi: generalize aml_package / aml_varpackage > > > acpi: aml_load/aml_unload > > > acpi: export acpi_checksum > > > acpi: init header without linking > > > acpi: aml generation for _CST > > > pc: HACK: acpi: tie in _CST object to Processor > > > > > > include/hw/acpi/acpi.h | 2 + > > > include/hw/acpi/aml-build.h | 14 ++- > > > include/hw/acpi/cst.h | 8 ++ > > > include/hw/i386/pc.h| 5 ++ > > > hw/acpi/aml-build.c | 81 ++--- > > > hw/acpi/core.c | 2 +- > > > hw/acpi/cpu.c