Re: [PATCH RESEND v12 0/6] Support query and use SGX

[Michal Prívozník <mprivozn@xxxxxxxxxx>]

On 6/2/22 02:52, Yang, Lin A wrote:
> On 5/31/22, 7:29 AM, "Michal Prívozník" <mprivozn@xxxxxxxxxx> wrote:
> 

> 
>  
> 
>> 2) apparently, .node attribute is required? Now, it's true that
> 
>> initially my guest has 2 NUMA nodes defined, but even after I remove
> 
>> those I still see the error. I believe I've raised this issue in one of
> 
>> earlier reviews:
> 
>>
> 
>> https://listman.redhat.com/archives/libvir-list/2022-February/228835.html
> 
>>
> 
>> Please make sure that in v13 this is addressed (even at expense of not
> 
>> working with QEMU-6.2.0 and requiring newer QEMU, if that's needed).
> 
>> Wasting precious reviewer bandwidth does not help anybody.
> 
>  
> 
> The .node attribute is required for qemu 7.0.0, but qemu 6.2.0 doesn’t need
> 
> It. Missing .node will fail with qemu 7.0.0, even only define one NUMA node.
> 
>  
> 
> Giving it’s probably impossible to detect whether .node is needed or
> not, our
> 
> proposal here is this patch only works with qemu 6.2.0, then support
> qemu 7.0.0
> 
> in another patch. It might give user a chance to choose different
> libvirt version,
> 
> or pick up some commits to work with different qemu version.
> 
>  
> 
> If it’s unnecessary, we can definitely only support 7.0.0 as you suggested.

Worst case scenario we can do a version check. It's very suboptimal
because if somebody backports your patches in QEMU, libvirt will stop
working despite having the version check.

Therefore, I'm more inclined to just use the newest API and well, 6.2
won't work. In the long run - it's just one release that has the feature
but libvirt can't use it versus plenty of releases (that come after 7.0)
which have the feature and libvirt can use it. If we go this way then
we'll still need version check, but the other way round:

    if (qemuCaps->version < 7000000)
        virQEMUCapsClear(qemuCaps, QEMU_CAPS_SGX_EPC);

Or just exit early and don't even bother detecting SGX when version is
not 7.0.0:

    if (qemuCaps->version < 7000000)
        return 0;

This has the downside that when somebody backports QEMU patches to 6.2
to match the QAPI of 7.0 libvirt would still refuse to use the feature.
But one can argue that at that point the maintainer should also patch
libvirt (very trivial patch to remove these two lines of condition).

This is the reason we like QEMU to make features introspectable - we
could avoid all of this if we were able to detect .node attribute :-(

Now that I look at the output of query-qmp-schema command I see that 6.2
returns:

    {
      "name": "237",
      "members": [
        {
          "name": "sgx",
          "type": "bool"
        },
        {
          "name": "sgx1",
          "type": "bool"
        },
        {
          "name": "sgx2",
          "type": "bool"
        },
        {
          "name": "flc",
          "type": "bool"
        },
        {
          "name": "section-size",
          "type": "int"
        }
      ],
      "meta-type": "object"
    },

while 7.0 returns:

{
      "name": "237",
      "members": [
        {
          "name": "sgx",
          "type": "bool"
        },
        {
          "name": "sgx1",
          "type": "bool"
        },
        {
          "name": "sgx2",
          "type": "bool"
        },
        {
          "name": "flc",
          "type": "bool"
        },
        {
          "name": "section-size",
          "type": "int",
          "features": [
            "deprecated"
          ]
        },
        {
          "name": "sections",
          "type": "[454]"
        }
      ],
      "meta-type": "object"
    }


So maybe in the end libvirt CAN know the difference without having to do
any version check. We have a "dialect" of XPATH that we use to traverse
the QMP schema: look at the comment above virQEMUQAPISchemaPathGet().

Michal