On Wed, 22 May 2024 17:34:49 +0200
Anthony Harivel <ahari...@redhat.com> wrote:
> Dear maintainers,
>
> First of all, thank you very much for your review of my patch
> [1].
I've tried to play with this feature and have a few questions about it
1. trying to start with non accessible or not existent socket
-accel kvm,rapl=on,rapl-helper-socket=/tmp/socket
I get:
qemu-system-x86_64: -accel kvm,rapl=on,rapl-helper-socket=/tmp/socks:
vmsr socket opening failed
qemu-system-x86_64: -accel kvm,rapl=on,rapl-helper-socket=/tmp/socks: kvm
: error RAPL feature requirement not met
* is it possible to report actual OS error that happened during
open/connect,
instead of unhelpful 'socket opening failed'?
What I see in vmsr_open_socket() error is ignored
and btw it's error leak as well
* 2nd line shouldn't be there if the 1st error already present.
2. getting periodic error on console where QEMU has been starter
# ./qemu-vmsr-helper -k /tmp/sock
./qemu-system-x86_64 -snapshot -m 4G -accel
kvm,rapl=on,rapl-helper-socket=/tmp/sock rhel90.img -vnc :0 -cpu host
and let it run
it appears rdmsr works (well, it returns some values at least)
however there are recurring errors in qemu's stderr(or out)
qemu-system-x86_64: Error opening /proc/2496093/task/2496109/stat
qemu-system-x86_64: Error opening /proc/2496093/task/2496095/stat
My guess it's some temporary threads, that come and go, but still
they shouldn't cause errors if it's normal operation.
Also on daemon side, I a few times got while guest was running:
qemu-vmsr-helper: Failed to open /proc at /proc/2496026/task/2496044
qemu-vmsr-helper: Requested TID not in peer PID: 2496026 2496044
though I can't reproduce it reliably
3. when starting daemon not as root, it starts 'fine' but later on complains
qemu-vmsr-helper: Failed to open MSR file at /dev/cpu/0/msr
perhaps it would be better to fail at start daemon if it doesn't have
access to necessary files.
4. in case #3, guest also fails to start with errors:
qemu-system-x86_64: -accel kvm,rapl=on,rapl-helper-socket=/tmp/sock:
can't read any virtual msr
qemu-system-x86_64: -accel kvm,rapl=on,rapl-helper-socket=/tmp/sock: kvm
: error RAPL feature requirement not met
again line #2 is not useful and probably not needed (maybe make it
tracepoint)
and #1 is unhelpful - it would be better if it directed user to check
qemu-vmsr-helper
5. does AMD have similar MSRs that we could use to make this feature complete?
6. What happens to power accounting if host constantly migrates
vcpus between sockets, are values we are getting still correct/meaningful?
Or do we need to pin vcpus to get 'accurate' values?
7. do we have to have a dedicated thread for pooling data from daemon?
Can we fetch data from vcpu thread that have accessed msr
(with some caching and rate limiting access to the daemon)?
> In this version (v6), I have attempted to address all the problems
> addressed by Daniel and Paolo during the last review.
>
> However, two open questions remains unanswered that would require the
> attention of a x86 maintainers:
>
> 1)Should I move from -kvm to -cpu the rapl feature ? [2]
>
> 2)Should I already rename to "rapl_vmsr_*" in order to anticipate the
> futur TMPI architecture ? [end of 3]
>
> Thank you again for your continued guidance.
>
> v5 -> v6
> --------
> - Better error consistency in qio_channel_get_peerpid()
> - Memory leak g_strdup_printf/g_build_filename corrected
> - Renaming several struct with "vmsr_*" for better namespace
> - Renamed several struct with "guest_*" for better comprehension
> - Optimization suggerate from Daniel
> - Crash problem solved [4]
>
> v4 -> v5
> --------
>
> - correct qio_channel_get_peerpid: return pid = -1 in case of error
> - Vmsr_helper: compile only for x86
> - Vmsr_helper: use qio_channel_read/write_all
> - Vmsr_helper: abandon user/group
> - Vmsr_energy.c: correct all error_report
> - Vmsr thread: compute default socket path only once
> - Vmsr thread: open socket only once
> - Pass relevant QEMU CI
>
> v3 -> v4
> --------
>
> - Correct memory leaks with AddressSanitizer
> - Add sanity check for QEMU and qemu-vmsr-helper for checking if host is
> INTEL and if RAPL is activated.
> - Rename poor variables naming for easier comprehension
> - Move code that checks Host before creating the VMSR thread
> - Get rid of libnuma: create function that read sysfs for reading the
> Host topology instead
>
> v2 -> v3
> --------
>
> - Move all memory allocations from Clib to Glib
> - Compile on *BSD (working on Linux only)
> - No more limitation on the virtual package: each vCPU that belongs to
> the same virtual package is giving the same results like expected on
> a real CPU.
> This has been tested topology like:
> -smp 4,sockets=2
> -smp 16,sockets=4,cores=2,threads=2
>
> v1 -> v2
> --------
>
> - To overcome the CVE-2020-8694 a socket communication is created
> to a priviliged helper
> - Add the priviliged helper (qemu-vmsr-helper)
> - Add SO_PEERCRED in qio channel socket
>
> RFC -> v1
> ---------
>
> - Add vmsr_* in front of all vmsr specific function
> - Change malloc()/calloc()... with all glib equivalent
> - Pre-allocate all dynamic memories when possible
> - Add a Documentation of implementation, limitation and usage
>
> Best regards,
> Anthony
>
> [1]: https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg01570.html
> [2]: https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg03947.html
> [3]: https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg02350.html
> [4]: https://mail.gnu.org/archive/html/qemu-devel/2024-04/msg02481.html
>
> Anthony Harivel (3):
> qio: add support for SO_PEERCRED for socket channel
> tools: build qemu-vmsr-helper
> Add support for RAPL MSRs in KVM/Qemu
>
> accel/kvm/kvm-all.c | 27 ++
> contrib/systemd/qemu-vmsr-helper.service | 15 +
> contrib/systemd/qemu-vmsr-helper.socket | 9 +
> docs/specs/index.rst | 1 +
> docs/specs/rapl-msr.rst | 155 +++++++
> docs/tools/index.rst | 1 +
> docs/tools/qemu-vmsr-helper.rst | 89 ++++
> include/io/channel.h | 21 +
> include/sysemu/kvm_int.h | 32 ++
> io/channel-socket.c | 28 ++
> io/channel.c | 13 +
> meson.build | 7 +
> target/i386/cpu.h | 8 +
> target/i386/kvm/kvm.c | 431 +++++++++++++++++-
> target/i386/kvm/meson.build | 1 +
> target/i386/kvm/vmsr_energy.c | 337 ++++++++++++++
> target/i386/kvm/vmsr_energy.h | 99 +++++
> tools/i386/qemu-vmsr-helper.c | 530 +++++++++++++++++++++++
> tools/i386/rapl-msr-index.h | 28 ++
> 19 files changed, 1831 insertions(+), 1 deletion(-)
> create mode 100644 contrib/systemd/qemu-vmsr-helper.service
> create mode 100644 contrib/systemd/qemu-vmsr-helper.socket
> create mode 100644 docs/specs/rapl-msr.rst
> create mode 100644 docs/tools/qemu-vmsr-helper.rst
> create mode 100644 target/i386/kvm/vmsr_energy.c
> create mode 100644 target/i386/kvm/vmsr_energy.h
> create mode 100644 tools/i386/qemu-vmsr-helper.c
> create mode 100644 tools/i386/rapl-msr-index.h
>
