RE: [libvirt][PATCH v13 3/6] Convert QMP capabilities to domain capabilities
> BTW: I've discussed with Yang that I'll fix up these patches and resend. > Is that agreement still on? Yes, it's still on. Please go ahead. I just didn't get chance to sync with Haibin after we made agreement last week. Thanks, Lin.
Re: [libvirt][PATCH v13 3/6] Convert QMP capabilities to domain capabilities
On 7/25/22 03:31, Huang, Haibin wrote:
> Hi Michal Peter,
>
> Thank you for your comments.
>
> Way 1:
> virBufferAsprintf(buf, "%s\n", sgx->flc ? "yes" : "no");
>
> Way 2:
> if (sgx->flc) {
> virBufferAsprintf(buf, "yes\n");
> } else {
> virBufferAsprintf(buf, "yes\n");
> }
>
> For this section, both ways of writing work.
>
> Peter Krempa said: "Don't use the ternary operator ('?'), use a full if/else
> branch instead or pick a better data structure."
> You mean to be more concise use the ternary operator ('?').
Yeah, this one should be exempt from our ternary operator rule. The idea
behin the rule is to avoid complicated (or even composed) expressions
with a ternary operator. However, this is not the case and in fact,
ternary operator makes the code more readable. And we already use this
pattern for formatting other booleans, indeed.
We may switch to virTristateBool but one can argue that FLC is not
really tristate, it's either present or not; there's no _ABSENT state.
On the other hand, we may assign _ABSENT special meaning - the structure
was just allocated and does not hold decisive answer for FLC presence.
So let me switch to virTristateBool.
BTW: I've discussed with Yang that I'll fix up these patches and resend.
Is that agreement still on?
Michal
RE: [libvirt][PATCH v13 3/6] Convert QMP capabilities to domain capabilities
Hi Michal Peter,
Thank you for your comments.
Way 1:
virBufferAsprintf(buf, "%s\n", sgx->flc ? "yes" : "no");
Way 2:
if (sgx->flc) {
virBufferAsprintf(buf, "yes\n");
} else {
virBufferAsprintf(buf, "yes\n");
}
For this section, both ways of writing work.
Peter Krempa said: "Don't use the ternary operator ('?'), use a full if/else
branch instead or pick a better data structure."
You mean to be more concise use the ternary operator ('?').
I feel like it all makes sense.
> -Original Message-
> From: Michal Prívozník
> Sent: Wednesday, July 20, 2022 7:27 PM
> To: Yang, Lin A ; libvir-list@redhat.com; Huang, Haibin
> ; Ding, Jian-feng
> Subject: Re: [libvirt][PATCH v13 3/6] Convert QMP capabilities to domain
> capabilities
>
> On 7/1/22 21:14, Lin Yang wrote:
> > From: Haibin Huang
> >
> > the QMP capabilities:
> > {"return":
> > {
> > "sgx": true,
> > "section-size": 1024,
> > "flc": true
> > }
> > }
> >
> > the domain capabilities:
> >
> > yes
> > 1
> >
> >
> > Signed-off-by: Michal Privoznik
> > Signed-off-by: Haibin Huang
> > ---
> > src/qemu/qemu_capabilities.c | 230 ++
> > src/qemu/qemu_capabilities.h | 4 +
> > .../caps_6.2.0.x86_64.replies | 30 ++-
> > .../caps_6.2.0.x86_64.xml | 7 +
> > .../caps_7.0.0.x86_64.replies | 34 ++-
> > .../caps_7.0.0.x86_64.xml | 11 +
> > .../caps_7.1.0.x86_64.replies | 34 ++-
> > .../caps_7.1.0.x86_64.xml | 11 +
> > 8 files changed, 346 insertions(+), 15 deletions(-)
> >
> > diff --git a/src/qemu/qemu_capabilities.c
> > b/src/qemu/qemu_capabilities.c index 2c3be3ecec..57b5acb150 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
> >"chardev.qemu-vdagent", /*
> QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
> >"display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
> >"iothread.thread-pool-max", /*
> > QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
> > + "sgx-epc", /* QEMU_CAPS_SGX_EPC */
> > );
> >
> >
> > @@ -752,6 +753,8 @@ struct _virQEMUCaps {
> >
> > virSEVCapability *sevCapabilities;
> >
> > +virSGXCapability *sgxCapabilities;
> > +
> > /* Capabilities which may differ depending on the accelerator. */
> > virQEMUCapsAccel kvm;
> > virQEMUCapsAccel hvf;
> > @@ -1394,6 +1397,7 @@ struct virQEMUCapsStringFlags
> virQEMUCapsObjectTypes[] = {
> > { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
> > { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
> > { "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
> > +{ "sgx-epc", QEMU_CAPS_SGX_EPC },
> > };
> >
> >
> > @@ -1972,6 +1976,32 @@ virQEMUCapsSEVInfoCopy(virSEVCapability
> **dst,
> > }
> >
> >
> > +static int
> > +virQEMUCapsSGXInfoCopy(virSGXCapability **dst,
> > + virSGXCapability *src) {
> > +g_autoptr(virSGXCapability) tmp = NULL;
> > +
>
> For a convenience of caller, we can have a src == NULL check here and return
> early.
>
> > +tmp = g_new0(virSGXCapability, 1);
> > +
> > +tmp->flc = src->flc;
> > +tmp->sgx1 = src->sgx1;
> > +tmp->sgx2 = src->sgx2;
> > +tmp->section_size = src->section_size;
> > +
> > +if (src->nSections == 0) {
> > +tmp->nSections = 0;
> > +tmp->pSections = NULL;
> > +} else {
> > +tmp->nSections = src->nSections;
> > +tmp->pSections = src->pSections;
>
> Ouch, this will definitely lead to a double free. If I run
> qemucapabilitiestest
> after this patch I can see it clearly. I don't quite understand why you didn't
> though. Fortunately, we can use plain
> memcpy() since virSection struct does not contain any pointer, just a pair of
> integers.
>
> > +}
> > +
> > +*dst = g_steal_pointer();
> > +return 0;
> > +}
> > +
> > +
> > static void
> > virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
> >
Re: [libvirt][PATCH v13 3/6] Convert QMP capabilities to domain capabilities
On 7/1/22 21:14, Lin Yang wrote:
> From: Haibin Huang
>
> the QMP capabilities:
> {"return":
> {
> "sgx": true,
> "section-size": 1024,
> "flc": true
> }
> }
>
> the domain capabilities:
>
> yes
> 1
>
>
> Signed-off-by: Michal Privoznik
> Signed-off-by: Haibin Huang
> ---
> src/qemu/qemu_capabilities.c | 230 ++
> src/qemu/qemu_capabilities.h | 4 +
> .../caps_6.2.0.x86_64.replies | 30 ++-
> .../caps_6.2.0.x86_64.xml | 7 +
> .../caps_7.0.0.x86_64.replies | 34 ++-
> .../caps_7.0.0.x86_64.xml | 11 +
> .../caps_7.1.0.x86_64.replies | 34 ++-
> .../caps_7.1.0.x86_64.xml | 11 +
> 8 files changed, 346 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 2c3be3ecec..57b5acb150 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
>"chardev.qemu-vdagent", /* QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
>"display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
>"iothread.thread-pool-max", /*
> QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
> + "sgx-epc", /* QEMU_CAPS_SGX_EPC */
> );
>
>
> @@ -752,6 +753,8 @@ struct _virQEMUCaps {
>
> virSEVCapability *sevCapabilities;
>
> +virSGXCapability *sgxCapabilities;
> +
> /* Capabilities which may differ depending on the accelerator. */
> virQEMUCapsAccel kvm;
> virQEMUCapsAccel hvf;
> @@ -1394,6 +1397,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[]
> = {
> { "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
> { "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
> { "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
> +{ "sgx-epc", QEMU_CAPS_SGX_EPC },
> };
>
>
> @@ -1972,6 +1976,32 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst,
> }
>
>
> +static int
> +virQEMUCapsSGXInfoCopy(virSGXCapability **dst,
> + virSGXCapability *src)
> +{
> +g_autoptr(virSGXCapability) tmp = NULL;
> +
For a convenience of caller, we can have a src == NULL check here and
return early.
> +tmp = g_new0(virSGXCapability, 1);
> +
> +tmp->flc = src->flc;
> +tmp->sgx1 = src->sgx1;
> +tmp->sgx2 = src->sgx2;
> +tmp->section_size = src->section_size;
> +
> +if (src->nSections == 0) {
> +tmp->nSections = 0;
> +tmp->pSections = NULL;
> +} else {
> +tmp->nSections = src->nSections;
> +tmp->pSections = src->pSections;
Ouch, this will definitely lead to a double free. If I run
qemucapabilitiestest after this patch I can see it clearly. I don't
quite understand why you didn't though. Fortunately, we can use plain
memcpy() since virSection struct does not contain any pointer, just a
pair of integers.
> +}
> +
> +*dst = g_steal_pointer();
> +return 0;
> +}
> +
> +
> static void
> virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
> virQEMUCapsAccel *src)
> @@ -2053,6 +2083,12 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps)
> qemuCaps->sevCapabilities) < 0)
> return NULL;
>
> +
> +if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC) &&
> +virQEMUCapsSGXInfoCopy(>sgxCapabilities,
> + qemuCaps->sgxCapabilities) < 0)
> +return NULL;
> +
> return g_steal_pointer();
> }
>
> @@ -2091,6 +2127,7 @@ void virQEMUCapsDispose(void *obj)
> virCPUDataFree(qemuCaps->cpuData);
>
> virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
> +virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
>
> virQEMUCapsAccelClear(>kvm);
> virQEMUCapsAccelClear(>hvf);
> @@ -2616,6 +2653,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps)
> }
>
>
> +virSGXCapabilityPtr
> +virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps)
> +{
> +return qemuCaps->sgxCapabilities;
> +}
> +
> +
> static int
> virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps,
> qemuMonitor *mon)
> @@ -3442,6 +3486,31 @@ virQEMUCapsProbeQMPSEVCapabilities(virQEMUCaps
> *qemuCaps,
> }
>
>
> +static int
> +virQEMUCapsProbeQMPSGXCapabilities(virQEMUCaps *qemuCaps,
> + qemuMonitor *mon)
> +{
> +int rc = -1;
> +virSGXCapability *caps = NULL;
> +
> +if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC))
> +return 0;
> +
> +if ((rc = qemuMonitorGetSGXCapabilities(mon, )) < 0)
> +return -1;
> +
> +/* SGX isn't actually supported */
> +if (rc == 0) {
> +virQEMUCapsClear(qemuCaps, QEMU_CAPS_SGX_EPC);
> +return 0;
> +}
> +
> +virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
> +qemuCaps->sgxCapabilities = caps;
[libvirt][PATCH v13 3/6] Convert QMP capabilities to domain capabilities
From: Haibin Huang
the QMP capabilities:
{"return":
{
"sgx": true,
"section-size": 1024,
"flc": true
}
}
the domain capabilities:
yes
1
Signed-off-by: Michal Privoznik
Signed-off-by: Haibin Huang
---
src/qemu/qemu_capabilities.c | 230 ++
src/qemu/qemu_capabilities.h | 4 +
.../caps_6.2.0.x86_64.replies | 30 ++-
.../caps_6.2.0.x86_64.xml | 7 +
.../caps_7.0.0.x86_64.replies | 34 ++-
.../caps_7.0.0.x86_64.xml | 11 +
.../caps_7.1.0.x86_64.replies | 34 ++-
.../caps_7.1.0.x86_64.xml | 11 +
8 files changed, 346 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 2c3be3ecec..57b5acb150 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -671,6 +671,7 @@ VIR_ENUM_IMPL(virQEMUCaps,
"chardev.qemu-vdagent", /* QEMU_CAPS_CHARDEV_QEMU_VDAGENT */
"display-dbus", /* QEMU_CAPS_DISPLAY_DBUS */
"iothread.thread-pool-max", /*
QEMU_CAPS_IOTHREAD_THREAD_POOL_MAX */
+ "sgx-epc", /* QEMU_CAPS_SGX_EPC */
);
@@ -752,6 +753,8 @@ struct _virQEMUCaps {
virSEVCapability *sevCapabilities;
+virSGXCapability *sgxCapabilities;
+
/* Capabilities which may differ depending on the accelerator. */
virQEMUCapsAccel kvm;
virQEMUCapsAccel hvf;
@@ -1394,6 +1397,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = {
{ "s390-pv-guest", QEMU_CAPS_S390_PV_GUEST },
{ "virtio-mem-pci", QEMU_CAPS_DEVICE_VIRTIO_MEM_PCI },
{ "virtio-iommu-pci", QEMU_CAPS_DEVICE_VIRTIO_IOMMU_PCI },
+{ "sgx-epc", QEMU_CAPS_SGX_EPC },
};
@@ -1972,6 +1976,32 @@ virQEMUCapsSEVInfoCopy(virSEVCapability **dst,
}
+static int
+virQEMUCapsSGXInfoCopy(virSGXCapability **dst,
+ virSGXCapability *src)
+{
+g_autoptr(virSGXCapability) tmp = NULL;
+
+tmp = g_new0(virSGXCapability, 1);
+
+tmp->flc = src->flc;
+tmp->sgx1 = src->sgx1;
+tmp->sgx2 = src->sgx2;
+tmp->section_size = src->section_size;
+
+if (src->nSections == 0) {
+tmp->nSections = 0;
+tmp->pSections = NULL;
+} else {
+tmp->nSections = src->nSections;
+tmp->pSections = src->pSections;
+}
+
+*dst = g_steal_pointer();
+return 0;
+}
+
+
static void
virQEMUCapsAccelCopyMachineTypes(virQEMUCapsAccel *dst,
virQEMUCapsAccel *src)
@@ -2053,6 +2083,12 @@ virQEMUCaps *virQEMUCapsNewCopy(virQEMUCaps *qemuCaps)
qemuCaps->sevCapabilities) < 0)
return NULL;
+
+if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC) &&
+virQEMUCapsSGXInfoCopy(>sgxCapabilities,
+ qemuCaps->sgxCapabilities) < 0)
+return NULL;
+
return g_steal_pointer();
}
@@ -2091,6 +2127,7 @@ void virQEMUCapsDispose(void *obj)
virCPUDataFree(qemuCaps->cpuData);
virSEVCapabilitiesFree(qemuCaps->sevCapabilities);
+virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
virQEMUCapsAccelClear(>kvm);
virQEMUCapsAccelClear(>hvf);
@@ -2616,6 +2653,13 @@ virQEMUCapsGetSEVCapabilities(virQEMUCaps *qemuCaps)
}
+virSGXCapabilityPtr
+virQEMUCapsGetSGXCapabilities(virQEMUCaps *qemuCaps)
+{
+return qemuCaps->sgxCapabilities;
+}
+
+
static int
virQEMUCapsProbeQMPCommands(virQEMUCaps *qemuCaps,
qemuMonitor *mon)
@@ -3442,6 +3486,31 @@ virQEMUCapsProbeQMPSEVCapabilities(virQEMUCaps *qemuCaps,
}
+static int
+virQEMUCapsProbeQMPSGXCapabilities(virQEMUCaps *qemuCaps,
+ qemuMonitor *mon)
+{
+int rc = -1;
+virSGXCapability *caps = NULL;
+
+if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_SGX_EPC))
+return 0;
+
+if ((rc = qemuMonitorGetSGXCapabilities(mon, )) < 0)
+return -1;
+
+/* SGX isn't actually supported */
+if (rc == 0) {
+virQEMUCapsClear(qemuCaps, QEMU_CAPS_SGX_EPC);
+return 0;
+}
+
+virSGXCapabilitiesFree(qemuCaps->sgxCapabilities);
+qemuCaps->sgxCapabilities = caps;
+return 0;
+}
+
+
/*
* Filter for features which should never be passed to QEMU. Either because
* QEMU never supported them or they were dropped as they never did anything
@@ -4220,6 +4289,116 @@ virQEMUCapsParseSEVInfo(virQEMUCaps *qemuCaps,
xmlXPathContextPtr ctxt)
}
+static int
+virQEMUCapsParseSGXInfo(virQEMUCaps *qemuCaps,
+xmlXPathContextPtr ctxt)
+{
+g_autoptr(virSGXCapability) sgx = NULL;
+xmlNodePtr node;
+
+g_autofree xmlNodePtr *nodes = NULL;
+g_autofree xmlNodePtr *sectionNodes = NULL;
+g_autofree char *flc = NULL;
+g_autofree char *sgx1 = NULL;
+g_autofree char *sgx2 = NULL;
+
+int n = 0;
+int nsections = 0;
+
+if
