[libvirt PATCH v2] qemu: substitute missing model name for host-passthrough

[Collin Walling]

On 9/23/20 5:01 PM, Jiri Denemark wrote:
> On Wed, Sep 23, 2020 at 16:17:04 -0400, Collin Walling wrote:
>> On 9/23/20 2:52 PM, Collin Walling wrote:
>>> On 9/23/20 10:18 AM, Jiri Denemark wrote:
>>>> On Wed, Sep 23, 2020 at 09:26:58 +0200, Tim Wiederhake wrote:
>>>>> From: Collin Walling <walling at linux.ibm.com>
>>>>>
>>>>> Before:
>>>>>   $ uname -m
>>>>>   s390x
>>>>>   $ cat passthrough-cpu.xml
>>>>>   <cpu check="none" mode="host-passthrough" />
>>>>>   $ virsh hypervisor-cpu-compare passthrough-cpu.xml
>>>>>   error: Failed to compare hypervisor CPU with passthrough-cpu.xml
>>>>>   error: internal error: unable to execute QEMU command 'query-cpu-model-comp
>>>>>   arison': Invalid parameter type for 'modelb.name', expected: string
>>>>>
>>>>> After:
>>>>>   $ virsh hypervisor-cpu-compare passthrough-cpu.xml
>>>>>   CPU described in passthrough-cpu.xml is identical to the CPU provided by hy
>>>>>   pervisor on the host
>>>>>
>>>>> Signed-off-by: Tim Wiederhake <twiederh at redhat.com>
>>>>> ---
>>>>>  src/qemu/qemu_driver.c | 9 +++++++++
>>>>>  1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>>>>> index ae715c01d7..1cecef01f7 100644
>>>>> --- a/src/qemu/qemu_driver.c
>>>>> +++ b/src/qemu/qemu_driver.c
>>>>> @@ -12336,6 +12336,15 @@ qemuConnectCompareHypervisorCPU(virConnectPtr conn,
>>>>>          if (virCPUDefParseXMLString(xmlCPU, VIR_CPU_TYPE_AUTO, &cpu) < 0)
>>>>>              goto cleanup;
>>>>>  
>>>>> +        if (!cpu->model) {
>>>>> +            if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
>>>>> +                cpu->model = g_strdup("host");
>>>>> +            } else {
>>>>> +                virReportError(VIR_ERR_INVALID_ARG, "%s",
>>>>> +                               _("cpu parameter is missing a model name"));
>>>>> +                goto cleanup;
>>>>> +            }
>>>>> +        }
>>>>>          ret = qemuConnectCPUModelComparison(qemuCaps, cfg->libDir,
>>>>>                                              cfg->user, cfg->group,
>>>>>                                              hvCPU, cpu, failIncompatible);
>>>>
>>>> Reviewed-by: Jiri Denemark <jdenemar at redhat.com>
>>>>
>>>> I'll wait some time for Collin to add Signed-of-by tag before pushing
>>>> this.
>>>>
>>>
>>> Signed-off-by: Collin Walling <walling at linux.ibm.com>
>>>
>>> Thanks!
>>>
>>
>> Actually, it might help to extend this functionality for baseline as
>> well. If anything to at least catch the case when a CPU definition in
>> the XML file is missing a <model> tag. Right now, virsh will either
>> report "an unknown error occurred" when the XML file contains a _single_
>> <cpu> element without a <model> tag, or it will report the same "Invalid
>> parameter type for 'modela.name', expected: string" mentioned above when
>> there are multiple definitions in the file, and at least one of them is
>> missing a <model> tag.
> 
> Hmm, I would expect libvirt to complain about missing CPU model. If
> that's not the case, we need to fix it. But we should not fix it the
> same way. This change in the Compare API is hidden inside libvirt, we
> just tell QEMU we're comparing "host" when the cpu->mode is
> host-passthrough and get the result, which is basically yes/no.
> 
> But with baseline we get the result and make a guest CPU definition out
> of it. By setting cpu->model to "host" we would could end up with
> <model>host</model> in the result or even random result depending on the
> host performing the baseline API. This API should just reject any CPU
> definition without <model> as invalid argument.
> 
> Jirka
> 

On s390x, QMP will internally convert the model "host" to a proper CPU
model.

How about when baselining and if there is only a single CPU definition
and the model is "host" (either provided verbatim by the file, or
converted when the mode is host-passthrough), then perform a CPU model
expansion on the single CPU definition? No baselining would technically
be performed in this case.

I think this would support the behavior reported by the virsh man page:

"""
When FILE contains only a single CPU definition, the command will print
     the same CPU with restrictions  imposed  by  the  capabilities  of
the       hypervisor.   Specifically,  running  th  virsh
hypervisor-cpu-baseline       command with no additional options on the
result of virsh  domcapabili‐       ties  will transform the host CPU
model from domain capabilities XML to       a form directly usable in
domain XML.
"""

Essentially 2 patches:
 - set model name to "host" when mode is "host-passthrough" for baseline
 - use CPU model expansion when baseline is executed with only a single CPU

I can propose the patches tonight and we can look them over whenever.

-- 
Regards,
Collin

Stay safe and stay healthy