Re: [Qemu-devel] [RFC PATCH] target/s390x/cpu_models: Set some additional feature bits for the "qemu" CPU

[David Hildenbrand <david@redhat.com>]

On 18.05.2017 03:55, Thomas Huth wrote:
> On 17.05.2017 18:49, David Hildenbrand wrote:
>> On 17.05.2017 17:35, Thomas Huth wrote:
>>> Currently we only present the plain z900 feature bits to the guest,
>>> but QEMU already emulates some additional features (but not all of
>>> the next CPU generation, so we can not use the next CPU level as
>>> default yet). Since newer Linux kernels are checking the feature bits
>>> and refuse to work if a required feature is missing, we should present
>>> as much of the supported features as possible when we are running
>>> with the default "qemu" CPU.
>>> This patch now adds the "stfle", "extended immediate" and "stckf"
>>> facility bits to the "qemu" CPU, which are already supported facilities.
>>> It is unfortunately still not enough to run e.g. recent Fedora kernels,
>>> but at least it's a first step into the right direction.
>>>
>>
>> Three things:
>>
>> 1. Should we care about backwards compatibility? I think so. This should
>> be fixed up using compat machine properties. (qemu model is a
>> migration-safe model and could e.g. be used in KVM setups, too).
> 
> Theoretically, I agree, but do we really care about backwards
> compatibility at this point in time? All major distro kernels (except
> Debian, I think) currently do not work in QEMU, so there is currently
> not that much that can be migrated...
> And currently, the "qemu" CPU is the very same as the "z900" CPU, so you
> might also get along with simply using "-cpu z900" on the destination
> instead, I guess.

If possible, I would like to avoid changing migration safe CPU model.
And I guess it shouldn't be too hard for now (unless we really change
the base model to e.g. a z9, then some more work might have to be done)

I think for now, setting "stfle=off" on "s390-cpu-qemu" for compat
machines should do the trick.

> 
>> 2. I would recommend to not enable STFLE for now. Why?
>>
>> It is/was an indication that the system is running on a z9 (and
>> implicitly has the basic features). This was not only done because
>> people were lazy, but because this bit was implicitly connected to other
>> machine properties.
> 
> Uh, that's ugly!
> 
>> One popular example is the "DAT-enhancement facility 2". It introduced
>> the "LOAD PAGE TABLE ENTRY ADDRESS" instruction, but no facility bit was
>> introduced. SO there is no way to check if the instruction is available
>> and actually working.
> 
> Does the Linux kernel use this instruction at all? I just grep'ed
> through the kernel sources and did not find it. If the Linux kernel does
> not use it, I think we should ignore this interdependency and just
> provide the STFLE feature bit to the guest - since recent Linux kernels
> depend on it.

Yes, current linux doesn't use it, I don't remember if previous versions
did. Most likely not. The question is if they relied on the stfle==z9
assumption. The STFLE facility really is special in that sense.

> 
>> Please note that we added a feature representation for this facility,
>> because this would allow us later on to at least model removal of such a
>> facility (if HW actually would drop it) on a CPU model level.
> 
> What about STFLE bit 78, according to my version of the POP, it says:
> 
> "The enhanced-DAT facility 2 is installed in the
>  z/Architecture architectural mode."
> 
> ?

As Aurelien already mentioned, there seemed to be different ways to
enhance DAT :) enhanced-DAT facility 2 is 2GB page support.

> 
>> 3. This introduces some inconsistency. s390x/cpu_models.c:set_feature()
>> explicitly tests for such inconsistencies.
>>
>> So your QEMU CPU model would have a feature, but you would not be able
>> to run that model using QEMU when manually specifying it on the command
>> line. Especially, expanding the "qemu" model and feeding it back to QEMU
>> will fail.
> 
> I've checked that I can also successfully disable the features again at
> the command line, using "-cpu qemu,eimm=false" for example, so not sure
> what exactly you're talking about here. Could you please elaborate?

Assume libvirt/the user expands the CPU model name "qemu" via
"qmp-expand-cpu-model "qemu", you will get something like

"z900-base,.....,stfle=on"

If you feed that to QEMU using "-cpu z900-base,...,stfle=on", QEMU will
detect the inconsistency when setting the property and abort. However,
"-cpu qemu" will succeed. Please note that these checks actually make
sense for KVM:

If you're on a z13 and configure a zEC12, you might be tempted to add
e.g. "vx=on". However, IBC (Instruction Blocking Control) in the machine
will block any attempt to execute a vx instruction. So these checks make
sure that only facilities really supported for a machine generation can
be enabled.

If we really want that, we might decide to drop such checks for models <
e.g. z9, because nobody will most likely care.

> 
>> So I am not sure if we should introduce such inconsistencies at that
>> point. Rather fix up the basics and then move the CPU model to a
>> consistent model.
> 
> I think we're very far away from being able to use the next official CPU
> model generation in QEMU TCG, so having at least something that let's us
> run other recent distro kernels apart from the Debian ones would be very
> helpful. I also understand the "qemu" CPU this way: "Simply give me the
> best CPU features that TCG currently can provide". If you want to have a
> "consistent" CPU state, you can simply use an official model like "z900"
> instead.

"qemu" is just like what "host" is for kvm. A consistent model, because
it is the default.

However, KVM folks also have the requirement to allow
"unfiltered"/"inconsistent" models, e.g. for development purposes.

There was the idea to introduce a CPU model "-cpu off", that would act
as before, without any CPU model support: Blindly enable anything we can.

This model would of course not be static, not migration-safe, and one
would not be able to modify features. This would map to an "unfiltered
qemu" model under TCG. We could blindly enable anything there.

> 
>  Thomas
> 


-- 

Thanks,

David