Re: [libvirt][PATCH v9 1/5] Get SGX Capabilities from QEMU
On 1/20/22 02:59, Huang, Haibin wrote:
Hi Michael,
Thank you for your comments.
> There's too much going on in this patch. You are querying qemu for SGX
> support and filling domain caps. At least the domain caps should go into the
> next patch.
Ok , we can put domain_capabilities.c in to the next patch, but virSGXCapability is
define domain_capabilities.h, qemu_monitor_json.c will use it.
If we also put it to next patch, then this patch will not work.
So, I think we can just put domain_capabilities.c in to the next patch, ok?
Yes, that's one of the options. Firstly, modify
src/qemu/qemu_capabilities.c so that the capability is detected, and
only after that implement domain_capabilities so that the capability is
reported.
Alternatively, you can introduce virSGXCapability machinery in one patch
and then QEMU detection in another.
> -----Original Message-----
> From: Michal Prívozník <mprivozn(a)redhat.com>
> Sent: Friday, January 7, 2022 11:06 PM
> To: Huang, Haibin <haibin.huang(a)intel.com>; libvir-list(a)redhat.com; Ding,
> Jian-feng <jian-feng.ding(a)intel.com>; Yang, Lin A <lin.a.yang(a)intel.com>;
Lu,
> Lianhao <lianhao.lu(a)intel.com>; Zhong, Yang <yang.zhong(a)intel.com>
> Subject: Re: [libvirt][PATCH v9 1/5] Get SGX Capabilities from QEMU
>
> On 12/15/21 04:40, Haibin Huang wrote:
>> The Qemu QMP provide the command "query-sgx-capabilities"
>> libvirt call the command to get sgx capabilities
>>
>> {"execute":"query-sgx-capabilities"}
>> {"return":
>> {"sgx": true, "sgx1": true, "sgx2": false,
"section-size": 0, \
>> "flc": false}}
>>
>> Signed-off-by: Haibin Huang <haibin.huang(a)intel.com>
>> ---
>> src/conf/domain_capabilities.c | 10 ++
>> src/conf/domain_capabilities.h | 13 ++
>> src/libvirt_private.syms | 1 +
>> src/qemu/qemu_capabilities.c | 143 +++++++++++++++++-
>> src/qemu/qemu_capabilities.h | 4 +
>> src/qemu/qemu_monitor.c | 10 ++
>> src/qemu/qemu_monitor.h | 3 +
>> src/qemu/qemu_monitor_json.c | 83 ++++++++++
>> src/qemu/qemu_monitor_json.h | 3 +
>> .../caps_6.2.0.x86_64.replies | 22 ++-
>> .../caps_6.2.0.x86_64.xml | 5 +
>> 11 files changed, 292 insertions(+), 5 deletions(-)
>
>
> There's too much going on in this patch. You are querying qemu for SGX
> support and filling domain caps. At least the domain caps should go into the
> next patch.
Ok , we can put domain_capabilities.c in to the next patch, but virSGXCapability is
define domain_capabilities.h, qemu_monitor_json.c will use it.
If we also put it to next patch, then this patch will not work.
The rule is to break huge change into small semantic chunks. It doesn't
mean that only one file/dir can be changed. If you need to declare a
struct just do it. But detecting SGX capability from QEMU and reporting
it in domain capabilities are two semantically disticnt things, thus
should be in two separate patches.
Michal