* [Qemu-devel] [PATCH 0/2] s390x: Diag 308 improvements
@ 2019-01-11 11:36 Janosch Frank
2019-01-11 11:36 ` [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function Janosch Frank
2019-01-11 11:36 ` [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes Janosch Frank
0 siblings, 2 replies; 8+ messages in thread
From: Janosch Frank @ 2019-01-11 11:36 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, cohuck, david, qemu-s390x, qemu-stable
Let's make it architecture compliant and a bit more readable.
Each non implemented diag 308 supcode should result in a specification
exception. Operating systems use the exceptions to query which codes
are available.
Janosch Frank (2):
s390x: Diag308 move common parameter checking into function
s390x: Return specification exception for unimplemented diag 308
subcodes
target/s390x/diag.c | 36 +++++++++++++++++++-----------------
1 file changed, 19 insertions(+), 17 deletions(-)
--
2.14.3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function
2019-01-11 11:36 [Qemu-devel] [PATCH 0/2] s390x: Diag 308 improvements Janosch Frank
@ 2019-01-11 11:36 ` Janosch Frank
2019-01-11 15:40 ` David Hildenbrand
2019-01-11 11:36 ` [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes Janosch Frank
1 sibling, 1 reply; 8+ messages in thread
From: Janosch Frank @ 2019-01-11 11:36 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, cohuck, david, qemu-s390x, qemu-stable
Let's make that switch statement a bit shorter.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
target/s390x/diag.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index acb0f3d4af..cfd7222ddd 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -53,6 +53,22 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
#define DIAG_308_RC_NO_CONF 0x0102
#define DIAG_308_RC_INVALID 0x0402
+static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
+ uintptr_t ra)
+{
+ if ((r1 & 1) || (addr & 0x0fffULL)) {
+ s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
+ return -EINVAL;
+ }
+ if (!address_space_access_valid(&address_space_memory, addr,
+ sizeof(IplParameterBlock), true,
+ MEMTXATTRS_UNSPECIFIED)) {
+ s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
+ return -EFAULT;
+ }
+ return 0;
+}
+
void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
{
CPUState *cs = CPU(s390_env_get_cpu(env));
@@ -81,14 +97,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
s390_ipl_reset_request(cs, S390_RESET_REIPL);
break;
case 5:
- if ((r1 & 1) || (addr & 0x0fffULL)) {
- s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
- return;
- }
- if (!address_space_access_valid(&address_space_memory, addr,
- sizeof(IplParameterBlock), false,
- MEMTXATTRS_UNSPECIFIED)) {
- s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
+ if (diag308_parm_check(env, r1, addr, ra)) {
return;
}
iplb = g_new0(IplParameterBlock, 1);
@@ -111,14 +120,7 @@ out:
g_free(iplb);
return;
case 6:
- if ((r1 & 1) || (addr & 0x0fffULL)) {
- s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
- return;
- }
- if (!address_space_access_valid(&address_space_memory, addr,
- sizeof(IplParameterBlock), true,
- MEMTXATTRS_UNSPECIFIED)) {
- s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
+ if (diag308_parm_check(env, r1, addr, ra)) {
return;
}
iplb = s390_ipl_get_iplb();
--
2.14.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes
2019-01-11 11:36 [Qemu-devel] [PATCH 0/2] s390x: Diag 308 improvements Janosch Frank
2019-01-11 11:36 ` [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function Janosch Frank
@ 2019-01-11 11:36 ` Janosch Frank
2019-01-11 11:43 ` Christian Borntraeger
` (2 more replies)
1 sibling, 3 replies; 8+ messages in thread
From: Janosch Frank @ 2019-01-11 11:36 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, cohuck, david, qemu-s390x, qemu-stable
The architecture specifies specification exceptions for all
unavailable subcodes.
The presence of subcodes is indicated by checking some query subcode.
For example 6 will indicate that 3-6 are available. So future systems
might call new subcodes to check for new features. This should not
trigger a hw error, instead we return the architectured specification
exception.
Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
Cc: qemu-stable@nongnu.org
---
target/s390x/diag.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/s390x/diag.c b/target/s390x/diag.c
index cfd7222ddd..c28cf1d9f1 100644
--- a/target/s390x/diag.c
+++ b/target/s390x/diag.c
@@ -132,7 +132,7 @@ out:
}
return;
default:
- hw_error("Unhandled diag308 subcode %" PRIx64, subcode);
+ s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
break;
}
}
--
2.14.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes
2019-01-11 11:36 ` [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes Janosch Frank
@ 2019-01-11 11:43 ` Christian Borntraeger
2019-01-11 15:37 ` David Hildenbrand
2019-01-14 17:32 ` Cornelia Huck
2 siblings, 0 replies; 8+ messages in thread
From: Christian Borntraeger @ 2019-01-11 11:43 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: cohuck, david, qemu-s390x, qemu-stable
On 11.01.2019 12:36, Janosch Frank wrote:
> The architecture specifies specification exceptions for all
> unavailable subcodes.
>
> The presence of subcodes is indicated by checking some query subcode.
> For example 6 will indicate that 3-6 are available. So future systems
> might call new subcodes to check for new features. This should not
> trigger a hw error, instead we return the architectured specification
> exception.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Cc: qemu-stable@nongnu.org
Yes, PIC06 is definitively the right thing to do.
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
> target/s390x/diag.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index cfd7222ddd..c28cf1d9f1 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -132,7 +132,7 @@ out:
> }
> return;
> default:
> - hw_error("Unhandled diag308 subcode %" PRIx64, subcode);
> + s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> break;
> }
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes
2019-01-11 11:36 ` [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes Janosch Frank
2019-01-11 11:43 ` Christian Borntraeger
@ 2019-01-11 15:37 ` David Hildenbrand
2019-01-14 17:32 ` Cornelia Huck
2 siblings, 0 replies; 8+ messages in thread
From: David Hildenbrand @ 2019-01-11 15:37 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, qemu-s390x, qemu-stable
On 11.01.19 12:36, Janosch Frank wrote:
> The architecture specifies specification exceptions for all
> unavailable subcodes.
>
> The presence of subcodes is indicated by checking some query subcode.
> For example 6 will indicate that 3-6 are available. So future systems
> might call new subcodes to check for new features. This should not
> trigger a hw error, instead we return the architectured specification
> exception.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Cc: qemu-stable@nongnu.org
> ---
> target/s390x/diag.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index cfd7222ddd..c28cf1d9f1 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -132,7 +132,7 @@ out:
> }
> return;
> default:
> - hw_error("Unhandled diag308 subcode %" PRIx64, subcode);
> + s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> break;
> }
> }
>
Reviewed-by: David Hildenbrand <david@redhat.com>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function
2019-01-11 11:36 ` [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function Janosch Frank
@ 2019-01-11 15:40 ` David Hildenbrand
2019-01-11 15:51 ` Janosch Frank
0 siblings, 1 reply; 8+ messages in thread
From: David Hildenbrand @ 2019-01-11 15:40 UTC (permalink / raw)
To: Janosch Frank, qemu-devel; +Cc: borntraeger, cohuck, qemu-s390x, qemu-stable
On 11.01.19 12:36, Janosch Frank wrote:
> Let's make that switch statement a bit shorter.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
> target/s390x/diag.c | 34 ++++++++++++++++++----------------
> 1 file changed, 18 insertions(+), 16 deletions(-)
>
> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
> index acb0f3d4af..cfd7222ddd 100644
> --- a/target/s390x/diag.c
> +++ b/target/s390x/diag.c
> @@ -53,6 +53,22 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
> #define DIAG_308_RC_NO_CONF 0x0102
> #define DIAG_308_RC_INVALID 0x0402
>
> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
> + uintptr_t ra)
> +{
> + if ((r1 & 1) || (addr & 0x0fffULL)) {
> + s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> + return -EINVAL;
> + }
> + if (!address_space_access_valid(&address_space_memory, addr,
> + sizeof(IplParameterBlock), true,
This is wrong, you would check for writing although you are only
reading. (true vs. false)
> + MEMTXATTRS_UNSPECIFIED)) {
> + s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
> + return -EFAULT;
> + }
> + return 0;
> +}
> +
> void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
> {
> CPUState *cs = CPU(s390_env_get_cpu(env));
> @@ -81,14 +97,7 @@ void handle_diag_308(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
> s390_ipl_reset_request(cs, S390_RESET_REIPL);
> break;
> case 5:
> - if ((r1 & 1) || (addr & 0x0fffULL)) {
> - s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> - return;
> - }
> - if (!address_space_access_valid(&address_space_memory, addr,
> - sizeof(IplParameterBlock), false,
> - MEMTXATTRS_UNSPECIFIED)) {
> - s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
> + if (diag308_parm_check(env, r1, addr, ra)) {
> return;
> }
> iplb = g_new0(IplParameterBlock, 1);
> @@ -111,14 +120,7 @@ out:
> g_free(iplb);
> return;
> case 6:
> - if ((r1 & 1) || (addr & 0x0fffULL)) {
> - s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
> - return;
> - }
> - if (!address_space_access_valid(&address_space_memory, addr,
> - sizeof(IplParameterBlock), true,
> - MEMTXATTRS_UNSPECIFIED)) {
> - s390_program_interrupt(env, PGM_ADDRESSING, ILEN_AUTO, ra);
> + if (diag308_parm_check(env, r1, addr, ra)) {
> return;
> }
> iplb = s390_ipl_get_iplb();
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function
2019-01-11 15:40 ` David Hildenbrand
@ 2019-01-11 15:51 ` Janosch Frank
0 siblings, 0 replies; 8+ messages in thread
From: Janosch Frank @ 2019-01-11 15:51 UTC (permalink / raw)
To: David Hildenbrand, qemu-devel
Cc: borntraeger, cohuck, qemu-s390x, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]
On 11.01.19 16:40, David Hildenbrand wrote:
> On 11.01.19 12:36, Janosch Frank wrote:
>> Let's make that switch statement a bit shorter.
>>
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>> target/s390x/diag.c | 34 ++++++++++++++++++----------------
>> 1 file changed, 18 insertions(+), 16 deletions(-)
>>
>> diff --git a/target/s390x/diag.c b/target/s390x/diag.c
>> index acb0f3d4af..cfd7222ddd 100644
>> --- a/target/s390x/diag.c
>> +++ b/target/s390x/diag.c
>> @@ -53,6 +53,22 @@ int handle_diag_288(CPUS390XState *env, uint64_t r1, uint64_t r3)
>> #define DIAG_308_RC_NO_CONF 0x0102
>> #define DIAG_308_RC_INVALID 0x0402
>>
>> +static int diag308_parm_check(CPUS390XState *env, uint64_t r1, uint64_t addr,
>> + uintptr_t ra)
>> +{
>> + if ((r1 & 1) || (addr & 0x0fffULL)) {
>> + s390_program_interrupt(env, PGM_SPECIFICATION, ILEN_AUTO, ra);
>> + return -EINVAL;
>> + }
>> + if (!address_space_access_valid(&address_space_memory, addr,
>> + sizeof(IplParameterBlock), true,
>
> This is wrong, you would check for writing although you are only
> reading. (true vs. false)
Friday morning patches are the best patches :)
Let's drop that one for now, we'll touch it again, if we need to tinker
there.
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes
2019-01-11 11:36 ` [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes Janosch Frank
2019-01-11 11:43 ` Christian Borntraeger
2019-01-11 15:37 ` David Hildenbrand
@ 2019-01-14 17:32 ` Cornelia Huck
2 siblings, 0 replies; 8+ messages in thread
From: Cornelia Huck @ 2019-01-14 17:32 UTC (permalink / raw)
To: Janosch Frank; +Cc: qemu-devel, borntraeger, david, qemu-s390x, qemu-stable
On Fri, 11 Jan 2019 12:36:57 +0100
Janosch Frank <frankja@linux.ibm.com> wrote:
> The architecture specifies specification exceptions for all
> unavailable subcodes.
>
> The presence of subcodes is indicated by checking some query subcode.
> For example 6 will indicate that 3-6 are available. So future systems
> might call new subcodes to check for new features. This should not
> trigger a hw error, instead we return the architectured specification
> exception.
>
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Cc: qemu-stable@nongnu.org
> ---
> target/s390x/diag.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Thanks, applied.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-01-14 17:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 11:36 [Qemu-devel] [PATCH 0/2] s390x: Diag 308 improvements Janosch Frank
2019-01-11 11:36 ` [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function Janosch Frank
2019-01-11 15:40 ` David Hildenbrand
2019-01-11 15:51 ` Janosch Frank
2019-01-11 11:36 ` [Qemu-devel] [PATCH 2/2] s390x: Return specification exception for unimplemented diag 308 subcodes Janosch Frank
2019-01-11 11:43 ` Christian Borntraeger
2019-01-11 15:37 ` David Hildenbrand
2019-01-14 17:32 ` Cornelia Huck
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.