target/s390x/cpu.h | 1 + target/s390x/tcg/mem_helper.c | 61 ++++++++++++++++++++++++++++------- 2 files changed, 50 insertions(+), 12 deletions(-)
TPROT allows to specify an access key that should be used for checking
with the storage key of the destination page, to see whether an access
is allowed or not. Honor this storage key checking now in the emulated
TPROT instruction, too.
Since we need the absolute address of the page (for getting the storage
key), we are now also calling mmu_translate() directly instead of
going via s390_cpu_virt_mem_check_write/read() - since we're only
interested in one page, and since mmu_translate() does not try to inject
excetions directly (it reports them via the return code instead), this
is likely the better function to use in TPROT anyway.
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
This fixes the new TPROT-related storage key checks in this new
kvm-unit-tests patch:
https://lore.kernel.org/kvm/20220425161731.1575742-1-scgl@linux.ibm.com/
target/s390x/cpu.h | 1 +
target/s390x/tcg/mem_helper.c | 61 ++++++++++++++++++++++++++++-------
2 files changed, 50 insertions(+), 12 deletions(-)
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 7d6d01325b..348950239f 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -328,6 +328,7 @@ extern const VMStateDescription vmstate_s390_cpu;
/* Control register 0 bits */
#define CR0_LOWPROT 0x0000000010000000ULL
#define CR0_SECONDARY 0x0000000004000000ULL
+#define CR0_STOR_PROT_OVERRIDE 0x0000000001000000ULL
#define CR0_EDAT 0x0000000000800000ULL
#define CR0_AFP 0x0000000000040000ULL
#define CR0_VECTOR 0x0000000000020000ULL
diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
index fc52aa128b..1e0309bbe8 100644
--- a/target/s390x/tcg/mem_helper.c
+++ b/target/s390x/tcg/mem_helper.c
@@ -2141,43 +2141,80 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
return 0;
}
+static uint8_t get_skey(target_ulong real_addr)
+{
+ static S390SKeysClass *skeyclass;
+ static S390SKeysState *skeystate;
+ uint8_t skey = 0;
+
+ if (unlikely(!skeystate)) {
+ skeystate = s390_get_skeys_device();
+ skeyclass = S390_SKEYS_GET_CLASS(skeystate);
+ }
+
+ if (skeyclass->skeys_are_enabled(skeystate)) {
+ skeyclass->get_skeys(skeystate, real_addr / TARGET_PAGE_SIZE, 1, &skey);
+ }
+
+ return skey;
+}
+
uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
{
S390CPU *cpu = env_archcpu(env);
- CPUState *cs = env_cpu(env);
+ const int tp_acc = (a2 & SK_ACC_MASK) >> 4;
+ uint8_t skey;
+ int acc, pgm_code, pflags;
+ target_ulong abs_addr;
+ uint64_t asc = cpu->env.psw.mask & PSW_MASK_ASC;
+ uint64_t tec;
/*
* TODO: we currently don't handle all access protection types
- * (including access-list and key-controlled) as well as AR mode.
+ * (including access-list) as well as AR mode.
*/
- if (!s390_cpu_virt_mem_check_write(cpu, a1, 0, 1)) {
- /* Fetching permitted; storing permitted */
+ pgm_code = mmu_translate(env, a1, true, asc, &abs_addr, &pflags, &tec);
+ if (!pgm_code) {
+ /* Fetching permitted; storing permitted - but still check skeys */
+ skey = get_skey(abs_addr);
+ acc = (skey & SK_ACC_MASK) >> 4;
+ if (tp_acc != 0 && tp_acc != acc &&
+ !((env->cregs[0] & CR0_STOR_PROT_OVERRIDE) && acc == 9)) {
+ if (skey & SK_F) {
+ return 2;
+ } else {
+ return 1;
+ }
+ }
return 0;
}
- if (env->int_pgm_code == PGM_PROTECTION) {
+ if (pgm_code == PGM_PROTECTION) {
/* retry if reading is possible */
- cs->exception_index = -1;
- if (!s390_cpu_virt_mem_check_read(cpu, a1, 0, 1)) {
+ pgm_code = mmu_translate(env, a1, false, asc, &abs_addr, &pflags, &tec);
+ if (!pgm_code) {
/* Fetching permitted; storing not permitted */
+ skey = get_skey(abs_addr);
+ acc = (skey & SK_ACC_MASK) >> 4;
+ if ((skey & SK_F) && tp_acc != 0 && tp_acc != acc &&
+ !((env->cregs[0] & CR0_STOR_PROT_OVERRIDE) && acc == 9)) {
+ return 2;
+ }
return 1;
}
}
- switch (env->int_pgm_code) {
+ switch (pgm_code) {
case PGM_PROTECTION:
/* Fetching not permitted; storing not permitted */
- cs->exception_index = -1;
return 2;
case PGM_ADDRESSING:
case PGM_TRANS_SPEC:
/* exceptions forwarded to the guest */
- s390_cpu_virt_mem_handle_exc(cpu, GETPC());
- return 0;
+ tcg_s390_program_interrupt(env, pgm_code, GETPC());
}
/* Translation not available */
- cs->exception_index = -1;
return 3;
}
--
2.27.0On 5/2/22 10:25, Thomas Huth wrote:
> TPROT allows to specify an access key that should be used for checking
> with the storage key of the destination page, to see whether an access
> is allowed or not. Honor this storage key checking now in the emulated
> TPROT instruction, too.
>
> Since we need the absolute address of the page (for getting the storage
> key), we are now also calling mmu_translate() directly instead of
> going via s390_cpu_virt_mem_check_write/read() - since we're only
> interested in one page, and since mmu_translate() does not try to inject
> excetions directly (it reports them via the return code instead), this
> is likely the better function to use in TPROT anyway.
>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
> This fixes the new TPROT-related storage key checks in this new
> kvm-unit-tests patch:
> https://lore.kernel.org/kvm/20220425161731.1575742-1-scgl@linux.ibm.com/
Thanks for having a go at this.
The key checking logic looks good to me; the expressions get a bit unwieldy,
but that is a style thing.
However, I'm wondering whether it would be better to mirror what the kernel
is doing and address the
* TODO: key-controlled protection. Only CPU accesses make use of the
* PSW key. CSS accesses are different - we have to pass in the key.
in mmu_handle_skey, then tprot emulation would just relay the result of trying
the translation with store or fetch, passing in the key.
>
> target/s390x/cpu.h | 1 +
> target/s390x/tcg/mem_helper.c | 61 ++++++++++++++++++++++++++++-------
> 2 files changed, 50 insertions(+), 12 deletions(-)
>
> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> index 7d6d01325b..348950239f 100644
> --- a/target/s390x/cpu.h
> +++ b/target/s390x/cpu.h
> @@ -328,6 +328,7 @@ extern const VMStateDescription vmstate_s390_cpu;
> /* Control register 0 bits */
> #define CR0_LOWPROT 0x0000000010000000ULL
> #define CR0_SECONDARY 0x0000000004000000ULL
> +#define CR0_STOR_PROT_OVERRIDE 0x0000000001000000ULL
> #define CR0_EDAT 0x0000000000800000ULL
> #define CR0_AFP 0x0000000000040000ULL
> #define CR0_VECTOR 0x0000000000020000ULL
> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
> index fc52aa128b..1e0309bbe8 100644
> --- a/target/s390x/tcg/mem_helper.c
> +++ b/target/s390x/tcg/mem_helper.c
> @@ -2141,43 +2141,80 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
> return 0;
> }
>
[...]
> +
> uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
> {
> S390CPU *cpu = env_archcpu(env);
> - CPUState *cs = env_cpu(env);
> + const int tp_acc = (a2 & SK_ACC_MASK) >> 4;
> + uint8_t skey;
> + int acc, pgm_code, pflags;
> + target_ulong abs_addr;
> + uint64_t asc = cpu->env.psw.mask & PSW_MASK_ASC;
> + uint64_t tec;
>
> /*
> * TODO: we currently don't handle all access protection types
> - * (including access-list and key-controlled) as well as AR mode.
> + * (including access-list) as well as AR mode.
> */
> - if (!s390_cpu_virt_mem_check_write(cpu, a1, 0, 1)) {
> - /* Fetching permitted; storing permitted */
> + pgm_code = mmu_translate(env, a1, true, asc, &abs_addr, &pflags, &tec);
I don't like the use of true to indicate a store here, when values other than 0 and 1 are possible.
Any reason not to use MMU_DATA_STORE?
A comment about fetch protection override might be nice here:
/*
* Since fetch protection override may apply to half of page 0 only,
* it need not be considered in the following.
*/
> + if (!pgm_code) {
> + /* Fetching permitted; storing permitted - but still check skeys */
> + skey = get_skey(abs_addr);
> + acc = (skey & SK_ACC_MASK) >> 4;
> + if (tp_acc != 0 && tp_acc != acc &&
> + !((env->cregs[0] & CR0_STOR_PROT_OVERRIDE) && acc == 9)) {
> + if (skey & SK_F) {
> + return 2;
> + } else {
> + return 1;
> + }
> + }
> return 0;
> }
>
> - if (env->int_pgm_code == PGM_PROTECTION) {
> + if (pgm_code == PGM_PROTECTION) {
> /* retry if reading is possible */
> - cs->exception_index = -1;
> - if (!s390_cpu_virt_mem_check_read(cpu, a1, 0, 1)) {
> + pgm_code = mmu_translate(env, a1, false, asc, &abs_addr, &pflags, &tec);
> + if (!pgm_code) {
> /* Fetching permitted; storing not permitted */
> + skey = get_skey(abs_addr);
> + acc = (skey & SK_ACC_MASK) >> 4;
> + if ((skey & SK_F) && tp_acc != 0 && tp_acc != acc &&
> + !((env->cregs[0] & CR0_STOR_PROT_OVERRIDE) && acc == 9)) {
> + return 2;
> + }
> return 1;
> }
> }
>
> - switch (env->int_pgm_code) {
> + switch (pgm_code) {
> case PGM_PROTECTION:
> /* Fetching not permitted; storing not permitted */
> - cs->exception_index = -1;
> return 2;
> case PGM_ADDRESSING:
> case PGM_TRANS_SPEC:
> /* exceptions forwarded to the guest */
> - s390_cpu_virt_mem_handle_exc(cpu, GETPC());
> - return 0;
> + tcg_s390_program_interrupt(env, pgm_code, GETPC());
> }
>
> /* Translation not available */
> - cs->exception_index = -1;
> return 3;
> }
>
On 5/2/22 12:17, Janis Schoetterl-Glausch wrote:
> On 5/2/22 10:25, Thomas Huth wrote:
>> TPROT allows to specify an access key that should be used for checking
>> with the storage key of the destination page, to see whether an access
>> is allowed or not. Honor this storage key checking now in the emulated
>> TPROT instruction, too.
>>
>> Since we need the absolute address of the page (for getting the storage
>> key), we are now also calling mmu_translate() directly instead of
>> going via s390_cpu_virt_mem_check_write/read() - since we're only
>> interested in one page, and since mmu_translate() does not try to inject
>> excetions directly (it reports them via the return code instead), this
>> is likely the better function to use in TPROT anyway.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>> This fixes the new TPROT-related storage key checks in this new
>> kvm-unit-tests patch:
>> https://lore.kernel.org/kvm/20220425161731.1575742-1-scgl@linux.ibm.com/
>
> Thanks for having a go at this.
> The key checking logic looks good to me; the expressions get a bit unwieldy,
> but that is a style thing.
> However, I'm wondering whether it would be better to mirror what the kernel
> is doing and address the
>
> * TODO: key-controlled protection. Only CPU accesses make use of the
> * PSW key. CSS accesses are different - we have to pass in the key.
>
> in mmu_handle_skey, then tprot emulation would just relay the result of trying
> the translation with store or fetch, passing in the key.
>>
>> target/s390x/cpu.h | 1 +
>> target/s390x/tcg/mem_helper.c | 61 ++++++++++++++++++++++++++++-------
>> 2 files changed, 50 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
>> index 7d6d01325b..348950239f 100644
>> --- a/target/s390x/cpu.h
>> +++ b/target/s390x/cpu.h
>> @@ -328,6 +328,7 @@ extern const VMStateDescription vmstate_s390_cpu;
>> /* Control register 0 bits */
>> #define CR0_LOWPROT 0x0000000010000000ULL
>> #define CR0_SECONDARY 0x0000000004000000ULL
>> +#define CR0_STOR_PROT_OVERRIDE 0x0000000001000000ULL
>> #define CR0_EDAT 0x0000000000800000ULL
>> #define CR0_AFP 0x0000000000040000ULL
>> #define CR0_VECTOR 0x0000000000020000ULL
>> diff --git a/target/s390x/tcg/mem_helper.c b/target/s390x/tcg/mem_helper.c
>> index fc52aa128b..1e0309bbe8 100644
>> --- a/target/s390x/tcg/mem_helper.c
>> +++ b/target/s390x/tcg/mem_helper.c
>> @@ -2141,43 +2141,80 @@ uint32_t HELPER(testblock)(CPUS390XState *env, uint64_t real_addr)
>> return 0;
>> }
>>
>
> [...]
>
>> +
>> uint32_t HELPER(tprot)(CPUS390XState *env, uint64_t a1, uint64_t a2)
>> {
>> S390CPU *cpu = env_archcpu(env);
>> - CPUState *cs = env_cpu(env);
>> + const int tp_acc = (a2 & SK_ACC_MASK) >> 4;
>> + uint8_t skey;
>> + int acc, pgm_code, pflags;
>> + target_ulong abs_addr;
>> + uint64_t asc = cpu->env.psw.mask & PSW_MASK_ASC;
>> + uint64_t tec;
>>
>> /*
>> * TODO: we currently don't handle all access protection types
>> - * (including access-list and key-controlled) as well as AR mode.
>> + * (including access-list) as well as AR mode.
>> */
>> - if (!s390_cpu_virt_mem_check_write(cpu, a1, 0, 1)) {
>> - /* Fetching permitted; storing permitted */
>> + pgm_code = mmu_translate(env, a1, true, asc, &abs_addr, &pflags, &tec);
mmu_translate/mmu_handle_skey sets the change bit for stores, whereas TPROT specifies
that it doesn't.
Not sure what the best way to handle this is.
Additional pretend fetch/store access modes?
>
> I don't like the use of true to indicate a store here, when values other than 0 and 1 are possible.
> Any reason not to use MMU_DATA_STORE?
>
> A comment about fetch protection override might be nice here:
> /*
> * Since fetch protection override may apply to half of page 0 only,
> * it need not be considered in the following.
> */
Disregard that, it's not true, TPROT does honor fetch-protection override, I just
made a mistake while adding a test for it to the kvm-unit-test.
[...]
© 2016 - 2024 Red Hat, Inc.