| Message ID | 20211216131657.1057978-1-nrb@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [qemu] s390x/css: fix PMCW invalid mask | expand |
On Thu, 16 Dec 2021 14:16:57 +0100 Nico Boehr <nrb@linux.ibm.com> wrote: > Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, > as per the principles of operation, bit 5 is ignored in MSCH and bits 0, > 1, 6 and 7 need to be zero. On a second thought, don't we have to make sure then that bit 5 is ignored? static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src) { int i; dest->intparm = be32_to_cpu(src->intparm); dest->flags = be16_to_cpu(src->flags); dest->devno = be16_to_cpu(src->devno); Here we seem to grab flags as a whole, but actually we would have to mask of bit 5. I can spin a patch myself, provided we agree on that this needs to be fixed, but, it would probably be better to have the two changes in one patch. Regards, Halil > > As both PMCW_FLAGS_MASK_INVALID and ioinst_schib_valid() are only used > by ioinst_handle_msch(), adjust the mask accordingly. > > Fixes: db1c8f53bfb1 ("s390: Channel I/O basic definitions.") > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
On Fri, 17 Dec 2021 14:58:11 +0100 Halil Pasic <pasic@linux.ibm.com> wrote: > On Thu, 16 Dec 2021 14:16:57 +0100 > Nico Boehr <nrb@linux.ibm.com> wrote: > > > Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, > > as per the principles of operation, bit 5 is ignored in MSCH and bits 0, > > 1, 6 and 7 need to be zero. > > On a second thought, don't we have to make sure then that bit 5 is > ignored? > > static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src) > { > int i; > > dest->intparm = be32_to_cpu(src->intparm); > dest->flags = be16_to_cpu(src->flags); > dest->devno = be16_to_cpu(src->devno); > > Here we seem to grab flags as a whole, but actually we would have to > mask of bit 5. > > I can spin a patch myself, provided we agree on that this needs to be > fixed, but, it would probably be better to have the two changes in one > patch. > I didn't read far enough. We do mask bit 5 in in css_do_msch() and copy_pmcw_from_guest() works on a schib_copy. Everything is fine! Regards, Halil
On 12/17/21 14:58, Halil Pasic wrote: > On Thu, 16 Dec 2021 14:16:57 +0100 > Nico Boehr <nrb@linux.ibm.com> wrote: > >> Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, >> as per the principles of operation, bit 5 is ignored in MSCH and bits 0, >> 1, 6 and 7 need to be zero. > > On a second thought, don't we have to make sure then that bit 5 is > ignored? > > static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src) > { > int i; > > dest->intparm = be32_to_cpu(src->intparm); > dest->flags = be16_to_cpu(src->flags); > dest->devno = be16_to_cpu(src->devno); > > Here we seem to grab flags as a whole, but actually we would have to > mask of bit 5. Why? If this bit is ignored by the machine shouldn't we just ignore it? Forcing it to 0 or to 1 is purely arbitrary no? > > I can spin a patch myself, provided we agree on that this needs to be > fixed, but, it would probably be better to have the two changes in one > patch. > > Regards, > Halil > > >> >> As both PMCW_FLAGS_MASK_INVALID and ioinst_schib_valid() are only used >> by ioinst_handle_msch(), adjust the mask accordingly. >> >> Fixes: db1c8f53bfb1 ("s390: Channel I/O basic definitions.") >> Signed-off-by: Nico Boehr <nrb@linux.ibm.com> >> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> >> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> >> Reviewed-by: Janosch Frank <frankja@linux.ibm.com>
On Fri, 17 Dec 2021 18:13:47 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > >> Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, > >> as per the principles of operation, bit 5 is ignored in MSCH and bits 0, > >> 1, 6 and 7 need to be zero. > > > > On a second thought, don't we have to make sure then that bit 5 is > > ignored? > > > > static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src) > > { > > int i; > > > > dest->intparm = be32_to_cpu(src->intparm); > > dest->flags = be16_to_cpu(src->flags); > > dest->devno = be16_to_cpu(src->devno); > > > > Here we seem to grab flags as a whole, but actually we would have to > > mask of bit 5. > > Why? > If this bit is ignored by the machine shouldn't we just ignore it? > Forcing it to 0 or to 1 is purely arbitrary no? We do the masking later on: IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib) { [..] /* Only update the program-modifiable fields. */ schib->pmcw.intparm = schib_copy.pmcw.intparm; oldflags = schib->pmcw.flags; schib->pmcw.flags &= ~(PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA | PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME | PMCW_FLAGS_MASK_MP); schib->pmcw.flags |= schib_copy.pmcw.flags & (PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA | PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME | PMCW_FLAGS_MASK_MP); [..] I just didn't read far enough. We do that for a while now. The PoP says that the machine shall ignore other fields of the PMCW when an MSCH is performed. I.e. we should not update "our" pmcw.flags bit 5 from 0 to 1 even if 1 was supplied, and thus STSCH should keep storing the bit 5 as 0 even if there was a MSCH with bit 5 set. Regards, Halil
On 12/17/21 20:28, Halil Pasic wrote: > On Fri, 17 Dec 2021 18:13:47 +0100 > Pierre Morel <pmorel@linux.ibm.com> wrote: > >>>> Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, >>>> as per the principles of operation, bit 5 is ignored in MSCH and bits 0, >>>> 1, 6 and 7 need to be zero. >>> >>> On a second thought, don't we have to make sure then that bit 5 is >>> ignored? >>> >>> static void copy_pmcw_from_guest(PMCW *dest, const PMCW *src) >>> { >>> int i; >>> >>> dest->intparm = be32_to_cpu(src->intparm); >>> dest->flags = be16_to_cpu(src->flags); >>> dest->devno = be16_to_cpu(src->devno); >>> >>> Here we seem to grab flags as a whole, but actually we would have to >>> mask of bit 5. >> >> Why? >> If this bit is ignored by the machine shouldn't we just ignore it? >> Forcing it to 0 or to 1 is purely arbitrary no? > > We do the masking later on: > IOInstEnding css_do_msch(SubchDev *sch, const SCHIB *orig_schib) > { > [..] > /* Only update the program-modifiable fields. */ > schib->pmcw.intparm = schib_copy.pmcw.intparm; > oldflags = schib->pmcw.flags; > schib->pmcw.flags &= ~(PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA | > PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME | > PMCW_FLAGS_MASK_MP); > schib->pmcw.flags |= schib_copy.pmcw.flags & > (PMCW_FLAGS_MASK_ISC | PMCW_FLAGS_MASK_ENA | > PMCW_FLAGS_MASK_LM | PMCW_FLAGS_MASK_MME | > PMCW_FLAGS_MASK_MP); > [..] > > I just didn't read far enough. We do that for a while now. yes. > > The PoP says that the machine shall ignore other fields > of the PMCW when an MSCH is performed. I.e. we should not update > "our" pmcw.flags bit 5 from 0 to 1 even if 1 was supplied, and > thus STSCH should keep storing the bit 5 as 0 even if there was > a MSCH with bit 5 set. So I do understand that there is no problem, we do not keep track of this bit in our pmcw.flags and stsch keep storing this bit as 0. right? Regards, Pierre > > Regards, > Halil >
On Mon, 20 Dec 2021 11:44:44 +0100 Pierre Morel <pmorel@linux.ibm.com> wrote: > > > > The PoP says that the machine shall ignore other fields > > of the PMCW when an MSCH is performed. I.e. we should not update > > "our" pmcw.flags bit 5 from 0 to 1 even if 1 was supplied, and > > thus STSCH should keep storing the bit 5 as 0 even if there was > > a MSCH with bit 5 set. > > So I do understand that there is no problem, we do not keep track > of this bit in our pmcw.flags and stsch keep storing this bit as 0. right? Right! False alarm by me -- sorry. Regards, Halil
On Thu, Dec 16 2021, Nico Boehr <nrb@linux.ibm.com> wrote: > Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, > as per the principles of operation, bit 5 is ignored in MSCH and bits 0, > 1, 6 and 7 need to be zero. > > As both PMCW_FLAGS_MASK_INVALID and ioinst_schib_valid() are only used > by ioinst_handle_msch(), adjust the mask accordingly. > > Fixes: db1c8f53bfb1 ("s390: Channel I/O basic definitions.") > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > --- > include/hw/s390x/ioinst.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h > index 3771fff9d44d..ea8d0f244492 100644 > --- a/include/hw/s390x/ioinst.h > +++ b/include/hw/s390x/ioinst.h > @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(sizeof(PMCW) != 28, "size of PMCW is wrong"); > #define PMCW_FLAGS_MASK_MP 0x0004 > #define PMCW_FLAGS_MASK_TF 0x0002 > #define PMCW_FLAGS_MASK_DNV 0x0001 > -#define PMCW_FLAGS_MASK_INVALID 0x0700 > +#define PMCW_FLAGS_MASK_INVALID 0xc300 Removing bit 5 from this mask makes sense, at it is simply ignored. I'm a bit confused about bits 0 and 1, however. They are _QF and _W, respectively (just out of the context here), which are in the same class as _DNV (i.e. characteristics of the subchannel that cannot be modified via msch). Looking at the PoP, I don't see what is supposed to happen if the program tries to modify the dnv bit (maybe I'm simply overlooking it.) I would naively assume that the w bit should behave in the same way (as it does for message subchannels what dnv does for I/O subchannels, and the rest of the values are not meaningful if it is not set), and probably also the qf bit (as it doesn't make sense for the program to turn QDIO capabilities on and off.) The main question is whether trying to modify these bits causes an error or is ignored. The PoP suggests an error (no idea if the internal architecture agrees, it hopefully does); what happens for dnv? We support neither message subchannels nor QDIO in QEMU, so it's probably not relevant right now; but it would still be good if we could clarify the expected behaviour here :) > > #define PMCW_CHARS_MASK_ST 0x00e00000 > #define PMCW_CHARS_MASK_MBFC 0x00000004
On Wed, 22 Dec 2021 17:46:11 +0100 Cornelia Huck <cohuck@redhat.com> wrote: > On Thu, Dec 16 2021, Nico Boehr <nrb@linux.ibm.com> wrote: > > > Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, > > as per the principles of operation, bit 5 is ignored in MSCH and bits 0, > > 1, 6 and 7 need to be zero. > > > > As both PMCW_FLAGS_MASK_INVALID and ioinst_schib_valid() are only used > > by ioinst_handle_msch(), adjust the mask accordingly. > > > > Fixes: db1c8f53bfb1 ("s390: Channel I/O basic definitions.") > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > > --- > > include/hw/s390x/ioinst.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h > > index 3771fff9d44d..ea8d0f244492 100644 > > --- a/include/hw/s390x/ioinst.h > > +++ b/include/hw/s390x/ioinst.h > > @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(sizeof(PMCW) != 28, "size of PMCW is wrong"); > > #define PMCW_FLAGS_MASK_MP 0x0004 > > #define PMCW_FLAGS_MASK_TF 0x0002 > > #define PMCW_FLAGS_MASK_DNV 0x0001 > > -#define PMCW_FLAGS_MASK_INVALID 0x0700 > > +#define PMCW_FLAGS_MASK_INVALID 0xc300 > > Removing bit 5 from this mask makes sense, at it is simply ignored. > > I'm a bit confused about bits 0 and 1, however. They are _QF and _W, > respectively (just out of the context here), which are in the same class > as _DNV (i.e. characteristics of the subchannel that cannot be modified > via msch). Looking at the PoP, I don't see what is supposed to happen if > the program tries to modify the dnv bit (maybe I'm simply overlooking > it.) I would naively assume that the w bit should behave in the same way > (as it does for message subchannels what dnv does for I/O subchannels, > and the rest of the values are not meaningful if it is not set), and > probably also the qf bit (as it doesn't make sense for the program to > turn QDIO capabilities on and off.) The main question is whether trying > to modify these bits causes an error or is ignored. The PoP suggests an > error (no idea if the internal architecture agrees, it hopefully does); > what happens for dnv? """ Bits 0, 1, 6, and 7 of word 1, and bits 0-28 of word 6 of the SCHIB operand must be zeros, and bits 9 and 10 of word 1 must not both be ones. When the extended-I/O-measurement-block facility is installed and a format-1 measurement block is specified, bits 26-31 of word 11 must be specified as zeros. """ (IBM z/Architecture Principles of Operation (SA22-7832-10), 14-8) The internal architecture agrees. DNV bit is ignored. Regarding why, I don't know. Probably for historic reasons. The PoP tells us that whatever is not listed as significant or checked and results in an operation exception if not appropriate is ignored: """ The remaining fields of the SCHIB are ignored and do not affect the processing of MODIFY SUBCHANNEL. (For further details, see “Subchannel-Information Block” on page 2 """ (same page) Regarding word 1 of the SCHIB the alignment between PoP and AR is perfect AFAICT. > > We support neither message subchannels nor QDIO in QEMU, so it's > probably not relevant right now; but it would still be good if we could > clarify the expected behaviour here :) > > > > > #define PMCW_CHARS_MASK_ST 0x00e00000 > > #define PMCW_CHARS_MASK_MBFC 0x00000004 > >
On Thu, Dec 23 2021, Halil Pasic <pasic@linux.ibm.com> wrote: > On Wed, 22 Dec 2021 17:46:11 +0100 > Cornelia Huck <cohuck@redhat.com> wrote: > >> On Thu, Dec 16 2021, Nico Boehr <nrb@linux.ibm.com> wrote: >> >> > Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, >> > as per the principles of operation, bit 5 is ignored in MSCH and bits 0, >> > 1, 6 and 7 need to be zero. >> > >> > As both PMCW_FLAGS_MASK_INVALID and ioinst_schib_valid() are only used >> > by ioinst_handle_msch(), adjust the mask accordingly. >> > >> > Fixes: db1c8f53bfb1 ("s390: Channel I/O basic definitions.") >> > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> >> > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> >> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> >> > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> >> > --- >> > include/hw/s390x/ioinst.h | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h >> > index 3771fff9d44d..ea8d0f244492 100644 >> > --- a/include/hw/s390x/ioinst.h >> > +++ b/include/hw/s390x/ioinst.h >> > @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(sizeof(PMCW) != 28, "size of PMCW is wrong"); >> > #define PMCW_FLAGS_MASK_MP 0x0004 >> > #define PMCW_FLAGS_MASK_TF 0x0002 >> > #define PMCW_FLAGS_MASK_DNV 0x0001 >> > -#define PMCW_FLAGS_MASK_INVALID 0x0700 >> > +#define PMCW_FLAGS_MASK_INVALID 0xc300 >> >> Removing bit 5 from this mask makes sense, at it is simply ignored. >> >> I'm a bit confused about bits 0 and 1, however. They are _QF and _W, >> respectively (just out of the context here), which are in the same class >> as _DNV (i.e. characteristics of the subchannel that cannot be modified >> via msch). Looking at the PoP, I don't see what is supposed to happen if >> the program tries to modify the dnv bit (maybe I'm simply overlooking >> it.) I would naively assume that the w bit should behave in the same way >> (as it does for message subchannels what dnv does for I/O subchannels, >> and the rest of the values are not meaningful if it is not set), and >> probably also the qf bit (as it doesn't make sense for the program to >> turn QDIO capabilities on and off.) The main question is whether trying >> to modify these bits causes an error or is ignored. The PoP suggests an >> error (no idea if the internal architecture agrees, it hopefully does); >> what happens for dnv? > > """ > Bits 0, 1, 6, and 7 of word 1, and bits 0-28 of word 6 > of the SCHIB operand must be zeros, and bits 9 and > 10 of word 1 must not both be ones. When the > extended-I/O-measurement-block facility is installed > and a format-1 measurement block is specified, bits > 26-31 of word 11 must be specified as zeros. > """ > (IBM z/Architecture Principles of Operation (SA22-7832-10), 14-8) > > The internal architecture agrees. Thanks for checking. > > DNV bit is ignored. Regarding why, I don't know. Probably for historic > reasons. Yeah, it's a bit odd, "for historic reason" seems plausible. > The PoP tells us that whatever is not listed as significant > or checked and results in an operation exception if not appropriate > is ignored: > """ > The remaining > fields of the SCHIB are ignored and do not affect the > processing of MODIFY SUBCHANNEL. (For further > details, see “Subchannel-Information Block” on > page 2 > """ > (same page) > > Regarding word 1 of the SCHIB the alignment between PoP and AR is > perfect AFAICT. > >> >> We support neither message subchannels nor QDIO in QEMU, so it's >> probably not relevant right now; but it would still be good if we could >> clarify the expected behaviour here :) >> >> > >> > #define PMCW_CHARS_MASK_ST 0x00e00000 >> > #define PMCW_CHARS_MASK_MBFC 0x00000004 >> >> In that case, Reviewed-by: Cornelia Huck <cohuck@redhat.com> <not sure whether I will pick it, or Thomas will; but probably in the new year :)>
On 16/12/2021 14.16, Nico Boehr wrote: > Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, > as per the principles of operation, bit 5 is ignored in MSCH and bits 0, > 1, 6 and 7 need to be zero. > > As both PMCW_FLAGS_MASK_INVALID and ioinst_schib_valid() are only used > by ioinst_handle_msch(), adjust the mask accordingly. > > Fixes: db1c8f53bfb1 ("s390: Channel I/O basic definitions.") > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> > Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> > --- > include/hw/s390x/ioinst.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h > index 3771fff9d44d..ea8d0f244492 100644 > --- a/include/hw/s390x/ioinst.h > +++ b/include/hw/s390x/ioinst.h > @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(sizeof(PMCW) != 28, "size of PMCW is wrong"); > #define PMCW_FLAGS_MASK_MP 0x0004 > #define PMCW_FLAGS_MASK_TF 0x0002 > #define PMCW_FLAGS_MASK_DNV 0x0001 > -#define PMCW_FLAGS_MASK_INVALID 0x0700 > +#define PMCW_FLAGS_MASK_INVALID 0xc300 > > #define PMCW_CHARS_MASK_ST 0x00e00000 > #define PMCW_CHARS_MASK_MBFC 0x00000004 Thanks, queued to my s390x-next branch now: https://gitlab.com/thuth/qemu/-/commits/s390x-next/ Thomas
diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h index 3771fff9d44d..ea8d0f244492 100644 --- a/include/hw/s390x/ioinst.h +++ b/include/hw/s390x/ioinst.h @@ -107,7 +107,7 @@ QEMU_BUILD_BUG_MSG(sizeof(PMCW) != 28, "size of PMCW is wrong"); #define PMCW_FLAGS_MASK_MP 0x0004 #define PMCW_FLAGS_MASK_TF 0x0002 #define PMCW_FLAGS_MASK_DNV 0x0001 -#define PMCW_FLAGS_MASK_INVALID 0x0700 +#define PMCW_FLAGS_MASK_INVALID 0xc300 #define PMCW_CHARS_MASK_ST 0x00e00000 #define PMCW_CHARS_MASK_MBFC 0x00000004
Previously, we required bits 5, 6 and 7 to be zero (0x07 == 0b111). But, as per the principles of operation, bit 5 is ignored in MSCH and bits 0, 1, 6 and 7 need to be zero. As both PMCW_FLAGS_MASK_INVALID and ioinst_schib_valid() are only used by ioinst_handle_msch(), adjust the mask accordingly. Fixes: db1c8f53bfb1 ("s390: Channel I/O basic definitions.") Signed-off-by: Nico Boehr <nrb@linux.ibm.com> Reviewed-by: Pierre Morel <pmorel@linux.ibm.com> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> Reviewed-by: Janosch Frank <frankja@linux.ibm.com> --- include/hw/s390x/ioinst.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)