[v2,1/6] target/s390x: Implement STORE FACILITIES LIST EXTENDED

On 2017-05-09 07:51, Richard Henderson wrote:
> On 05/09/2017 01:14 AM, Aurelien Jarno wrote:
> > > +/* The maximum bit defined at the moment is 129.  */
> > > +#define MAX_STFL_WORDS  3
> > 
> > Could it be computed from S390_FEAT_MAX? in gen-features.c,
> > S390_FEAT_MAX / 64 + 1 is used.
> 
> No, because the features list in cpu_features_def.h bears no relation to the
> facilities bit index (as seen in the third column of s390_features
> structure).
> 
> > Is there a reason to not use s390_fill_feat_block here?
> 
> Yes.  I used s390_fill_feat_block in the v1 patch and changed this in
> response to review from David Hildenbrand.
> 
> Using s390_fill_feat_block isn't completely trivial.  It stores into a
> big-endian uint8_t array, which means that (for a little-endian host) we
> have to be careful how we copy that data to the guest, lest we inadvertently
> byte swap again.
> 
> Here in v2 I store into a host-endian uint64_t array, which makes the
> ultimate users of this helper more straightforward.

Ok, thanks for the detailed explanations. Then I guess you should fold
the following patch to correctly set the zArch active bit as done in
s390_fill_feat_block:


> > Otherwise it looks fine to me. It's probably a discussion for later
> > patches, but should we also implement a TCG feature mask, like for
> > example on PowerPC? Currently the only allowed CPU for TCG is z900,
> > which makes this code almost useless. And while QEMU implements many
> > features from newer CPU, some of them are missing and we don't want
> > them to appear in the feature list.
> 
> This is complicated by the fact that for a long time, the kernel checked for
> an exact match of facilities present.
> 
> From linux 3.13 arch/s390/kernel/head.S:
> 
> # List of facilities that are required. If not all facilities are present
> # the kernel will crash. Format is number of facility words with bits set,
> # followed by the facility words.
> 
> #if defined(CONFIG_64BIT)
> #if defined(CONFIG_MARCH_ZEC12)
>         .long 3, 0xc100efe3, 0xf46ce800, 0x00400000
> #elif defined(CONFIG_MARCH_Z196)
>         .long 2, 0xc100efe3, 0xf46c0000
> #elif defined(CONFIG_MARCH_Z10)
>         .long 2, 0xc100efe3, 0xf0680000
> #elif defined(CONFIG_MARCH_Z9_109)
>         .long 1, 0xc100efc3
> #elif defined(CONFIG_MARCH_Z990)
>         .long 1, 0xc0002000
> #elif defined(CONFIG_MARCH_Z900)
>         .long 1, 0xc0000000
> #endif
> 
> I'm pleased to see this appears to have been dropped at some point; I cannot
> see it present in linux 4.11.  However, this still applies to the kernels
> supplied by the shipping distributions.
> 
> Which means that we cannot mask *anything* from TCG, including the useless
> stuff like hexadecimal floating point, and still have the kernel boot.  So
> in practice a feature mask for TCG will be not only useless but actively
> harmful.

That's true if you want to boot an optimized kernel. That said you might
want to boot a z900 kernel on a more recent machine and still have the
userland check for some facilities using the STFLE instructions. Right
now the choice is limited to the z900 machine. As soon as you try to
use a newer CPU like the z990, the DAT-enhancement facility bit is set
in STFL, and the kernel try to use the idte instruction which is not
emulated by QEMU. OTOH, it's possible to boot a z900 kernel with a -cpu
z990,dateh=off.

Aurelien

On 05/09/2017 08:17 AM, Aurelien Jarno wrote:
> Ok, thanks for the detailed explanations. Then I guess you should fold
> the following patch to correctly set the zArch active bit as done in
> s390_fill_feat_block:
> 
> --- a/target/s390x/misc_helper.c
> +++ b/target/s390x/misc_helper.c
> @@ -693,6 +693,11 @@ static unsigned do_stfle(CPUS390XState *env, uint64_t words[MAX_STFL_WORDS])
>   
>       memset(words, 0, sizeof(uint64_t) * MAX_STFL_WORDS);
>   
> +    if (test_bit(S390_FEAT_ZARCH, features)) {
> +        /* z/Architecture is always active if around */
> +        words[0] |= 1ull << 61;
> +    }

Oops, yes, good catch.

>> Which means that we cannot mask *anything* from TCG, including the useless
>> stuff like hexadecimal floating point, and still have the kernel boot.  So
>> in practice a feature mask for TCG will be not only useless but actively
>> harmful.
> 
> That's true if you want to boot an optimized kernel.

Well, yes.  But the last time I was really looking at this (2 years ago?) 
Fedora was built to use z9-109.  I guess maybe Debian is the only distribution 
that doesn't force some later cpu?

> As soon as you try to
> use a newer CPU like the z990, the DAT-enhancement facility bit is set
> in STFL, and the kernel try to use the idte instruction which is not
> emulated by QEMU.

Good to know.  Thanks,


r~