[PATCH v1] qemu: monitor: substitute missing model name for host-passthrough

Collin Walling posted 1 patch 3 years, 9 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20200625215846.30757-1-walling@linux.ibm.com
src/qemu/qemu_monitor_json.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
[PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Collin Walling 3 years, 9 months ago
When executing the hypervisor-cpu-compare/baseline commands and
the XML file contains a CPU definition using host-passthrough
and no model name, the commands will fail and return an error
message from the QMP response.

Let's fix this by checking for host-passthrough and a missing
model name when converting a CPU definition to a JSON object.
If these conditions are matched, then the JSON object will be
constructed using "host" as the model name.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680

Signed-off-by: Collin Walling <walling@linux.ibm.com>
---
 src/qemu/qemu_monitor_json.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 3070c1e6b3..448a3a9356 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
 {
     virJSONValuePtr model = virJSONValueNewObject();
     virJSONValuePtr props = NULL;
+    const char *model_name = cpu->model;
     size_t i;
 
-    if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
+    if (!model_name) {
+        if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
+            model_name = "host";
+        } else {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("cpu parameter is missing a model name"));
+            goto error;
+        }
+    }
+
+    if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
         goto error;
 
     if (cpu->nfeatures || !migratable) {
-- 
2.26.2

Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Peter Krempa 3 years, 9 months ago
On Thu, Jun 25, 2020 at 17:58:46 -0400, Collin Walling wrote:
> When executing the hypervisor-cpu-compare/baseline commands and
> the XML file contains a CPU definition using host-passthrough
> and no model name, the commands will fail and return an error
> message from the QMP response.

This kind of logic does not seem to belong ...

> Let's fix this by checking for host-passthrough and a missing
> model name when converting a CPU definition to a JSON object.
> If these conditions are matched, then the JSON object will be
> constructed using "host" as the model name.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680
> 
> Signed-off-by: Collin Walling <walling@linux.ibm.com>
> ---
>  src/qemu/qemu_monitor_json.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 3070c1e6b3..448a3a9356 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
>  {
>      virJSONValuePtr model = virJSONValueNewObject();
>      virJSONValuePtr props = NULL;
> +    const char *model_name = cpu->model;
>      size_t i;
>  
> -    if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
> +    if (!model_name) {
> +        if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
> +            model_name = "host";
> +        } else {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("cpu parameter is missing a model name"));
> +            goto error;
> +        }
> +    }

... to the monitor code, where we try to just deal with the monitor
itself rather than hiding any logic.

But the final word needs to be from Jirka, but he is on holidays until
the end of next week.


> +
> +    if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
>          goto error;
>  
>      if (cpu->nfeatures || !migratable) {
> -- 
> 2.26.2
> 

Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Collin Walling 3 years, 9 months ago
On 6/26/20 3:22 AM, Peter Krempa wrote:
> On Thu, Jun 25, 2020 at 17:58:46 -0400, Collin Walling wrote:
>> When executing the hypervisor-cpu-compare/baseline commands and
>> the XML file contains a CPU definition using host-passthrough
>> and no model name, the commands will fail and return an error
>> message from the QMP response.
> 
> This kind of logic does not seem to belong ...
> 
>> Let's fix this by checking for host-passthrough and a missing
>> model name when converting a CPU definition to a JSON object.
>> If these conditions are matched, then the JSON object will be
>> constructed using "host" as the model name.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680
>>
>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>> ---
>>  src/qemu/qemu_monitor_json.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index 3070c1e6b3..448a3a9356 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
>>  {
>>      virJSONValuePtr model = virJSONValueNewObject();
>>      virJSONValuePtr props = NULL;
>> +    const char *model_name = cpu->model;
>>      size_t i;
>>  
>> -    if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
>> +    if (!model_name) {
>> +        if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
>> +            model_name = "host";
>> +        } else {
>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                           _("cpu parameter is missing a model name"));
>> +            goto error;
>> +        }
>> +    }
> 
> ... to the monitor code, where we try to just deal with the monitor
> itself rather than hiding any logic.
> 

That's fair. Perhaps it should belong to the qemu driver code after the
CPU XML to a CPU def conversion. I'm also unsure if "host" is an s390
specific thing, or if other archs use it as well.

> But the final word needs to be from Jirka, but he is on holidays until
> the end of next week.
> 
> 
>> +
>> +    if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
>>          goto error;
>>  
>>      if (cpu->nfeatures || !migratable) {
>> -- 
>> 2.26.2
>>
> 


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Collin Walling 3 years, 7 months ago
Polite ping to the mailing list regarding this bug. I recall the logic I
proposed belongs elsewhere, but I'd like to kindly request a followup
from the respective maintainer(s) for a direction.

Thanks!

On 6/26/20 2:31 PM, Collin Walling wrote:
> On 6/26/20 3:22 AM, Peter Krempa wrote:
>> On Thu, Jun 25, 2020 at 17:58:46 -0400, Collin Walling wrote:
>>> When executing the hypervisor-cpu-compare/baseline commands and
>>> the XML file contains a CPU definition using host-passthrough
>>> and no model name, the commands will fail and return an error
>>> message from the QMP response.
>>
>> This kind of logic does not seem to belong ...
>>
>>> Let's fix this by checking for host-passthrough and a missing
>>> model name when converting a CPU definition to a JSON object.
>>> If these conditions are matched, then the JSON object will be
>>> constructed using "host" as the model name.
>>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680
>>>
>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>> ---
>>>  src/qemu/qemu_monitor_json.c | 13 ++++++++++++-
>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>>> index 3070c1e6b3..448a3a9356 100644
>>> --- a/src/qemu/qemu_monitor_json.c
>>> +++ b/src/qemu/qemu_monitor_json.c
>>> @@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
>>>  {
>>>      virJSONValuePtr model = virJSONValueNewObject();
>>>      virJSONValuePtr props = NULL;
>>> +    const char *model_name = cpu->model;
>>>      size_t i;
>>>  
>>> -    if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
>>> +    if (!model_name) {
>>> +        if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
>>> +            model_name = "host";
>>> +        } else {
>>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>> +                           _("cpu parameter is missing a model name"));
>>> +            goto error;
>>> +        }
>>> +    }
>>
>> ... to the monitor code, where we try to just deal with the monitor
>> itself rather than hiding any logic.
>>
> 
> That's fair. Perhaps it should belong to the qemu driver code after the
> CPU XML to a CPU def conversion. I'm also unsure if "host" is an s390
> specific thing, or if other archs use it as well.
> 
>> But the final word needs to be from Jirka, but he is on holidays until
>> the end of next week.
>>
>>
>>> +
>>> +    if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
>>>          goto error;
>>>  
>>>      if (cpu->nfeatures || !migratable) {
>>> -- 
>>> 2.26.2
>>>
>>
> 
> 


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Collin Walling 3 years, 6 months ago
One more ping in attempt to get this in the right direction. Otherwise
I'll post my next idea and we can go from there :)

Thinking about this issue, should a host-passthough CPU definition be
permitted for the baseline & comparison commands? The model represented
under this mode is not considered migration safe and it may make sense
to simply fail early since these commands aim to construct/determine a
migratable CPU model, respectively.

Perhaps if a host-passthrough CPU is detected, then an error reporting
something along the lines of "host-passthrough is not supported for
migration" would be sufficient for this?

Thanks again. Hope all is well.

On 8/19/20 11:05 AM, Collin Walling wrote:
> Polite ping to the mailing list regarding this bug. I recall the logic I
> proposed belongs elsewhere, but I'd like to kindly request a followup
> from the respective maintainer(s) for a direction.
> 
> Thanks!
> 
> On 6/26/20 2:31 PM, Collin Walling wrote:
>> On 6/26/20 3:22 AM, Peter Krempa wrote:
>>> On Thu, Jun 25, 2020 at 17:58:46 -0400, Collin Walling wrote:
>>>> When executing the hypervisor-cpu-compare/baseline commands and
>>>> the XML file contains a CPU definition using host-passthrough
>>>> and no model name, the commands will fail and return an error
>>>> message from the QMP response.
>>>
>>> This kind of logic does not seem to belong ...
>>>
>>>> Let's fix this by checking for host-passthrough and a missing
>>>> model name when converting a CPU definition to a JSON object.
>>>> If these conditions are matched, then the JSON object will be
>>>> constructed using "host" as the model name.
>>>>
>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1850680
>>>>
>>>> Signed-off-by: Collin Walling <walling@linux.ibm.com>
>>>> ---
>>>>  src/qemu/qemu_monitor_json.c | 13 ++++++++++++-
>>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>>>> index 3070c1e6b3..448a3a9356 100644
>>>> --- a/src/qemu/qemu_monitor_json.c
>>>> +++ b/src/qemu/qemu_monitor_json.c
>>>> @@ -5804,9 +5804,20 @@ qemuMonitorJSONMakeCPUModel(virCPUDefPtr cpu,
>>>>  {
>>>>      virJSONValuePtr model = virJSONValueNewObject();
>>>>      virJSONValuePtr props = NULL;
>>>> +    const char *model_name = cpu->model;
>>>>      size_t i;
>>>>  
>>>> -    if (virJSONValueObjectAppendString(model, "name", cpu->model) < 0)
>>>> +    if (!model_name) {
>>>> +        if (cpu->mode == VIR_CPU_MODE_HOST_PASSTHROUGH) {
>>>> +            model_name = "host";
>>>> +        } else {
>>>> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>>> +                           _("cpu parameter is missing a model name"));
>>>> +            goto error;
>>>> +        }
>>>> +    }
>>>
>>> ... to the monitor code, where we try to just deal with the monitor
>>> itself rather than hiding any logic.
>>>
>>
>> That's fair. Perhaps it should belong to the qemu driver code after the
>> CPU XML to a CPU def conversion. I'm also unsure if "host" is an s390
>> specific thing, or if other archs use it as well.
>>
>>> But the final word needs to be from Jirka, but he is on holidays until
>>> the end of next week.
>>>
>>>
>>>> +
>>>> +    if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
>>>>          goto error;
>>>>  
>>>>      if (cpu->nfeatures || !migratable) {
>>>> -- 
>>>> 2.26.2
>>>>
>>>
>>
>>
> 
>

Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Michal Privoznik 3 years, 6 months ago
On 9/15/20 10:25 PM, Collin Walling wrote:
> One more ping in attempt to get this in the right direction. Otherwise
> I'll post my next idea and we can go from there :)

I agree with Peter that while the idea might look correct it's too deep.

> 
> Thinking about this issue, should a host-passthough CPU definition be
> permitted for the baseline & comparison commands? The model represented
> under this mode is not considered migration safe and it may make sense
> to simply fail early since these commands aim to construct/determine a
> migratable CPU model, respectively.

Honestly, I don't know much about this CPU models area, but is that true 
even for two identical hosts? Say I have two desktops next to each 
other, with the same CPU and I want to migrate. I could use host model, 
couldn't I?

> 
> Perhaps if a host-passthrough CPU is detected, then an error reporting
> something along the lines of "host-passthrough is not supported for
> migration" would be sufficient for this?
> 
> Thanks again. Hope all is well.

Michal

Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Collin Walling 3 years, 6 months ago
On 9/16/20 3:03 AM, Michal Privoznik wrote:
> On 9/15/20 10:25 PM, Collin Walling wrote:
>> One more ping in attempt to get this in the right direction. Otherwise
>> I'll post my next idea and we can go from there :)
> 
> I agree with Peter that while the idea might look correct it's too deep.
> 
>>
>> Thinking about this issue, should a host-passthough CPU definition be
>> permitted for the baseline & comparison commands? The model represented
>> under this mode is not considered migration safe and it may make sense
>> to simply fail early since these commands aim to construct/determine a
>> migratable CPU model, respectively.
> 
> Honestly, I don't know much about this CPU models area, but is that true
> even for two identical hosts? Say I have two desktops next to each
> other, with the same CPU and I want to migrate. I could use host model,
> couldn't I?
> 

"Host-model" is an alias for a CPU model that closely represents the
capabilities of the host machine (on s390, because this model is defined
by the hypervisor, it can also be called the "hypervisor CPU model" --
not an important detail).

However, a guest running with the host-passthrough mode is not
considered migration safe as that guest may covertly run with
features/capabilities that are not directly exposed to the hypervisor.

>From what I understand regarding the hypervisor-cpu-compare and
hypervisor-cpu-baseline commands is that they aim to assist with
determining the migratability of guests based on their CPU model and
feature set (usually along with a host CPU in the equation as well).

When the user passes an XML file to the hypervisor-cpu-compare and
hypervisor-cpu-baseline (sorry, forgot to be specific with the commands
here), AND the XML file contains a CPU definition using
mode='host-passthrough', then the commands should mention that this CPU
model is not migratable because the CPU definition using
host-passthrough is not migration safe.

>>
>> Perhaps if a host-passthrough CPU is detected, then an error reporting
>> something along the lines of "host-passthrough is not supported for
>> migration" would be sufficient for this?
>>
>> Thanks again. Hope all is well.
> 
> Michal
> 


-- 
Regards,
Collin

Stay safe and stay healthy

Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Jiri Denemark 3 years, 6 months ago
Collin, I apologize for not getting to you earlier.

On Wed, Sep 16, 2020 at 12:11:08 -0400, Collin Walling wrote:
> On 9/16/20 3:03 AM, Michal Privoznik wrote:
> > On 9/15/20 10:25 PM, Collin Walling wrote:
> >> One more ping in attempt to get this in the right direction. Otherwise
> >> I'll post my next idea and we can go from there :)
> > 
> > I agree with Peter that while the idea might look correct it's too deep.
> > 
> >>
> >> Thinking about this issue, should a host-passthough CPU definition be
> >> permitted for the baseline & comparison commands? The model represented
> >> under this mode is not considered migration safe and it may make sense
> >> to simply fail early since these commands aim to construct/determine a
> >> migratable CPU model, respectively.
> > 
> > Honestly, I don't know much about this CPU models area, but is that true
> > even for two identical hosts? Say I have two desktops next to each
> > other, with the same CPU and I want to migrate. I could use host model,
> > couldn't I?
> > 
> 
> "Host-model" is an alias for a CPU model that closely represents the
> capabilities of the host machine (on s390, because this model is defined
> by the hypervisor, it can also be called the "hypervisor CPU model" --
> not an important detail).
> 
> However, a guest running with the host-passthrough mode is not
> considered migration safe as that guest may covertly run with
> features/capabilities that are not directly exposed to the hypervisor.

Right, but migration may still be possible and working fine if both host
are identical.

> From what I understand regarding the hypervisor-cpu-compare and
> hypervisor-cpu-baseline commands is that they aim to assist with
> determining the migratability of guests based on their CPU model and
> feature set (usually along with a host CPU in the equation as well).

Baseline with a host-passthrough CPU is not indeed very useful, but
compare could still be used and its usage is not limited to migration.
For example, you can use it to check whether a domain with a guest CPU
configuration can be started on a specific host before you actually try
to start it. And reporting host-passthrough as incompatible would be
wrong.

Anyway, thanks for your patch, it was mostly correct, it just needed to
be done a bit higher in the call graph. Incidentally, Tim Wiederhake [1]
took this original patch and moved the change to the right place. The
authorship is still yours, so if you want to append you signed-off-by
tag there, I'll wait a bit before pushing Tim's patch.

Jirka

[1] https://www.redhat.com/archives/libvir-list/2020-September/msg01177.html

Re: [PATCH v1] qemu: monitor: substitute missing model name for host-passthrough
Posted by Collin Walling 3 years, 6 months ago
On 9/23/20 9:50 AM, Jiri Denemark wrote:
> Collin, I apologize for not getting to you earlier.
> 
> On Wed, Sep 16, 2020 at 12:11:08 -0400, Collin Walling wrote:
>> On 9/16/20 3:03 AM, Michal Privoznik wrote:
>>> On 9/15/20 10:25 PM, Collin Walling wrote:
>>>> One more ping in attempt to get this in the right direction. Otherwise
>>>> I'll post my next idea and we can go from there :)
>>>
>>> I agree with Peter that while the idea might look correct it's too deep.
>>>
>>>>
>>>> Thinking about this issue, should a host-passthough CPU definition be
>>>> permitted for the baseline & comparison commands? The model represented
>>>> under this mode is not considered migration safe and it may make sense
>>>> to simply fail early since these commands aim to construct/determine a
>>>> migratable CPU model, respectively.
>>>
>>> Honestly, I don't know much about this CPU models area, but is that true
>>> even for two identical hosts? Say I have two desktops next to each
>>> other, with the same CPU and I want to migrate. I could use host model,
>>> couldn't I?
>>>
>>
>> "Host-model" is an alias for a CPU model that closely represents the
>> capabilities of the host machine (on s390, because this model is defined
>> by the hypervisor, it can also be called the "hypervisor CPU model" --
>> not an important detail).
>>
>> However, a guest running with the host-passthrough mode is not
>> considered migration safe as that guest may covertly run with
>> features/capabilities that are not directly exposed to the hypervisor.
> 
> Right, but migration may still be possible and working fine if both host
> are identical.
> 
>> From what I understand regarding the hypervisor-cpu-compare and
>> hypervisor-cpu-baseline commands is that they aim to assist with
>> determining the migratability of guests based on their CPU model and
>> feature set (usually along with a host CPU in the equation as well).
> 
> Baseline with a host-passthrough CPU is not indeed very useful, but

Agreed, but I think baseline would still benefit from the error catching
that is proposed in the CPU comparison patch (I continue the
conversation over on that thread).

> compare could still be used and its usage is not limited to migration.
> For example, you can use it to check whether a domain with a guest CPU
> configuration can be started on a specific host before you actually try
> to start it. And reporting host-passthrough as incompatible would be
> wrong.
> 
> Anyway, thanks for your patch, it was mostly correct, it just needed to
> be done a bit higher in the call graph. Incidentally, Tim Wiederhake [1]
> took this original patch and moved the change to the right place. The
> authorship is still yours, so if you want to append you signed-off-by
> tag there, I'll wait a bit before pushing Tim's patch.

Thanks. Gave my sign-off.

> 
> Jirka
> 
> [1] https://www.redhat.com/archives/libvir-list/2020-September/msg01177.html
> 




-- 
Regards,
Collin

Stay safe and stay healthy