[v2] s390x/cpu_model: disallow unpack for --only-migratable

[PATCH v2] s390x/cpu_model: disallow unpack for --only-migratable

Posted by Christian Borntraeger 4 years, 9 months ago

secure execution (aka protected virtualization) guests cannot be
migrated at the moment. Disallow the unpack facility if the user
specifies --only-migratable.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
v1->v2:
- add missing return
- protect check with CONFIG_USER_ONLY for building non softmmu binaries

 target/s390x/cpu_models.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 35179f9dc7ba..e844a4007210 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -26,6 +26,7 @@
 #include "qapi/qmp/qdict.h"
 #ifndef CONFIG_USER_ONLY
 #include "sysemu/arch_init.h"
+#include "sysemu/sysemu.h"
 #include "hw/pci/pci.h"
 #endif
 #include "qapi/qapi-commands-machine-target.h"
@@ -878,6 +879,14 @@ static void check_compatibility(const S390CPUModel *max_model,
         return;
     }
 
+#ifndef CONFIG_USER_ONLY
+    if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) {
+        error_setg(errp, "The unpack facility is not compatible with "
+                   "the --only-migratable option");
+	return;
+    }
+#endif
+
     /* detect the missing features to properly report them */
     bitmap_andnot(missing, model->features, max_model->features, S390_FEAT_MAX);
     if (bitmap_empty(missing, S390_FEAT_MAX)) {
-- 
2.28.0

Re: [PATCH v2] s390x/cpu_model: disallow unpack for --only-migratable

Posted by no-reply@patchew.org 4 years, 9 months ago

Patchew URL: https://patchew.org/QEMU/20210125132238.179972-1-borntraeger@de.ibm.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210125132238.179972-1-borntraeger@de.ibm.com
Subject: [PATCH v2] s390x/cpu_model: disallow unpack for --only-migratable

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20210125125312.138491-1-borntraeger@de.ibm.com -> patchew/20210125125312.138491-1-borntraeger@de.ibm.com
 * [new tag]         patchew/20210125132238.179972-1-borntraeger@de.ibm.com -> patchew/20210125132238.179972-1-borntraeger@de.ibm.com
Switched to a new branch 'test'
9d55ad2 s390x/cpu_model: disallow unpack for --only-migratable

=== OUTPUT BEGIN ===
ERROR: code indent should never use tabs
#37: FILE: target/s390x/cpu_models.c:886:
+^Ireturn;$

total: 1 errors, 0 warnings, 21 lines checked

Commit 9d55ad23e018 (s390x/cpu_model: disallow unpack for --only-migratable) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210125132238.179972-1-borntraeger@de.ibm.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v2] s390x/cpu_model: disallow unpack for --only-migratable

Posted by Cornelia Huck 4 years, 9 months ago

On Mon, 25 Jan 2021 14:22:38 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> secure execution (aka protected virtualization) guests cannot be
> migrated at the moment. Disallow the unpack facility if the user
> specifies --only-migratable.

Maybe make the explanation a bit more verbose?

"Secure execution (aka protected virtualization) guests cannot be
migrated at the moment. If the unpack facility is provided in the cpu
model, a guest may choose to transition to secure mode, making the
guest unmigratable at that point in time. If the machine was explicitly
started with --only-migratable, we would get a failure only when the
guest actually tries to transition; instead, explicitly disallow the
unpack facility if --only-migratable was specified to avoid late
surprises."

> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> v1->v2:
> - add missing return
> - protect check with CONFIG_USER_ONLY for building non softmmu binaries
> 
>  target/s390x/cpu_models.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 35179f9dc7ba..e844a4007210 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -26,6 +26,7 @@
>  #include "qapi/qmp/qdict.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #include "hw/pci/pci.h"
>  #endif
>  #include "qapi/qapi-commands-machine-target.h"
> @@ -878,6 +879,14 @@ static void check_compatibility(const S390CPUModel *max_model,
>          return;
>      }
>  
> +#ifndef CONFIG_USER_ONLY
> +    if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) {
> +        error_setg(errp, "The unpack facility is not compatible with "
> +                   "the --only-migratable option");

Might be a bit surprising if the host model had been specified... is
there a way to add a hint how to get rid of the unpack bit?

> +	return;
> +    }
> +#endif
> +
>      /* detect the missing features to properly report them */
>      bitmap_andnot(missing, model->features, max_model->features, S390_FEAT_MAX);
>      if (bitmap_empty(missing, S390_FEAT_MAX)) {

Re: [PATCH v2] s390x/cpu_model: disallow unpack for --only-migratable

Posted by Christian Borntraeger 4 years, 9 months ago


On 25.01.21 14:38, Cornelia Huck wrote:
> On Mon, 25 Jan 2021 14:22:38 +0100
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> secure execution (aka protected virtualization) guests cannot be
>> migrated at the moment. Disallow the unpack facility if the user
>> specifies --only-migratable.
> 
> Maybe make the explanation a bit more verbose?
> 
> "Secure execution (aka protected virtualization) guests cannot be
> migrated at the moment. If the unpack facility is provided in the cpu
> model, a guest may choose to transition to secure mode, making the
> guest unmigratable at that point in time. If the machine was explicitly
> started with --only-migratable, we would get a failure only when the
> guest actually tries to transition; instead, explicitly disallow the
> unpack facility if --only-migratable was specified to avoid late
> surprises."
> 
>>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>> v1->v2:
>> - add missing return
>> - protect check with CONFIG_USER_ONLY for building non softmmu binaries
>>
>>  target/s390x/cpu_models.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 35179f9dc7ba..e844a4007210 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -26,6 +26,7 @@
>>  #include "qapi/qmp/qdict.h"
>>  #ifndef CONFIG_USER_ONLY
>>  #include "sysemu/arch_init.h"
>> +#include "sysemu/sysemu.h"
>>  #include "hw/pci/pci.h"
>>  #endif
>>  #include "qapi/qapi-commands-machine-target.h"
>> @@ -878,6 +879,14 @@ static void check_compatibility(const S390CPUModel *max_model,
>>          return;
>>      }
>>  
>> +#ifndef CONFIG_USER_ONLY
>> +    if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) {
>> +        error_setg(errp, "The unpack facility is not compatible with "
>> +                   "the --only-migratable option");
> 
> Might be a bit surprising if the host model had been specified... is
> there a way to add a hint how to get rid of the unpack bit?

I can try to make the error message a bit more verbose. 
> 
>> +	return;
>> +    }
>> +#endif
>> +
>>      /* detect the missing features to properly report them */
>>      bitmap_andnot(missing, model->features, max_model->features, S390_FEAT_MAX);
>>      if (bitmap_empty(missing, S390_FEAT_MAX)) {
>

Re: [PATCH v2] s390x/cpu_model: disallow unpack for --only-migratable

Posted by David Hildenbrand 4 years, 9 months ago

On 25.01.21 14:22, Christian Borntraeger wrote:
> secure execution (aka protected virtualization) guests cannot be
> migrated at the moment. Disallow the unpack facility if the user
> specifies --only-migratable.
> 
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> v1->v2:
> - add missing return
> - protect check with CONFIG_USER_ONLY for building non softmmu binaries
> 
>  target/s390x/cpu_models.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 35179f9dc7ba..e844a4007210 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -26,6 +26,7 @@
>  #include "qapi/qmp/qdict.h"
>  #ifndef CONFIG_USER_ONLY
>  #include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
>  #include "hw/pci/pci.h"
>  #endif
>  #include "qapi/qapi-commands-machine-target.h"
> @@ -878,6 +879,14 @@ static void check_compatibility(const S390CPUModel *max_model,
>          return;
>      }
>  
> +#ifndef CONFIG_USER_ONLY
> +    if (only_migratable && test_bit(S390_FEAT_UNPACK, model->features)) {
> +        error_setg(errp, "The unpack facility is not compatible with "
> +                   "the --only-migratable option");
> +	return;
> +    }
> +#endif
> +
>      /* detect the missing features to properly report them */
>      bitmap_andnot(missing, model->features, max_model->features, S390_FEAT_MAX);
>      if (bitmap_empty(missing, S390_FEAT_MAX)) {
> 

BTW, I wonder if the right place for this would be apply_cpu_model().

Anyhow

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb