| Message ID | 1607546066-2240-4-git-send-email-mjrosato@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x/pci: Fixing s390 vfio-pci ISM support | expand |
On Wed, 9 Dec 2020 15:34:21 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > In pcistb_service_call, we are grabbing 8 bits from a guest register to > indicate the length of the store operation -- but per the architecture > the length is actually defined by 13 bits of the guest register. > > Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > --- > hw/s390x/s390-pci-inst.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 70bfd91..db86f12 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -750,7 +750,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, > int i; > uint32_t fh; > uint8_t pcias; > - uint8_t len; > + uint16_t len; > uint8_t buffer[128]; > > if (env->psw.mask & PSW_MASK_PSTATE) { > @@ -760,7 +760,7 @@ 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; > + len = env->regs[r1] & 0x1fff; > offset = env->regs[r3]; > > if (!(fh & FH_MASK_ENABLE)) { Is that a general problem that we just did not notice before? If yes, this probably deserves a Fixes: tag and can be queued independently of the rest of the series.
On 12/10/20 5:30 AM, Cornelia Huck wrote: > On Wed, 9 Dec 2020 15:34:21 -0500 > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > >> In pcistb_service_call, we are grabbing 8 bits from a guest register to >> indicate the length of the store operation -- but per the architecture >> the length is actually defined by 13 bits of the guest register. >> >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> >> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> >> --- >> hw/s390x/s390-pci-inst.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >> index 70bfd91..db86f12 100644 >> --- a/hw/s390x/s390-pci-inst.c >> +++ b/hw/s390x/s390-pci-inst.c >> @@ -750,7 +750,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, >> int i; >> uint32_t fh; >> uint8_t pcias; >> - uint8_t len; >> + uint16_t len; >> uint8_t buffer[128]; >> >> if (env->psw.mask & PSW_MASK_PSTATE) { >> @@ -760,7 +760,7 @@ 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; >> + len = env->regs[r1] & 0x1fff; >> offset = env->regs[r3]; >> >> if (!(fh & FH_MASK_ENABLE)) { > > Is that a general problem that we just did not notice before? > > If yes, this probably deserves a Fixes: tag and can be queued > independently of the rest of the series. > Good point. I can split this out, and same for "s390x/pci: Fix memory_region_access_valid call"
On Thu, 10 Dec 2020 10:15:06 -0500 Matthew Rosato <mjrosato@linux.ibm.com> wrote: > On 12/10/20 5:30 AM, Cornelia Huck wrote: > > On Wed, 9 Dec 2020 15:34:21 -0500 > > Matthew Rosato <mjrosato@linux.ibm.com> wrote: > > > >> In pcistb_service_call, we are grabbing 8 bits from a guest register to > >> indicate the length of the store operation -- but per the architecture > >> the length is actually defined by 13 bits of the guest register. > >> > >> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com> > >> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > >> --- > >> hw/s390x/s390-pci-inst.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > >> index 70bfd91..db86f12 100644 > >> --- a/hw/s390x/s390-pci-inst.c > >> +++ b/hw/s390x/s390-pci-inst.c > >> @@ -750,7 +750,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, > >> int i; > >> uint32_t fh; > >> uint8_t pcias; > >> - uint8_t len; > >> + uint16_t len; > >> uint8_t buffer[128]; > >> > >> if (env->psw.mask & PSW_MASK_PSTATE) { > >> @@ -760,7 +760,7 @@ 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; > >> + len = env->regs[r1] & 0x1fff; > >> offset = env->regs[r3]; > >> > >> if (!(fh & FH_MASK_ENABLE)) { > > > > Is that a general problem that we just did not notice before? > > > > If yes, this probably deserves a Fixes: tag and can be queued > > independently of the rest of the series. > > > > Good point. I can split this out, and same for "s390x/pci: Fix > memory_region_access_valid call" > Any plans for sending this? I'm currently collecting patches for a last pull request before the holidays; but I'm sure there will be more pull requests next year :)
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 70bfd91..db86f12 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -750,7 +750,7 @@ int pcistb_service_call(S390CPU *cpu, uint8_t r1, uint8_t r3, uint64_t gaddr, int i; uint32_t fh; uint8_t pcias; - uint8_t len; + uint16_t len; uint8_t buffer[128]; if (env->psw.mask & PSW_MASK_PSTATE) { @@ -760,7 +760,7 @@ 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; + len = env->regs[r1] & 0x1fff; offset = env->regs[r3]; if (!(fh & FH_MASK_ENABLE)) {