runtime-rs: add QMP support for Qemu(part I) #9604

Apokleos · 2024-05-07T08:51:28Z

add support QMP socket in cmdline

Signed-off-by: Alex Lyn alex.lyn@antgroup.com

Apokleos · 2024-05-07T08:57:03Z

This patch can make kata/qemu running with qmp socket set well ! and the dumped cmdline as below:

/opt/kata/bin/qemu-system-x86_64 -name sandbox-1e28630de2b9b2e8bf22714561899249f6254fc8a9d167c454f3112f088a0710 -kernel /opt/kata/share/kata-containers/vmlinux-6.1.52-116 -append reboot=k panic=1 systemd.unit=kata-containers.target systemd.mask=systemd-networkd.service root=/dev/pmem0p1 rootflags=dax,data=ordered,errors=remount-ro ro rootfstype=ext4 -qmp unix:fd=14,server=on,wait=off -smp 1,maxcpus=6 -machine q35,accel=kvm,nvdimm=on -cpu host,pmu=off -m 2G,slots=128,maxmem=15668M -object memory-backend-file,id=entire-guest-memory-share,mem-path=/dev/shm,size=2G,share=on,readonly=off -rtc base=utc,clock=host,driftfix=slew -device vhost-vsock-pci,vhostfd=15,guest-cid=3259189863 -netdev tap,id=network-0,vhost=on,vhostfds=18,fds=17 -device virtio-net-pci,netdev=network-0,mac=9a:6a:d4:5f:60:60,mq=on,vectors=4 -chardev socket,id=virtiofsd-chardev,path=virtiofsd.sock -device vhost-user-fs-pci,chardev=virtiofsd-chardev,tag=kataShared -numa node,memdev=entire-guest-memory-share -object memory-backend-file,id=TODO,mem-path=/opt/kata/share/kata-containers/kata-ubuntu-latest.image,size=128M,share=off,readonly=on -device nvdimm,memdev=TODO,unarmed=on -device virtio-serial-pci,id=serial0 -device virtconsole,id=console0,chardev=charconsole0 -chardev socket,id=charconsole0,server=on,wait=off,path=console.sock -vga none -no-user-config -nodefaults -nographic -no-reboot

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs

BbolroC

Other than an ignorant question regarding a log message for validation. It looks good to me. Thanks @Apokleos !

gkurz · 2024-05-07T12:55:08Z

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs

+    // QMP listener file descriptor to be passed to qemu
+    pub fd: Option<File>,
+    // name is the socket name.
+    pub name: String,


Passing an fd and passing a name are mutually exclusive in QEMU. It is certainly possible to model that in a more idiomatic way.

Yes, I don't think we'll ever use both of these options, will we? We should leave out the one that we don't use. The rationale is again, unlike (the original) govmm our ambition is not a generic qemu support that can do whatever qemu can do - for the sake of simplicity, we only support what's actually useful for runtime-rs.

IMO, we can just use the unix:fd=xxx.. and no need to keep the name for unix:path=xx....
And I prepare to remove the field name in the next version.

IMO, we can just use the unix:fd=xxx.. and no need to keep the name for unix:path=xx.... And I prepare to remove the field name in the next version.

Hold on. It is true that the primary QMP control point will only be unix:fd=XXX but the go runtime also offers the extra_monitor_socket configuration that allows to add an extra QMP/HMP control point for debugging purpose. The latter can only be unix:path=xx....

Right, that's a good point. We need to decide now whether the base and extra sockets share enough configuration to justify both being represented by the same struct - otherwise the extra socket might be better represented by a separate struct.

@pmores @gkurz sure, I will take them into account ！

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs

pmores · 2024-05-07T16:42:21Z

@Apokleos Have you considered using an existing crate to take care of the low-level details of QMP? Maybe something like qapi-qmp could help us focus on what's important to us?

Apokleos · 2024-05-08T00:53:04Z

@Apokleos Have you considered using an existing crate to take care of the low-level details of QMP? Maybe something like qapi-qmp could help us focus on what's important to us?

Yes, you're right ! it seems fine to be the basis of QMP implementation for us and we don't need to reinvent the wheel with it.

Apokleos · 2024-05-14T07:55:28Z

Hi @pmores @gkurz PTAL!

BbolroC · 2024-05-14T13:42:54Z

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs

@@ -1212,6 +1320,8 @@ pub struct QemuCmdLine<'a> {
    smp: Smp,
    machine: Machine,
    cpu: Cpu,
+    rtc: Rtc,


I was wondering why this item is added here even if it is added to devices below.

This is certainly a rebase accident because rtc was just removed from there by #9571 😉

sorry, I will fix it.

gkurz · 2024-05-14T14:43:36Z

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs

@@ -1228,6 +1296,9 @@ impl<'a> QemuCmdLine<'a> {
            smp: Smp::new(config),
            machine: Machine::new(config),
            cpu: Cpu::new(config),
+            rtc: Rtc::new(),


gkurz · 2024-05-14T14:43:46Z

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs

@@ -1437,6 +1508,10 @@ impl<'a> QemuCmdLine<'a> {
        result.append(&mut self.machine.qemu_params().await?);
        result.append(&mut self.cpu.qemu_params().await?);
        result.append(&mut self.memory.qemu_params().await?);
+        result.append(&mut self.rtc.qemu_params().await?);


gkurz

@Apokleos , as explained in #9604 (comment) , the extra monitor socket must be created with unix:path=. The reason is that this extra control socket is to be used directly by the user with an external tool, e.g. nc. The shim won't need to pre-connect to the extra socket like it does with the primary one.

Apokleos · 2024-05-15T00:59:15Z

@Apokleos , as explained in #9604 (comment) , the extra monitor socket must be created with unix:path=. The reason is that this extra control socket is to be used directly by the user with an external tool, e.g. nc. The shim won't need to pre-connect to the extra socket like it does with the primary one.

Ok, Thx Greg!

pmores · 2024-05-15T09:29:03Z

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs

-    fn new() -> Result<Vec<Self>> {
-        let qmp_sockets: Vec<QmpSocket> = Vec::new();
-        let _proto = MonitorProtocol::new("qmp");
+    fn new(sid: &str, extra_monitor: bool) -> Result<Vec<Self>> {


I guess this works, however we now have a constructor that returns a container of instances instead of an instance - which is kind of unexpected. :-) Honestly I'd prefer if the QmpSocket::new() took the monitor protocol as a parameter and then returned a single instance as usual.

Then QmpSocket::fd and QmpSocket::name had probably better become a single member of an enum type since only one of them is ever needed.

Also, QemuCmdLine::new() would unconditionally create a QmpSocket instance for the Qmp socket and optionally, based on hypervisor config, another instance for Hmp, similar to how MMIO and RTC are already handled.

pmores · 2024-05-15T09:31:19Z

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs

@@ -1299,7 +1352,7 @@ impl<'a> QemuCmdLine<'a> {
            smp: Smp::new(config),
            machine: Machine::new(config),
            cpu: Cpu::new(config),
-            qmp_sockets: QmpSocket::new()?,
+            qmp_sockets: QmpSocket::new(id, false)?,


I believe that the Qmp sockets should go to the anonymous devices container since QemuCmdLine doesn't need to interact with them anymore once they are created (@gkurz ?) so there's no need to hold them by name for future reference.

pmores · 2024-05-15T09:33:32Z

@Apokleos I proposed a bunch of structural changes - if you're fine with them but perhaps lack time to rework the PR to implement them we can merge as-is and I can make the changes subsequently. What do you think?

pmores

Looks good to me, thanks @Apokleos !

(As a nit, I suspect the last two commits could be squashed however I leave that to your discretion.)

gkurz · 2024-05-21T15:32:18Z

src/libs/kata-types/src/config/hypervisor/mod.rs

+    /// - "qmp-pretty" (same as "qmp" with pretty json formatting)
+    /// If set to the empty string "", no debug monitor socket is added. This is
+    /// the default.
+    /// dbg_monitor_socket = hmp


Suggested change

/// dbg_monitor_socket = hmp

/// dbg_monitor_socket = "hmp"

Original config in go runtime also has this nit. No big deal.

This option allows to add a debug monitor socket when `enable_debug = true` to control QEMU within debugging case. Fixes: kata-containers#9603 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

Fixes: kata-containers#9603 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

Apokleos requested review from gkurz, pmores, lifupan and BbolroC May 7, 2024 08:51

katacontainersbot added the size/large Task of significant size label May 7, 2024

Apokleos added the ok-to-test label May 7, 2024

BbolroC reviewed May 7, 2024

View reviewed changes

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs Outdated Show resolved Hide resolved

pmores reviewed May 7, 2024

View reviewed changes

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs Outdated Show resolved Hide resolved

BbolroC reviewed May 7, 2024

View reviewed changes

src/runtime-rs/crates/hypervisor/src/qemu/cmdline_generator.rs Outdated Show resolved Hide resolved

BbolroC approved these changes May 7, 2024

View reviewed changes

gkurz reviewed May 7, 2024

View reviewed changes

Apokleos force-pushed the qmp-cmdline01 branch 2 times, most recently from b67794b to 2bfd42e Compare May 14, 2024 07:50

Apokleos requested review from gkurz, BbolroC and pmores May 14, 2024 07:54

BbolroC reviewed May 14, 2024

View reviewed changes

gkurz reviewed May 14, 2024

View reviewed changes

Apokleos force-pushed the qmp-cmdline01 branch 3 times, most recently from 8bcf53c to dd06917 Compare May 15, 2024 08:04

Apokleos requested a review from gkurz May 15, 2024 08:06

pmores reviewed May 15, 2024

View reviewed changes

Apokleos added the wip Work in Progress (PR incomplete - needs more work or rework) label May 16, 2024

Apokleos force-pushed the qmp-cmdline01 branch 2 times, most recently from bf528f5 to f3139c6 Compare May 16, 2024 13:09

Apokleos removed the wip Work in Progress (PR incomplete - needs more work or rework) label May 17, 2024

Apokleos requested a review from pmores May 20, 2024 02:32

pmores approved these changes May 21, 2024

View reviewed changes

gkurz reviewed May 21, 2024

View reviewed changes

amshinde approved these changes May 22, 2024

View reviewed changes

Apokleos added 2 commits May 24, 2024 22:16

runtime-rs: add an option dbg_monitor_socket for HMP support

7ed6c68

This option allows to add a debug monitor socket when `enable_debug = true` to control QEMU within debugging case. Fixes: kata-containers#9603 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

runtime-rs: add QMP params suppport in cmdline

41fb7ae

Fixes: kata-containers#9603 Signed-off-by: Alex Lyn <alex.lyn@antgroup.com>

Apokleos force-pushed the qmp-cmdline01 branch from f3139c6 to 41fb7ae Compare May 24, 2024 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime-rs: add QMP support for Qemu(part I) #9604

runtime-rs: add QMP support for Qemu(part I) #9604

Apokleos commented May 7, 2024

Apokleos commented May 7, 2024

BbolroC left a comment

gkurz May 7, 2024

pmores May 7, 2024

Apokleos May 8, 2024

gkurz May 13, 2024

pmores May 13, 2024

Apokleos May 14, 2024

pmores commented May 7, 2024

Apokleos commented May 8, 2024

Apokleos commented May 14, 2024

BbolroC May 14, 2024 •

edited

gkurz May 14, 2024 •

edited

Apokleos May 15, 2024

gkurz May 14, 2024

gkurz May 14, 2024

gkurz left a comment

Apokleos commented May 15, 2024

pmores May 15, 2024

pmores May 15, 2024

pmores commented May 15, 2024

pmores left a comment

gkurz May 21, 2024

runtime-rs: add QMP support for Qemu(part I) #9604

Are you sure you want to change the base?

runtime-rs: add QMP support for Qemu(part I) #9604

Conversation

Apokleos commented May 7, 2024

Apokleos commented May 7, 2024

BbolroC left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmores commented May 7, 2024

Apokleos commented May 8, 2024

Apokleos commented May 14, 2024

BbolroC May 14, 2024 • edited

Choose a reason for hiding this comment

gkurz May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkurz left a comment

Choose a reason for hiding this comment

Apokleos commented May 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmores commented May 15, 2024

pmores left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BbolroC May 14, 2024 •

edited

gkurz May 14, 2024 •

edited