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 <mpriv...@redhat.com>
>> Sent: Friday, January 7, 2022 11:06 PM
>> To: Huang, Haibin <haibin.hu...@intel.com>; libvir-list@redhat.com; Ding,
>> Jian-feng <jian-feng.d...@intel.com>; Yang, Lin A <lin.a.y...@intel.com>; Lu,
>> Lianhao <lianhao...@intel.com>; Zhong, Yang <yang.zh...@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.hu...@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
