Re: [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function

[Janosch Frank]

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 1/2] s390x: Diag308 move common parameter checking into function

[David Hildenbrand]

On 11.01.19 12:36, Janosch Frank wrote:
> Let's make that switch statement a bit shorter.
> 
> Signed-off-by: Janosch Frank 
> ---
>  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(_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(_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(_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