qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH qemu] s390x/css: fix PMCW invalid mask
@ 2021-12-16 13:16 Nico Boehr
  2021-12-17 13:58 ` Halil Pasic
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Nico Boehr @ 2021-12-16 13:16 UTC (permalink / raw)
  To: qemu-s390x
  Cc: thuth, Nico Boehr, frankja, pmorel, cohuck, qemu-devel, pasic,
	borntraeger

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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-16 13:16 [PATCH qemu] s390x/css: fix PMCW invalid mask Nico Boehr
@ 2021-12-17 13:58 ` Halil Pasic
  2021-12-17 14:54   ` Halil Pasic
  2021-12-17 17:13   ` Pierre Morel
  2021-12-22 16:46 ` Cornelia Huck
  2022-01-05  8:42 ` Thomas Huth
  2 siblings, 2 replies; 11+ messages in thread
From: Halil Pasic @ 2021-12-17 13:58 UTC (permalink / raw)
  To: Nico Boehr
  Cc: thuth, frankja, pmorel, cohuck, qemu-devel, Halil Pasic,
	borntraeger, qemu-s390x

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>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-17 13:58 ` Halil Pasic
@ 2021-12-17 14:54   ` Halil Pasic
  2021-12-17 17:13   ` Pierre Morel
  1 sibling, 0 replies; 11+ messages in thread
From: Halil Pasic @ 2021-12-17 14:54 UTC (permalink / raw)
  To: Nico Boehr
  Cc: thuth, frankja, pmorel, cohuck, qemu-devel, Halil Pasic,
	borntraeger, qemu-s390x

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-17 13:58 ` Halil Pasic
  2021-12-17 14:54   ` Halil Pasic
@ 2021-12-17 17:13   ` Pierre Morel
  2021-12-17 19:28     ` Halil Pasic
  1 sibling, 1 reply; 11+ messages in thread
From: Pierre Morel @ 2021-12-17 17:13 UTC (permalink / raw)
  To: Halil Pasic, Nico Boehr
  Cc: thuth, frankja, cohuck, qemu-devel, borntraeger, qemu-s390x



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-17 17:13   ` Pierre Morel
@ 2021-12-17 19:28     ` Halil Pasic
  2021-12-20 10:44       ` Pierre Morel
  0 siblings, 1 reply; 11+ messages in thread
From: Halil Pasic @ 2021-12-17 19:28 UTC (permalink / raw)
  To: Pierre Morel
  Cc: thuth, Nico Boehr, frankja, cohuck, qemu-devel, Halil Pasic,
	borntraeger, qemu-s390x

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-17 19:28     ` Halil Pasic
@ 2021-12-20 10:44       ` Pierre Morel
  2021-12-20 12:11         ` Halil Pasic
  0 siblings, 1 reply; 11+ messages in thread
From: Pierre Morel @ 2021-12-20 10:44 UTC (permalink / raw)
  To: Halil Pasic
  Cc: thuth, Nico Boehr, frankja, cohuck, qemu-devel, borntraeger, qemu-s390x



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-20 10:44       ` Pierre Morel
@ 2021-12-20 12:11         ` Halil Pasic
  0 siblings, 0 replies; 11+ messages in thread
From: Halil Pasic @ 2021-12-20 12:11 UTC (permalink / raw)
  To: Pierre Morel
  Cc: thuth, Nico Boehr, frankja, cohuck, qemu-devel, Halil Pasic,
	borntraeger, qemu-s390x

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-16 13:16 [PATCH qemu] s390x/css: fix PMCW invalid mask Nico Boehr
  2021-12-17 13:58 ` Halil Pasic
@ 2021-12-22 16:46 ` Cornelia Huck
  2021-12-23 10:41   ` Halil Pasic
  2022-01-05  8:42 ` Thomas Huth
  2 siblings, 1 reply; 11+ messages in thread
From: Cornelia Huck @ 2021-12-22 16:46 UTC (permalink / raw)
  To: Nico Boehr, qemu-s390x
  Cc: thuth, Nico Boehr, frankja, pmorel, qemu-devel, pasic, borntraeger

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-22 16:46 ` Cornelia Huck
@ 2021-12-23 10:41   ` Halil Pasic
  2021-12-23 11:12     ` Cornelia Huck
  0 siblings, 1 reply; 11+ messages in thread
From: Halil Pasic @ 2021-12-23 10:41 UTC (permalink / raw)
  To: Cornelia Huck
  Cc: thuth, Nico Boehr, frankja, pmorel, qemu-devel, Halil Pasic,
	borntraeger, qemu-s390x

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-23 10:41   ` Halil Pasic
@ 2021-12-23 11:12     ` Cornelia Huck
  0 siblings, 0 replies; 11+ messages in thread
From: Cornelia Huck @ 2021-12-23 11:12 UTC (permalink / raw)
  To: Halil Pasic
  Cc: thuth, Nico Boehr, frankja, pmorel, qemu-devel, Halil Pasic,
	borntraeger, qemu-s390x

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH qemu] s390x/css: fix PMCW invalid mask
  2021-12-16 13:16 [PATCH qemu] s390x/css: fix PMCW invalid mask Nico Boehr
  2021-12-17 13:58 ` Halil Pasic
  2021-12-22 16:46 ` Cornelia Huck
@ 2022-01-05  8:42 ` Thomas Huth
  2 siblings, 0 replies; 11+ messages in thread
From: Thomas Huth @ 2022-01-05  8:42 UTC (permalink / raw)
  To: Nico Boehr, qemu-s390x
  Cc: frankja, pmorel, cohuck, qemu-devel, pasic, borntraeger

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



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2022-01-05  8:43 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-16 13:16 [PATCH qemu] s390x/css: fix PMCW invalid mask Nico Boehr
2021-12-17 13:58 ` Halil Pasic
2021-12-17 14:54   ` Halil Pasic
2021-12-17 17:13   ` Pierre Morel
2021-12-17 19:28     ` Halil Pasic
2021-12-20 10:44       ` Pierre Morel
2021-12-20 12:11         ` Halil Pasic
2021-12-22 16:46 ` Cornelia Huck
2021-12-23 10:41   ` Halil Pasic
2021-12-23 11:12     ` Cornelia Huck
2022-01-05  8:42 ` Thomas Huth

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).