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

[David Hildenbrand]

On 25.07.2018 22:14, Eduardo Habkost wrote:
> On Wed, Jul 25, 2018 at 07:50:21PM +0200, David Hildenbrand wrote:
>> On 25.07.2018 19:09, Eduardo Habkost wrote:
> [...]
>>>> +    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).
> 
> To any code that will look at cpu->model.
> 
> You are wrapping an interface that needs to report errors
> (kvm_s390_get_host_cpu_model()) behind an interface that is not
> able to report errors (object_new()).  There's nothing that
> requires you to do that, does it?  You are free to provide an API
> that is actually able to report errors, instead of relying on
> object_new() only.

I see what you mean. One solution would be to preload and store the
model somewhere globally (not locally). So in the init function, we
would not have to handle errors.

But I am not even sure where we could do such a global initialization +
be able to report errors easily. I remember that we had a hard time to
get this running smoothly due to the dependency of
kvm_s390_get_host_cpu_model() on:
- accelerator
- machine
- KVM init state

And initializing cpu->model in realize() is too late, because all
properties have to access it. Even a pre_plug handler will not work.


On the other hand, I decided to ignore all errors back then and fallback
to the "host CPU model unknown" case, because there are some corner
cases where we still want to allow running the "host" model even though
there was a problem detecting it.

So my summary would be: We ignore errors (and rather treat them like
warnings) for a reason here and fallback to "unsupported CPU models",
which allows to run + use QEMU even in environments where our CPU model
detection fails (e.g. on a very strange new CPU model we could have in
the future).

Especially "!cpu->model" does not imply that there was an error. It
includes disabled CPU model support or unavailable CPU model support
(KVM), which is perfectly fine. Replicating initialization attempts at
all places where we access "cpu->model" does therefore not sound 100%
clean to me and most likely makes the code way more complicated.

Right now the semantics are clear: if we have "!cpu->model" after the
object has been created, details about the host CPU model are not
available (models unavailable/unsupported). Modifying properties,
baselining, expanding is not possible with that model then. But it can
be used for execution.

> 
> But I understand that the QOM/qdev API doesn't make that job
> easy.  On x86 we have X86CPU::max_features and
> X86CPU::user_features because of that.
> 


-- 

Thanks,

David / dhildenb