Re: [Qemu-devel] [PATCH v2 1/6] target/s390x: Implement STORE FACILITIES LIST EXTENDED

[Richard Henderson]

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.

> 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.


r~