| Message ID | 20180710000024.542612-1-mst@redhat.com |
|---|---|
| Headers | show |
| Series | Dynamic _CST generation | expand |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20180710000024.542612-1-mst@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-vsementsov@virtuozzo.com -> patchew/20180626135035.133432-1-vsementsov@virtuozzo.com
* [new tag] patchew/20180710000024.542612-1-mst@redhat.com -> patchew/20180710000024.542612-1-mst@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-devel@redhat.com
On Tue, 10 Jul 2018 03:01:30 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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 >
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" <mst@redhat.com> 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 > >
On Wed, 25 Jul 2018 15:44:37 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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 | 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 > > > >
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" <mst@redhat.com> 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" <mst@redhat.com> 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: > > > > 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 > > > > > >
On Thu, 26 Jul 2018 19:09:22 +0300 "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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" <mst@redhat.com> 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. I'll try to come up with a patch to make use of cpuhp registers to pass CST values and it's AML counterpart. Then you'd need to just wire it up to whatever means you use to get these values from host. BTW: I've noticed you have used SystemIO registers in CST (my patch used FFH registers) using mmio defeats purpose as it causes guest exit instead of mwait that we are targeting. Is it just for demo purpose of RFC with new AML interface? > > > > > > > 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 > > > > > > > >
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" <mst@redhat.com> 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" <mst@redhat.com> 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" <mst@redhat.com> 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. > > I'll try to come up with a patch to make use of cpuhp registers > to pass CST values and it's AML counterpart. > Then you'd need to just wire it up to whatever means you use > to get these values from host. > > BTW: > I've noticed you have used SystemIO registers in CST > (my patch used FFH registers) using mmio defeats purpose > as it causes guest exit instead of mwait that we are targeting. > > Is it just for demo purpose of RFC with new AML interface? Just a demo. Actual mwait will need linux changes to make linux obey acpi if it conflicts with cpuid. > > > > > > > > > > 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 > > > > > > > > > >
On Thu, 2 Aug 2018 11:18:08 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 26 Jul 2018 19:09:22 +0300 > "Michael S. Tsirkin" <mst@redhat.com> 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" <mst@redhat.com> 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" <mst@redhat.com> 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. > > I'll try to come up with a patch to make use of cpuhp registers > to pass CST values and it's AML counterpart. > Then you'd need to just wire it up to whatever means you use > to get these values from host. Posted a non dynamic variant [RFC PATCH 0/4] "pc: acpi: _CST support" with uses conventional approach to compose _CST object without opening AML to ABI issues [...]
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. 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. 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