[PATCH qemu] s390x/css: fix PMCW invalid mask

Nico Boehr posted 1 patch 2 years, 4 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211216131657.1057978-1-nrb@linux.ibm.com
Maintainers: Cornelia Huck <cohuck@redhat.com>, Thomas Huth <thuth@redhat.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Halil Pasic <pasic@linux.ibm.com>
include/hw/s390x/ioinst.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Nico Boehr 2 years, 4 months ago
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
-- 
2.31.1


Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Cornelia Huck 2 years, 4 months ago
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


Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Halil Pasic 2 years, 4 months ago
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  
> 
> 


Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Cornelia Huck 2 years, 4 months ago
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 :)>


Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Thomas Huth 2 years, 4 months ago
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


Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Halil Pasic 2 years, 4 months ago
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>

Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Halil Pasic 2 years, 4 months ago
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

Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Pierre Morel 2 years, 4 months ago

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>

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Halil Pasic 2 years, 4 months ago
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

Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Pierre Morel 2 years, 4 months ago

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
> 

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
Posted by Halil Pasic 2 years, 4 months ago
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