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

[Cornelia Huck]

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 :)>