[libvirt][PATCH v9 1/5] Get SGX Capabilities from QEMU

[Michal Prívozník]

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 at redhat.com>
>> Sent: Friday, January 7, 2022 11:06 PM
>> To: Huang, Haibin <haibin.huang at intel.com>; libvir-list at redhat.com; Ding,
>> Jian-feng <jian-feng.ding at intel.com>; Yang, Lin A <lin.a.yang at intel.com>; Lu,
>> Lianhao <lianhao.lu at intel.com>; Zhong, Yang <yang.zhong at 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 at 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