New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
runtime-rs: add QMP support for Qemu(part I) #9604
base: main
Are you sure you want to change the base?
Conversation
This patch can make kata/qemu running with qmp socket set well ! and the dumped cmdline as below:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than an ignorant question regarding a log message for validation. It looks good to me. Thanks @Apokleos !
// QMP listener file descriptor to be passed to qemu | ||
pub fd: Option<File>, | ||
// name is the socket name. | ||
pub name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing an fd and passing a name are mutually exclusive in QEMU. It is certainly possible to model that in a more idiomatic way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, we can just use the
unix:fd=xxx..
and no need to keep thename
forunix:path=xx...
. And I prepare to remove the fieldname
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...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
b67794b
to
2bfd42e
Compare
@@ -1212,6 +1320,8 @@ pub struct QemuCmdLine<'a> { | |||
smp: Smp, | |||
machine: Machine, | |||
cpu: Cpu, | |||
rtc: Rtc, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering why this item is added here even if it is added to devices
below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is certainly a rebase accident because rtc
was just removed from there by #9571 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I will fix it.
@@ -1228,6 +1296,9 @@ impl<'a> QemuCmdLine<'a> { | |||
smp: Smp::new(config), | |||
machine: Machine::new(config), | |||
cpu: Cpu::new(config), | |||
rtc: Rtc::new(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -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?); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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! |
8bcf53c
to
dd06917
Compare
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>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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)?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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? |
bf528f5
to
f3139c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
/// - "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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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>
add support QMP socket in cmdline
Fixes: #9603
Signed-off-by: Alex Lyn alex.lyn@antgroup.com