| Message ID | 1510075479-17224-5-git-send-email-pmorel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 :(
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 :( >
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 :( > > > > >
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 --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 */