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

2020-09-24 Thread Jiri Denemark 
On Wed, Sep 23, 2020 at 18:29:54 -0400, Collin Walling wrote:
> 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:
> >> 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  tag. Right now, virsh will either
> >> report "an unknown error occurred" when the XML file contains a _single_
> >>  element without a  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  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
> > host in the result or even random result depending on the
> > host performing the baseline API. This API should just reject any CPU
> > definition without  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.

Baseline is meant to be called on CPU definitions created by libvirt in
either domain capabilities (this is the best way) or capabilities.
Neither of them will contain mode='host-passthrough'. And since no
 means the host CPU could not be detected, baseline should just
return an error. I don't see why s390x should be any special. This is
what happens if you try this in x86:

virsh hypervisor-cpu-baseline cpu-host.xml
error: invalid argument: no CPU model specified at index 0

All we need to do is to make sure we report the right error message.

Overloading baseline to perform model expansion is not needed as what
you suggest is already provided by domain capabilities.

> 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.

Host-passthrough CPU is by definition already restricted to what the
hypervisor is able to do so there's no need to ask baseline for the
result.

Jirka

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

2020-09-23 Thread 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 
>
> Before:
>   $ uname -m
>   s390x
>   $ cat passthrough-cpu.xml
>   
>   $ 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 
> ---
>  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, ) < 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 

 I'll wait some time for Collin to add Signed-of-by tag before pushing
 this.

>>>
>>> Signed-off-by: Collin Walling 
>>>
>>> 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  tag. Right now, virsh will either
>> report "an unknown error occurred" when the XML file contains a _single_
>>  element without a  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  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
> host in the result or even random result depending on the
> host performing the baseline API. This API should just reject any CPU
> definition without  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

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

2020-09-23 Thread Jiri Denemark 
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 
> >>>
> >>> Before:
> >>>   $ uname -m
> >>>   s390x
> >>>   $ cat passthrough-cpu.xml
> >>>   
> >>>   $ 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 
> >>> ---
> >>>  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, ) < 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 
> >>
> >> I'll wait some time for Collin to add Signed-of-by tag before pushing
> >> this.
> >>
> > 
> > Signed-off-by: Collin Walling 
> > 
> > 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  tag. Right now, virsh will either
> report "an unknown error occurred" when the XML file contains a _single_
>  element without a  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  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
host in the result or even random result depending on the
host performing the baseline API. This API should just reject any CPU
definition without  as invalid argument.

Jirka

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

2020-09-23 Thread Collin Walling 
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 
>>>
>>> Before:
>>>   $ uname -m
>>>   s390x
>>>   $ cat passthrough-cpu.xml
>>>   
>>>   $ 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 
>>> ---
>>>  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, ) < 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 
>>
>> I'll wait some time for Collin to add Signed-of-by tag before pushing
>> this.
>>
> 
> Signed-off-by: Collin Walling 
> 
> 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  tag. Right now, virsh will either
report "an unknown error occurred" when the XML file contains a _single_
 element without a  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  tag.

I can propose something to wrap the above in a helper function and
extend its use for baseline as well.

-- 
Regards,
Collin

Stay safe and stay healthy

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

2020-09-23 Thread Collin Walling 
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 
>>
>> Before:
>>   $ uname -m
>>   s390x
>>   $ cat passthrough-cpu.xml
>>   
>>   $ 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 
>> ---
>>  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, ) < 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 
> 
> I'll wait some time for Collin to add Signed-of-by tag before pushing
> this.
> 

Signed-off-by: Collin Walling 

Thanks!

-- 
Regards,
Collin

Stay safe and stay healthy

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

2020-09-23 Thread Jiri Denemark 
On Wed, Sep 23, 2020 at 09:26:58 +0200, Tim Wiederhake wrote:
> From: Collin Walling 
> 
> Before:
>   $ uname -m
>   s390x
>   $ cat passthrough-cpu.xml
>   
>   $ 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 
> ---
>  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, ) < 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 

I'll wait some time for Collin to add Signed-of-by tag before pushing
this.

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

2020-09-23 Thread Tim Wiederhake 
From: Collin Walling 

Before:
  $ uname -m
  s390x
  $ cat passthrough-cpu.xml
  
  $ 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 
---
 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, ) < 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);
-- 
2.26.2