qemu-s390x
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [qemu-s390x] [PATCH v1] s390x/cpu_models: Add "-cpu max" support


From: David Hildenbrand
Subject: Re: [qemu-s390x] [PATCH v1] s390x/cpu_models: Add "-cpu max" support
Date: Wed, 25 Jul 2018 19:50:21 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0

On 25.07.2018 19:09, Eduardo Habkost wrote:
> On Wed, Jul 25, 2018 at 11:12:33AM +0200, David Hildenbrand wrote:
>> The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like
>> a CPU with the maximum possible feature set when TCG is enabled.
>>
>> While the "host" model can not be used under TCG ("kvm_required"), the
>> "max" model can and "Enables all features supported by the accelerator in
>> the current host".
>>
>> So we can treat "host" just as a special case of "max" (like x86 does).
>> It differs to the "qemu" CPU model under TCG such that compatibility
>> handling will not be performed and that some experimental CPU features
>> not yet part of the "qemu" model might be indicated.
>>
>> These are right now under TCG (see "qemu_MAX"):
>> - stfle53
>> - msa5-base
>> - zpci
>>
>> This will result right now in the following warning when starting QEMU TCG
>> with the "max" model:
>>     "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'."
>>
>> The "qemu" model (used as default in QEMU under TCG) will continue to
>> work without such warnings. The "max" mdel in the current form
>> might be interesting for kvm-unit-tests (where we would e.g. now also
>> test "msa5-base").
>>
>> The "max" model is neither static nor migration safe (like the "host"
>> model). It is independent of the machine but dependends on the accelerator.
>> It can be used to detect the maximum CPU model also under TCG from upper
>> layers without having to care about CPU model names for CPU model
>> expansion.
>>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------
>>  1 file changed, 56 insertions(+), 25 deletions(-)
>>
>> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
>> index 604898a882..708bf0e3ba 100644
>> --- a/target/s390x/cpu_models.c
>> +++ b/target/s390x/cpu_models.c
>> @@ -307,7 +307,10 @@ static gint s390_cpu_list_compare(gconstpointer a, 
>> gconstpointer b)
>>      const char *name_a = object_class_get_name((ObjectClass *)a);
>>      const char *name_b = object_class_get_name((ObjectClass *)b);
>>  
>> -    /* move qemu and host to the top of the list, qemu first, host second */
>> +    /*
>> +     * Move qemu, host and max to the top of the list, qemu first, host 
>> second,
>> +     * max third.
>> +     */
>>      if (name_a[0] == 'q') {
>>          return -1;
>>      } else if (name_b[0] == 'q') {
>> @@ -316,6 +319,10 @@ static gint s390_cpu_list_compare(gconstpointer a, 
>> gconstpointer b)
>>          return -1;
>>      } else if (name_b[0] == 'h') {
>>          return 1;
>> +    } else if (name_a[0] == 'm') {
>> +        return -1;
>> +    } else if (name_b[0] == 'm') {
>> +        return 1;
>>      }
> 
> Isn't it simpler to add a S390CPUClass::ordering field?  See
> x86_cpu_list_compare() for an example.

Not sure if it is simpler, this way the whole sorting logic is located at a
single place.

But I agree that if this list grows bigger, we might want to use a
ordering field at least for the special cases (qemu/host/max/...)

> 
> 
>>  
>>      /* keep the same order we have in our table (sorted by release date) */
>> @@ -1077,27 +1084,6 @@ static void s390_cpu_model_initfn(Object *obj)
>>      }
>>  }
>>  
>> -#ifdef CONFIG_KVM
>> -static void s390_host_cpu_model_initfn(Object *obj)
>> -{
>> -    S390CPU *cpu = S390_CPU(obj);
>> -    Error *err = NULL;
>> -
>> -    if (!kvm_enabled() || !kvm_s390_cpu_models_supported()) {
>> -        return;
>> -    }
>> -
>> -    cpu->model = g_malloc0(sizeof(*cpu->model));
>> -    kvm_s390_get_host_cpu_model(cpu->model, &err);
>> -    if (err) {
>> -        error_report_err(err);
>> -        g_free(cpu->model);
>> -        /* fallback to unsupported cpu models */
>> -        cpu->model = NULL;
>> -    }
>> -}
>> -#endif
>> -
>>  static S390CPUDef s390_qemu_cpu_def;
>>  static S390CPUModel s390_qemu_cpu_model;
>>  
>> @@ -1136,6 +1122,30 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
>>      memcpy(cpu->model, &s390_qemu_cpu_model, sizeof(*cpu->model));
>>  }
>>  
>> +static void s390_max_cpu_model_initfn(Object *obj)
>> +{
>> +    const S390CPUModel *max_model;
>> +    S390CPU *cpu = S390_CPU(obj);
>> +    Error *local_err = NULL;
>> +
>> +    if (kvm_enabled() && !kvm_s390_cpu_models_supported()) {
>> +        /* "max" and "host" always work, even without CPU model support */
>> +        return;
>> +    }
> 
> What's the use case that requires this check to be here?
> 
> What do you expect 'query-cpu-model-expansion model=max' to
> return if !kvm_s390_cpu_models_supported()?

The same as for "host". We have been handling the host model like this forever.
(If we have no KVM interface, we have have basically no real idea on which CPU
 we are running, so we can't expand/baseline)

cpu_model_from_info():

if (!cpu->model) {
    error_setg(errp, "Details about the host CPU model are not available, "
               "it cannot be used.");
    object_unref(obj);
    return;
}

We might want to tweak this error message later maybe (use the
model name instead of "host", but as "max" maps to "host" under KVM,
this is not of high priority)

> 
> 
>> +
>> +    max_model = get_max_cpu_model(&local_err);
> 
> I've just confirmed that get_max_cpu_model() is already ready to
> work with TCG.

Yes, it was used for detecting "runability" already also for TCG.

> 
>> +    if (local_err) {
>> +        g_assert(kvm_enabled());
>> +        error_report_err(local_err);
>> +        /* fallback to unsupported CPU models */
>> +        return;
> 
> What about moving this outside instance_init?

To which place for example? We at least have to copy the CPU model
for each and every CPU instance (so we can modify it without side
effects using properties).

And if you look closely, 99% of this function will be exactly that
(as the max model is cached internally, it will only be looked up once).

> 
> On x86 we have a x86_cpu_expand_features() function to allow us
> to handle CPU model expansion errors more gracefully.
> 
> None of my comments are about new code, but existing code from
> "host", so:
> 
> Reviewed-by: Eduardo Habkost <address@hidden>

Thanks!

-- 

Thanks,

David / dhildenb



reply via email to

[Prev in Thread] Current Thread [Next in Thread]