Re: [Qemu-devel] [PATCH v1 5/6] target/s390x: add basic MSA features

2017-08-09 Thread Cornelia Huck
On Wed, 9 Aug 2017 15:25:36 +0200
David Hildenbrand  wrote:

> > A comment which subfunction this is might be helpful.  
> 
> Indeed.
> 
> /* query subfunction */

Looks good!

> 
> >   
> >> +for (i = 0; i < 16; i++) {
> >> +param_addr = wrap_address(env, env->regs[1] + i);  
> > 
> > This does not compile for me (after massaging the Makefile above), as
> > wrap_address does not seem to be exported... can you do a respin,
> > please?
> >   
> 
> Contained in patch nr4 in this series. But I'm planning to do a respin
> (most likely introducing internal.h).

Ah yes, that issue had dropped out of my cache :)

I'll just wait for a respin of your pending patches, then.



Re: [Qemu-devel] [PATCH v1 5/6] target/s390x: add basic MSA features

2017-08-09 Thread David Hildenbrand

>> @@ -792,6 +792,7 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
>> fbm)
>>  S390_FEAT_STFLE,
>>  S390_FEAT_EXTENDED_IMMEDIATE,
>>  S390_FEAT_EXTENDED_TRANSLATION_2,
>> +S390_FEAT_MSA,
>>  S390_FEAT_EXTENDED_TRANSLATION_3,
>>  S390_FEAT_LONG_DISPLACEMENT,
>>  S390_FEAT_LONG_DISPLACEMENT_FAST,
>> @@ -808,6 +809,9 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
>> fbm)
>>  S390_FEAT_STFLE_49,
>>  S390_FEAT_LOCAL_TLB_CLEARING,
>>  S390_FEAT_STFLE_53,
>> +S390_FEAT_MSA_EXT_5,
>> +S390_FEAT_MSA_EXT_3,
>> +S390_FEAT_MSA_EXT_4,
> 
> I first thought that this looks weird, but it is the actual sequence of
> the facility bits (probably the bit for MSA_EXT_5 has been reused?)

Probably, or they had it reserved for something else. Another reason
could be the non-hypervisor managed vs. hypervisor managed stuff (didn't
check the facility bit numbers, but this could be a reason).

[...]
> 
> A comment which subfunction this is might be helpful.

Indeed.

/* query subfunction */

> 
>> +for (i = 0; i < 16; i++) {
>> +param_addr = wrap_address(env, env->regs[1] + i);
> 
> This does not compile for me (after massaging the Makefile above), as
> wrap_address does not seem to be exported... can you do a respin,
> please?
> 

Contained in patch nr4 in this series. But I'm planning to do a respin
(most likely introducing internal.h).

>> +cpu_stb_data_ra(env, param_addr, subfunc[i], ra);
>> +}
>> +break;
>> +default:
>> +/* we don't implement any other subfunction yet */
>> +g_assert_not_reached();
>> +}
>> +
>> +return 0;
>> +}

Thanks!

-- 

Thanks,

David



Re: [Qemu-devel] [PATCH v1 5/6] target/s390x: add basic MSA features

2017-08-09 Thread Thomas Huth
On 09.08.2017 15:01, Cornelia Huck wrote:
> On Fri, 21 Jul 2017 14:56:08 +0200
> David Hildenbrand  wrote:
> 
> Finally got around to that one...
> 
>> The STFLE bits for the MSA (extension) facilities simply indicate that
>> the respective instructions can be executed. The QUERY subfunction can then
>> be used to identify which features exactly are available.
>>
>> Availability of subfunctions can also vary on real hardware. For now, we
>> simply implement a CPU model without any available subfunctions except
>> QUERY (which is always around).
>>
>> As all MSA functions behave quite similarly, we can use one translation
>> handler for now. Prepare the code for implementation of actual subfunctions.
> 
> Sounds reasonable.
> 
>>
>> At least MSA is helpful for now, as older Linux kernels require this
>> facility when compiled for a z9 model. Allow to enable the facilities
>> for the qemu cpu model.
> 
> Do you remember which kernels?

Basically everything before kernel version 4.13 - the patch to lift this
requirement has just been merged very recently:

 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8aa8680aa383bf6e2ac2a7d3369097268c75f7da

 Thomas



Re: [Qemu-devel] [PATCH v1 5/6] target/s390x: add basic MSA features

2017-08-09 Thread Cornelia Huck
On Fri, 21 Jul 2017 14:56:08 +0200
David Hildenbrand  wrote:

Finally got around to that one...

> The STFLE bits for the MSA (extension) facilities simply indicate that
> the respective instructions can be executed. The QUERY subfunction can then
> be used to identify which features exactly are available.
> 
> Availability of subfunctions can also vary on real hardware. For now, we
> simply implement a CPU model without any available subfunctions except
> QUERY (which is always around).
> 
> As all MSA functions behave quite similarly, we can use one translation
> handler for now. Prepare the code for implementation of actual subfunctions.

Sounds reasonable.

> 
> At least MSA is helpful for now, as older Linux kernels require this
> facility when compiled for a z9 model. Allow to enable the facilities
> for the qemu cpu model.

Do you remember which kernels?

> 
> Signed-off-by: David Hildenbrand 
> ---
>  target/s390x/Makefile.objs   |  2 +-
>  target/s390x/cpu_models.c|  4 +++
>  target/s390x/crypto_helper.c | 65 
> 
>  target/s390x/helper.h|  1 +
>  target/s390x/insn-data.def   | 13 +
>  target/s390x/translate.c | 56 ++
>  6 files changed, 140 insertions(+), 1 deletion(-)
>  create mode 100644 target/s390x/crypto_helper.c
> 
> diff --git a/target/s390x/Makefile.objs b/target/s390x/Makefile.objs
> index 46f6a8c..9d261ed 100644
> --- a/target/s390x/Makefile.objs
> +++ b/target/s390x/Makefile.objs
> @@ -1,4 +1,4 @@
> -obj-y += translate.o helper.o cpu.o interrupt.o
> +obj-y += translate.o helper.o cpu.o interrupt.o crypto_helper.o
>  obj-y += int_helper.o fpu_helper.o cc_helper.o mem_helper.o misc_helper.o
>  obj-y += gdbstub.o cpu_models.o cpu_features.o
>  obj-$(CONFIG_SOFTMMU) += machine.o ioinst.o arch_dump.o mmu_helper.o

This doesn't apply cleanly anymore.

> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 92120f4..45bd920 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -792,6 +792,7 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
> fbm)
>  S390_FEAT_STFLE,
>  S390_FEAT_EXTENDED_IMMEDIATE,
>  S390_FEAT_EXTENDED_TRANSLATION_2,
> +S390_FEAT_MSA,
>  S390_FEAT_EXTENDED_TRANSLATION_3,
>  S390_FEAT_LONG_DISPLACEMENT,
>  S390_FEAT_LONG_DISPLACEMENT_FAST,
> @@ -808,6 +809,9 @@ static void add_qemu_cpu_model_features(S390FeatBitmap 
> fbm)
>  S390_FEAT_STFLE_49,
>  S390_FEAT_LOCAL_TLB_CLEARING,
>  S390_FEAT_STFLE_53,
> +S390_FEAT_MSA_EXT_5,
> +S390_FEAT_MSA_EXT_3,
> +S390_FEAT_MSA_EXT_4,

I first thought that this looks weird, but it is the actual sequence of
the facility bits (probably the bit for MSA_EXT_5 has been reused?)

>  };
>  int i;
>  
> diff --git a/target/s390x/crypto_helper.c b/target/s390x/crypto_helper.c
> new file mode 100644
> index 000..125bd9b
> --- /dev/null
> +++ b/target/s390x/crypto_helper.c
> @@ -0,0 +1,65 @@
> +/*
> + *  s390x crypto helpers
> + *
> + *  Copyright (c) 2017 Red Hat Inc
> + *
> + *  Authors:
> + *   David Hildenbrand 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/main-loop.h"
> +#include "cpu.h"
> +#include "exec/helper-proto.h"
> +#include "exec/exec-all.h"
> +#include "exec/cpu_ldst.h"
> +
> +uint32_t HELPER(msa)(CPUS390XState *env, uint32_t r1, uint32_t r2, uint32_t 
> r3,
> + uint32_t type)
> +{
> +const uintptr_t ra = GETPC();
> +const uint8_t mod = env->regs[0] & 0x80ULL;
> +const uint8_t fc = env->regs[0] & 0x7fULL;
> +CPUState *cs = CPU(s390_env_get_cpu(env));
> +uint8_t subfunc[16] = { 0 };
> +uint64_t param_addr;
> +int i;
> +
> +switch (type) {
> +case S390_FEAT_TYPE_KMAC:
> +case S390_FEAT_TYPE_KIMD:
> +case S390_FEAT_TYPE_KLMD:
> +case S390_FEAT_TYPE_PCKMO:
> +case S390_FEAT_TYPE_PCC:
> +if (mod) {
> +cpu_restore_state(cs, ra);
> +program_interrupt(env, PGM_SPECIFICATION, 4);
> +return 0;
> +}
> +break;
> +}
> +
> +s390_get_feat_block(type, subfunc);
> +if (fc >= 128 || !test_be_bit(fc, subfunc)) {
> +cpu_restore_state(cs, ra);
> +program_interrupt(env, PGM_SPECIFICATION, 4);
> +return 0;
> +}
> +
> +switch (fc) {
> +case 0:

A comment which subfunction this is might be helpful.

> +for (i = 0; i < 16; i++) {
> +param_addr = wrap_address(env, env->regs[1] + i);

This does not compile for me (after massaging the Makefile above), as
wrap_address does not seem to be exported... can you do a respin,
please?

> +cpu_stb_data_ra(env, param_addr, subfunc[i], ra);
> +}
> +break;
> +default:
> +