diff mbox series

[4/7] s390x/pci: rework PCI STORE BLOCK

Message ID 1510075479-17224-5-git-send-email-pmorel@linux.vnet.ibm.com
State New
Headers show
Series s390x/pci: Improve zPCI to cover more cases | expand

Commit Message

Pierre Morel Nov. 7, 2017, 5:24 p.m. UTC 
Enhance the fault detection.

Add the maxstbl entry to both the Query PCI Function Group
response and the PCIBusDevice structure.

Initialize the maxstbl to 128 per default until we get
the actual data from the hardware.

Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-bus.h  |  1 +
 hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
 hw/s390x/s390-pci-inst.h |  2 +-
 3 files changed, 40 insertions(+), 25 deletions(-)

Comments

Cornelia Huck Nov. 13, 2017, 3:23 p.m. UTC | #1 
On Tue,  7 Nov 2017 18:24:36 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> Enhance the fault detection.
> 
> Add the maxstbl entry to both the Query PCI Function Group
> response and the PCIBusDevice structure.
> 
> Initialize the maxstbl to 128 per default until we get
> the actual data from the hardware.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-bus.h  |  1 +
>  hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
>  hw/s390x/s390-pci-inst.h |  2 +-
>  3 files changed, 40 insertions(+), 25 deletions(-)
> 

> @@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      fh = env->regs[r1] >> 32;
>      pcias = (env->regs[r1] >> 16) & 0xf;
>      len = env->regs[r1] & 0xff;
> +    offset = env->regs[r3];
>  
> -    if (pcias > 5) {
> -        DPRINTF("pcistb invalid space\n");
> -        setcc(cpu, ZPCI_PCI_LS_ERR);
> -        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> -        return 0;
> -    }
> -
> -    switch (len) {
> -    case 16:
> -    case 32:
> -    case 64:
> -    case 128:
> -        break;
> -    default:
> -        program_interrupt(env, PGM_SPECIFICATION, 6);
> +    if (!(fh & FH_MASK_ENABLE)) {
> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);

So this means you move checking for the device before checking for the
parameters or the as.

>          return 0;
>      }
>  
> @@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      }
>  
>      switch (pbdev->state) {
> -    case ZPCI_FS_RESERVED:
> -    case ZPCI_FS_STANDBY:
> -    case ZPCI_FS_DISABLED:
>      case ZPCI_FS_PERMANENT_ERROR:
> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> -        return 0;
>      case ZPCI_FS_ERROR:
>          setcc(cpu, ZPCI_PCI_LS_ERR);
>          s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
> @@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>          break;
>      }
>  
> +    if (pcias > 5) {
> +        DPRINTF("pcistb invalid space\n");
> +        setcc(cpu, ZPCI_PCI_LS_ERR);
> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> +        return 0;
> +    }
> +
> +    /* Verify the address, offset and length */
> +    /* offset must be a multiple of 8 */
> +    if (offset % 8) {
> +        goto addressing_error;
> +    }
> +    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
> +        goto addressing_error;
> +    }
> +    /* Do not cross a 4K-byte boundary */
> +    if (((offset & 0xfff) + len) > 0x1000) {
> +        goto addressing_error;
> +    }
> +    /* Guest address must be double word aligned */
> +    if (gaddr & 0x07UL) {
> +        goto addressing_error;
> +    }

So the checks here are only evaluated if the instruction actually pokes
at a valid region?

> +
>      mr = pbdev->pdev->io_regions[pcias].memory;
> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
> +    if (!memory_region_access_valid(mr, offset, len, true)) {
>          program_interrupt(env, PGM_OPERAND, 6);
>          return 0;
>      }
> @@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>      }
>  
>      for (i = 0; i < len / 8; i++) {
> -        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
> -                                     ldq_p(buffer + i * 8), 8,
> -                                     MEMTXATTRS_UNSPECIFIED);
> +        result = memory_region_dispatch_write(mr, offset + i * 8,
> +                                              ldq_p(buffer + i * 8), 8,
> +                                              MEMTXATTRS_UNSPECIFIED);
>          if (result != MEMTX_OK) {
>              program_interrupt(env, PGM_OPERAND, 6);
>              return 0;
> @@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>  
>      setcc(cpu, ZPCI_PCI_LS_OK);
>      return 0;
> +
> +addressing_error:
> +    program_interrupt(env, PGM_SPECIFICATION, 6);
> +    return 0;
>  }

This seems more readable; I can't verify whether it is actually correct
without access to the architecture, though :(
Pierre Morel Nov. 13, 2017, 4:38 p.m. UTC | #2 
On 13/11/2017 16:23, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:36 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> Enhance the fault detection.
>>
>> Add the maxstbl entry to both the Query PCI Function Group
>> response and the PCIBusDevice structure.
>>
>> Initialize the maxstbl to 128 per default until we get
>> the actual data from the hardware.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>   hw/s390x/s390-pci-bus.h  |  1 +
>>   hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
>>   hw/s390x/s390-pci-inst.h |  2 +-
>>   3 files changed, 40 insertions(+), 25 deletions(-)
>>
> 
>> @@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>       fh = env->regs[r1] >> 32;
>>       pcias = (env->regs[r1] >> 16) & 0xf;
>>       len = env->regs[r1] & 0xff;
>> +    offset = env->regs[r3];
>>   
>> -    if (pcias > 5) {
>> -        DPRINTF("pcistb invalid space\n");
>> -        setcc(cpu, ZPCI_PCI_LS_ERR);
>> -        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
>> -        return 0;
>> -    }
>> -
>> -    switch (len) {
>> -    case 16:
>> -    case 32:
>> -    case 64:
>> -    case 128:
>> -        break;
>> -    default:
>> -        program_interrupt(env, PGM_SPECIFICATION, 6);
>> +    if (!(fh & FH_MASK_ENABLE)) {
>> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> 
> So this means you move checking for the device before checking for the
> parameters or the as.

Yes, this is clearly following the specifications that CC=3 has priority 
over CC=1 or CC=2.

By the way I find that defining ZPCI_PCI_LS_INVAL_HANDLE is obfuscating, 
we have the information from the test we just made but we loose the 
information about if it is a 1, 2 or 3 CC value.
May be in another patch?

> 
>>           return 0;
>>       }
>>   
>> @@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>       }
>>   
>>       switch (pbdev->state) {
>> -    case ZPCI_FS_RESERVED:
>> -    case ZPCI_FS_STANDBY:
>> -    case ZPCI_FS_DISABLED:
>>       case ZPCI_FS_PERMANENT_ERROR:
>> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
>> -        return 0;
>>       case ZPCI_FS_ERROR:
>>           setcc(cpu, ZPCI_PCI_LS_ERR);
>>           s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
>> @@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>           break;
>>       }
>>   
>> +    if (pcias > 5) {
>> +        DPRINTF("pcistb invalid space\n");
>> +        setcc(cpu, ZPCI_PCI_LS_ERR);
>> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
>> +        return 0;
>> +    }
>> +
>> +    /* Verify the address, offset and length */
>> +    /* offset must be a multiple of 8 */
>> +    if (offset % 8) {
>> +        goto addressing_error;
>> +    }
>> +    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
>> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
>> +        goto addressing_error;
>> +    }
>> +    /* Do not cross a 4K-byte boundary */
>> +    if (((offset & 0xfff) + len) > 0x1000) {
>> +        goto addressing_error;
>> +    }
>> +    /* Guest address must be double word aligned */
>> +    if (gaddr & 0x07UL) {
>> +        goto addressing_error;
>> +    }
> 
> So the checks here are only evaluated if the instruction actually pokes
> at a valid region?

hum, I did not find the precedence of execution for PCI STORE BLOCK.

My logic is that you must find a destination before you start reading 
the source, so I would say it is the right way to do.
But the experience as already shown that my logic may not always be 
compatible with the internals of S390x :)

However, the documentation specifies that if an error condition is 
detected that precludes the *initiation* of the store operation a CC=1 
is set.
The fact that the *initiation* is focused on enforce the idea I have on 
the precedence between the low level operations.

> 
>> +
>>       mr = pbdev->pdev->io_regions[pcias].memory;
>> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
>> +    if (!memory_region_access_valid(mr, offset, len, true)) {
>>           program_interrupt(env, PGM_OPERAND, 6);
>>           return 0;
>>       }
>> @@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>       }
>>   
>>       for (i = 0; i < len / 8; i++) {
>> -        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
>> -                                     ldq_p(buffer + i * 8), 8,
>> -                                     MEMTXATTRS_UNSPECIFIED);
>> +        result = memory_region_dispatch_write(mr, offset + i * 8,
>> +                                              ldq_p(buffer + i * 8), 8,
>> +                                              MEMTXATTRS_UNSPECIFIED);
>>           if (result != MEMTX_OK) {
>>               program_interrupt(env, PGM_OPERAND, 6);
>>               return 0;
>> @@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>   
>>       setcc(cpu, ZPCI_PCI_LS_OK);
>>       return 0;
>> +
>> +addressing_error:
>> +    program_interrupt(env, PGM_SPECIFICATION, 6);
>> +    return 0;
>>   }
> 
> This seems more readable; I can't verify whether it is actually correct
> without access to the architecture, though :(
>
Cornelia Huck Nov. 13, 2017, 5:10 p.m. UTC | #3 
On Mon, 13 Nov 2017 17:38:40 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 13/11/2017 16:23, Cornelia Huck wrote:
> > On Tue,  7 Nov 2017 18:24:36 +0100
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >   
> >> Enhance the fault detection.
> >>
> >> Add the maxstbl entry to both the Query PCI Function Group
> >> response and the PCIBusDevice structure.
> >>
> >> Initialize the maxstbl to 128 per default until we get
> >> the actual data from the hardware.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> ---
> >>   hw/s390x/s390-pci-bus.h  |  1 +
> >>   hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
> >>   hw/s390x/s390-pci-inst.h |  2 +-
> >>   3 files changed, 40 insertions(+), 25 deletions(-)
> >>  
> >   
> >> @@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>       fh = env->regs[r1] >> 32;
> >>       pcias = (env->regs[r1] >> 16) & 0xf;
> >>       len = env->regs[r1] & 0xff;
> >> +    offset = env->regs[r3];
> >>   
> >> -    if (pcias > 5) {
> >> -        DPRINTF("pcistb invalid space\n");
> >> -        setcc(cpu, ZPCI_PCI_LS_ERR);
> >> -        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> >> -        return 0;
> >> -    }
> >> -
> >> -    switch (len) {
> >> -    case 16:
> >> -    case 32:
> >> -    case 64:
> >> -    case 128:
> >> -        break;
> >> -    default:
> >> -        program_interrupt(env, PGM_SPECIFICATION, 6);
> >> +    if (!(fh & FH_MASK_ENABLE)) {
> >> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);  
> > 
> > So this means you move checking for the device before checking for the
> > parameters or the as.  
> 
> Yes, this is clearly following the specifications that CC=3 has priority 
> over CC=1 or CC=2.

OK, it would then make sense to mention in the patch description that
you fixed up the precedence as well.

> 
> By the way I find that defining ZPCI_PCI_LS_INVAL_HANDLE is obfuscating, 
> we have the information from the test we just made but we loose the 
> information about if it is a 1, 2 or 3 CC value.
> May be in another patch?

Let's keep this for now, we can revisit that later.

> 
> >   
> >>           return 0;
> >>       }
> >>   
> >> @@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>       }
> >>   
> >>       switch (pbdev->state) {
> >> -    case ZPCI_FS_RESERVED:
> >> -    case ZPCI_FS_STANDBY:
> >> -    case ZPCI_FS_DISABLED:
> >>       case ZPCI_FS_PERMANENT_ERROR:
> >> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
> >> -        return 0;
> >>       case ZPCI_FS_ERROR:
> >>           setcc(cpu, ZPCI_PCI_LS_ERR);
> >>           s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
> >> @@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>           break;
> >>       }
> >>   
> >> +    if (pcias > 5) {
> >> +        DPRINTF("pcistb invalid space\n");
> >> +        setcc(cpu, ZPCI_PCI_LS_ERR);
> >> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
> >> +        return 0;
> >> +    }
> >> +
> >> +    /* Verify the address, offset and length */
> >> +    /* offset must be a multiple of 8 */
> >> +    if (offset % 8) {
> >> +        goto addressing_error;
> >> +    }
> >> +    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
> >> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
> >> +        goto addressing_error;
> >> +    }
> >> +    /* Do not cross a 4K-byte boundary */
> >> +    if (((offset & 0xfff) + len) > 0x1000) {
> >> +        goto addressing_error;
> >> +    }
> >> +    /* Guest address must be double word aligned */
> >> +    if (gaddr & 0x07UL) {
> >> +        goto addressing_error;
> >> +    }  
> > 
> > So the checks here are only evaluated if the instruction actually pokes
> > at a valid region?  
> 
> hum, I did not find the precedence of execution for PCI STORE BLOCK.
> 
> My logic is that you must find a destination before you start reading 
> the source, so I would say it is the right way to do.
> But the experience as already shown that my logic may not always be 
> compatible with the internals of S390x :)
> 
> However, the documentation specifies that if an error condition is 
> detected that precludes the *initiation* of the store operation a CC=1 
> is set.
> The fact that the *initiation* is focused on enforce the idea I have on 
> the precedence between the low level operations.

OK, this interpretation makes sense. It might be a good idea to poke the
architecture authors if it is ambiguous, though :)

> 
> >   
> >> +
> >>       mr = pbdev->pdev->io_regions[pcias].memory;
> >> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
> >> +    if (!memory_region_access_valid(mr, offset, len, true)) {
> >>           program_interrupt(env, PGM_OPERAND, 6);
> >>           return 0;
> >>       }
> >> @@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>       }
> >>   
> >>       for (i = 0; i < len / 8; i++) {
> >> -        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
> >> -                                     ldq_p(buffer + i * 8), 8,
> >> -                                     MEMTXATTRS_UNSPECIFIED);
> >> +        result = memory_region_dispatch_write(mr, offset + i * 8,
> >> +                                              ldq_p(buffer + i * 8), 8,
> >> +                                              MEMTXATTRS_UNSPECIFIED);
> >>           if (result != MEMTX_OK) {
> >>               program_interrupt(env, PGM_OPERAND, 6);
> >>               return 0;
> >> @@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
> >>   
> >>       setcc(cpu, ZPCI_PCI_LS_OK);
> >>       return 0;
> >> +
> >> +addressing_error:
> >> +    program_interrupt(env, PGM_SPECIFICATION, 6);
> >> +    return 0;
> >>   }  
> > 
> > This seems more readable; I can't verify whether it is actually correct
> > without access to the architecture, though :(
> >   
> 
> 
>
Pierre Morel Nov. 15, 2017, 10:05 a.m. UTC | #4 
On 13/11/2017 18:10, Cornelia Huck wrote:
> On Mon, 13 Nov 2017 17:38:40 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 13/11/2017 16:23, Cornelia Huck wrote:
>>> On Tue,  7 Nov 2017 18:24:36 +0100
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>    
>>>> Enhance the fault detection.
>>>>
>>>> Add the maxstbl entry to both the Query PCI Function Group
>>>> response and the PCIBusDevice structure.
>>>>
>>>> Initialize the maxstbl to 128 per default until we get
>>>> the actual data from the hardware.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> ---
>>>>    hw/s390x/s390-pci-bus.h  |  1 +
>>>>    hw/s390x/s390-pci-inst.c | 62 +++++++++++++++++++++++++++++-------------------
>>>>    hw/s390x/s390-pci-inst.h |  2 +-
>>>>    3 files changed, 40 insertions(+), 25 deletions(-)
>>>>   
>>>    
>>>> @@ -662,22 +664,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>        fh = env->regs[r1] >> 32;
>>>>        pcias = (env->regs[r1] >> 16) & 0xf;
>>>>        len = env->regs[r1] & 0xff;
>>>> +    offset = env->regs[r3];
>>>>    
>>>> -    if (pcias > 5) {
>>>> -        DPRINTF("pcistb invalid space\n");
>>>> -        setcc(cpu, ZPCI_PCI_LS_ERR);
>>>> -        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
>>>> -        return 0;
>>>> -    }
>>>> -
>>>> -    switch (len) {
>>>> -    case 16:
>>>> -    case 32:
>>>> -    case 64:
>>>> -    case 128:
>>>> -        break;
>>>> -    default:
>>>> -        program_interrupt(env, PGM_SPECIFICATION, 6);
>>>> +    if (!(fh & FH_MASK_ENABLE)) {
>>>> +        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
>>>
>>> So this means you move checking for the device before checking for the
>>> parameters or the as.
>>
>> Yes, this is clearly following the specifications that CC=3 has priority
>> over CC=1 or CC=2.
> 
> OK, it would then make sense to mention in the patch description that
> you fixed up the precedence as well.

I will do
thanks

> 
>>
>> By the way I find that defining ZPCI_PCI_LS_INVAL_HANDLE is obfuscating,
>> we have the information from the test we just made but we loose the
>> information about if it is a 1, 2 or 3 CC value.
>> May be in another patch?
> 
> Let's keep this for now, we can revisit that later.

OK

> 
>>
>>>    
>>>>            return 0;
>>>>        }
>>>>    
>>>> @@ -689,12 +679,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>        }
>>>>    
>>>>        switch (pbdev->state) {
>>>> -    case ZPCI_FS_RESERVED:
>>>> -    case ZPCI_FS_STANDBY:
>>>> -    case ZPCI_FS_DISABLED:
>>>>        case ZPCI_FS_PERMANENT_ERROR:
>>>> -        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
>>>> -        return 0;
>>>>        case ZPCI_FS_ERROR:
>>>>            setcc(cpu, ZPCI_PCI_LS_ERR);
>>>>            s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
>>>> @@ -703,8 +688,33 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>            break;
>>>>        }
>>>>    
>>>> +    if (pcias > 5) {
>>>> +        DPRINTF("pcistb invalid space\n");
>>>> +        setcc(cpu, ZPCI_PCI_LS_ERR);
>>>> +        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    /* Verify the address, offset and length */
>>>> +    /* offset must be a multiple of 8 */
>>>> +    if (offset % 8) {
>>>> +        goto addressing_error;
>>>> +    }
>>>> +    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
>>>> +    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
>>>> +        goto addressing_error;
>>>> +    }
>>>> +    /* Do not cross a 4K-byte boundary */
>>>> +    if (((offset & 0xfff) + len) > 0x1000) {
>>>> +        goto addressing_error;
>>>> +    }
>>>> +    /* Guest address must be double word aligned */
>>>> +    if (gaddr & 0x07UL) {
>>>> +        goto addressing_error;
>>>> +    }
>>>
>>> So the checks here are only evaluated if the instruction actually pokes
>>> at a valid region?
>>
>> hum, I did not find the precedence of execution for PCI STORE BLOCK.
>>
>> My logic is that you must find a destination before you start reading
>> the source, so I would say it is the right way to do.
>> But the experience as already shown that my logic may not always be
>> compatible with the internals of S390x :)
>>
>> However, the documentation specifies that if an error condition is
>> detected that precludes the *initiation* of the store operation a CC=1
>> is set.
>> The fact that the *initiation* is focused on enforce the idea I have on
>> the precedence between the low level operations.
> 
> OK, this interpretation makes sense. It might be a good idea to poke the
> architecture authors if it is ambiguous, though :)

Yes.

> 
>>
>>>    
>>>> +
>>>>        mr = pbdev->pdev->io_regions[pcias].memory;
>>>> -    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
>>>> +    if (!memory_region_access_valid(mr, offset, len, true)) {
>>>>            program_interrupt(env, PGM_OPERAND, 6);
>>>>            return 0;
>>>>        }
>>>> @@ -714,9 +724,9 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>        }
>>>>    
>>>>        for (i = 0; i < len / 8; i++) {
>>>> -        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
>>>> -                                     ldq_p(buffer + i * 8), 8,
>>>> -                                     MEMTXATTRS_UNSPECIFIED);
>>>> +        result = memory_region_dispatch_write(mr, offset + i * 8,
>>>> +                                              ldq_p(buffer + i * 8), 8,
>>>> +                                              MEMTXATTRS_UNSPECIFIED);
>>>>            if (result != MEMTX_OK) {
>>>>                program_interrupt(env, PGM_OPERAND, 6);
>>>>                return 0;
>>>> @@ -725,6 +735,10 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
>>>>    
>>>>        setcc(cpu, ZPCI_PCI_LS_OK);
>>>>        return 0;
>>>> +
>>>> +addressing_error:
>>>> +    program_interrupt(env, PGM_SPECIFICATION, 6);
>>>> +    return 0;
>>>>    }
>>>
>>> This seems more readable; I can't verify whether it is actually correct
>>> without access to the architecture, though :(
>>>    
>>
>>
>>
> 
>
diff mbox series

Patch

diff --git a/hw/s390x/s390-pci-bus.h b/hw/s390x/s390-pci-bus.h
index 560bd82..2993f0d 100644
--- a/hw/s390x/s390-pci-bus.h
+++ b/hw/s390x/s390-pci-bus.h
@@ -284,6 +284,7 @@  struct S390PCIBusDevice {
     uint64_t fmb_addr;
     uint8_t isc;
     uint16_t noi;
+    uint16_t maxstbl;
     uint8_t sum;
     S390MsixInfo msix;
     AdapterRoutes routes;
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 62ebaa0..646137e 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -294,6 +294,7 @@  int clp_service_call(S390CPU *cpu, uint8_t r2)
         stq_p(&resgrp->msia, ZPCI_MSI_ADDR);
         stw_p(&resgrp->mui, 0);
         stw_p(&resgrp->i, 128);
+        stw_p(&resgrp->maxstbl, 128);
         resgrp->version = 0;
 
         stw_p(&resgrp->hdr.rsp, CLP_RC_OK);
@@ -648,6 +649,7 @@  int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     S390PCIBusDevice *pbdev;
     MemoryRegion *mr;
     MemTxResult result;
+    uint64_t offset;
     int i;
     uint32_t fh;
     uint8_t pcias;
@@ -662,22 +664,10 @@  int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     fh = env->regs[r1] >> 32;
     pcias = (env->regs[r1] >> 16) & 0xf;
     len = env->regs[r1] & 0xff;
+    offset = env->regs[r3];
 
-    if (pcias > 5) {
-        DPRINTF("pcistb invalid space\n");
-        setcc(cpu, ZPCI_PCI_LS_ERR);
-        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
-        return 0;
-    }
-
-    switch (len) {
-    case 16:
-    case 32:
-    case 64:
-    case 128:
-        break;
-    default:
-        program_interrupt(env, PGM_SPECIFICATION, 6);
+    if (!(fh & FH_MASK_ENABLE)) {
+        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
         return 0;
     }
 
@@ -689,12 +679,7 @@  int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     }
 
     switch (pbdev->state) {
-    case ZPCI_FS_RESERVED:
-    case ZPCI_FS_STANDBY:
-    case ZPCI_FS_DISABLED:
     case ZPCI_FS_PERMANENT_ERROR:
-        setcc(cpu, ZPCI_PCI_LS_INVAL_HANDLE);
-        return 0;
     case ZPCI_FS_ERROR:
         setcc(cpu, ZPCI_PCI_LS_ERR);
         s390_set_status_code(env, r1, ZPCI_PCI_ST_BLOCKED);
@@ -703,8 +688,33 @@  int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
         break;
     }
 
+    if (pcias > 5) {
+        DPRINTF("pcistb invalid space\n");
+        setcc(cpu, ZPCI_PCI_LS_ERR);
+        s390_set_status_code(env, r1, ZPCI_PCI_ST_INVAL_AS);
+        return 0;
+    }
+
+    /* Verify the address, offset and length */
+    /* offset must be a multiple of 8 */
+    if (offset % 8) {
+        goto addressing_error;
+    }
+    /* Length must be greater than 8, a multiple of 8, lower than maxstbl */
+    if ((len <= 8) || (len % 8) || (len > pbdev->maxstbl)) {
+        goto addressing_error;
+    }
+    /* Do not cross a 4K-byte boundary */
+    if (((offset & 0xfff) + len) > 0x1000) {
+        goto addressing_error;
+    }
+    /* Guest address must be double word aligned */
+    if (gaddr & 0x07UL) {
+        goto addressing_error;
+    }
+
     mr = pbdev->pdev->io_regions[pcias].memory;
-    if (!memory_region_access_valid(mr, env->regs[r3], len, true)) {
+    if (!memory_region_access_valid(mr, offset, len, true)) {
         program_interrupt(env, PGM_OPERAND, 6);
         return 0;
     }
@@ -714,9 +724,9 @@  int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
     }
 
     for (i = 0; i < len / 8; i++) {
-        result = memory_region_dispatch_write(mr, env->regs[r3] + i * 8,
-                                     ldq_p(buffer + i * 8), 8,
-                                     MEMTXATTRS_UNSPECIFIED);
+        result = memory_region_dispatch_write(mr, offset + i * 8,
+                                              ldq_p(buffer + i * 8), 8,
+                                              MEMTXATTRS_UNSPECIFIED);
         if (result != MEMTX_OK) {
             program_interrupt(env, PGM_OPERAND, 6);
             return 0;
@@ -725,6 +735,10 @@  int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr,
 
     setcc(cpu, ZPCI_PCI_LS_OK);
     return 0;
+
+addressing_error:
+    program_interrupt(env, PGM_SPECIFICATION, 6);
+    return 0;
 }
 
 static int reg_irqs(CPUS390XState *env, S390PCIBusDevice *pbdev, ZpciFib fib)
diff --git a/hw/s390x/s390-pci-inst.h b/hw/s390x/s390-pci-inst.h
index 94a959f..d6aeadd 100644
--- a/hw/s390x/s390-pci-inst.h
+++ b/hw/s390x/s390-pci-inst.h
@@ -162,7 +162,7 @@  typedef struct ClpRspQueryPciGrp {
 #define CLP_RSP_QPCIG_MASK_FRAME   0x2
 #define CLP_RSP_QPCIG_MASK_REFRESH 0x1
     uint8_t fr;
-    uint16_t reserved2;
+    uint16_t maxstbl;
     uint16_t mui;
     uint64_t reserved3;
     uint64_t dasm; /* dma address space mask */