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

[Christian Borntraeger]

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)) {
>