| Message ID | 20220803135851.384805-2-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: verify EQBS/SQBS is unavailable | expand |
On Wed, 3 Aug 2022 15:58:51 +0200 Nico Boehr <nrb@linux.ibm.com> wrote: > QEMU doesn't provide EQBS/SQBS instructions, so we should check they > result in an exception. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > s390x/intercept.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/s390x/intercept.c b/s390x/intercept.c > index 9e826b6c79ad..48eb2d22a2cc 100644 > --- a/s390x/intercept.c > +++ b/s390x/intercept.c > @@ -197,6 +197,34 @@ static void test_diag318(void) > > } > > +static void test_qbs(void) > +{ > + report_prefix_push("qbs"); > + if (!host_is_qemu()) { > + report_skip("QEMU-only test"); > + report_prefix_pop(); > + return; > + } > + > + report_prefix_push("sqbs"); > + expect_pgm_int(); > + asm volatile( > + " .insn rsy,0xeb000000008a,0,0,0(0)\n" > + : : : "memory", "cc"); > + check_pgm_int_code(PGM_INT_CODE_OPERATION); > + report_prefix_pop(); > + > + report_prefix_push("eqbs"); > + expect_pgm_int(); > + asm volatile( > + " .insn rrf,0xb99c0000,0,0,0,0\n" > + : : : "memory", "cc"); > + check_pgm_int_code(PGM_INT_CODE_OPERATION); > + report_prefix_pop(); > + > + report_prefix_pop(); > +} > + > struct { > const char *name; > void (*func)(void); > @@ -208,6 +236,7 @@ struct { > { "stidp", test_stidp, false }, > { "testblock", test_testblock, false }, > { "diag318", test_diag318, false }, > + { "qbs", test_qbs, false }, > { NULL, NULL, false } > }; >
On 03/08/2022 15.58, Nico Boehr wrote: > QEMU doesn't provide EQBS/SQBS instructions, so we should check they > result in an exception. I somewhat fail to see the exact purpose of this patch... QEMU still doesn't emulate a lot of other instructions, too, so why are we checking now these QBS instructions? Why not all the others? Why do we need a test to verify that there is an exception in this case - was there a bug somewhere that didn't cause an exception in certain circumstances? > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > s390x/intercept.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/s390x/intercept.c b/s390x/intercept.c > index 9e826b6c79ad..48eb2d22a2cc 100644 > --- a/s390x/intercept.c > +++ b/s390x/intercept.c > @@ -197,6 +197,34 @@ static void test_diag318(void) > > } > > +static void test_qbs(void) > +{ > + report_prefix_push("qbs"); You should definitely add a comment here, explaining why this is only a test for QEMU and saying that this could be removed as soon as QEMU implements these instructions later - otherwise this would be very confusing to the readers later (if they forget or cannot check the commit message). > + if (!host_is_qemu()) { > + report_skip("QEMU-only test"); > + report_prefix_pop(); > + return; > + } > + > + report_prefix_push("sqbs"); > + expect_pgm_int(); > + asm volatile( > + " .insn rsy,0xeb000000008a,0,0,0(0)\n" > + : : : "memory", "cc"); > + check_pgm_int_code(PGM_INT_CODE_OPERATION); > + report_prefix_pop(); > + > + report_prefix_push("eqbs"); > + expect_pgm_int(); > + asm volatile( > + " .insn rrf,0xb99c0000,0,0,0,0\n" > + : : : "memory", "cc"); > + check_pgm_int_code(PGM_INT_CODE_OPERATION); > + report_prefix_pop(); > + > + report_prefix_pop(); > +} Thomas
Quoting Thomas Huth (2022-08-04 00:17:38) > On 03/08/2022 15.58, Nico Boehr wrote: > > QEMU doesn't provide EQBS/SQBS instructions, so we should check they > > result in an exception. > > I somewhat fail to see the exact purpose of this patch... QEMU still doesn't > emulate a lot of other instructions, too, so why are we checking now these > QBS instructions? I agree with you, it certainly doesn't make sense to test all kinds of random instructions that aren't implemented in QEMU. But, for the QBS instructions, there is a special case in handle_b9 and handle_eb in QEMU. I would argue since there is code for it, there can be (or even should be) tests for it. But if you guys say it is not worth having this test, this is fine for me as well. [...] > > diff --git a/s390x/intercept.c b/s390x/intercept.c > > index 9e826b6c79ad..48eb2d22a2cc 100644 > > --- a/s390x/intercept.c > > +++ b/s390x/intercept.c > > @@ -197,6 +197,34 @@ static void test_diag318(void) > > > > } > > > > +static void test_qbs(void) > > +{ > > + report_prefix_push("qbs"); > > You should definitely add a comment here, explaining why this is only a test > for QEMU and saying that this could be removed as soon as QEMU implements > these instructions later - otherwise this would be very confusing to the > readers later (if they forget or cannot check the commit message). OK, I can add this once we have an answer to your first question.
On 8/4/22 00:17, Thomas Huth wrote: > On 03/08/2022 15.58, Nico Boehr wrote: >> QEMU doesn't provide EQBS/SQBS instructions, so we should check they >> result in an exception. > > I somewhat fail to see the exact purpose of this patch... QEMU still doesn't > emulate a lot of other instructions, too, so why are we checking now these > QBS instructions? Why not all the others? Why do we need a test to verify > that there is an exception in this case - was there a bug somewhere that > didn't cause an exception in certain circumstances? Looking at the patch that introduced the QEMU handlers (1eecf41b) I wonder why those two cases were added. From my point of view it makes sense to remove the special handling for those two instructions. @Christian: Any idea why this was added? Can we remove it? The only reason I can think of to test this is the existence of EC* bits that control the behavior for those instructions. So if we set those without having QEMU handling code then we're in trouble. But then I'd also expect that we need to set a stfle bit to indicate the availability and this test doesn't check for that and would indicate a false-positive.
On 24/08/2022 09.40, Janosch Frank wrote: > On 8/4/22 00:17, Thomas Huth wrote: >> On 03/08/2022 15.58, Nico Boehr wrote: >>> QEMU doesn't provide EQBS/SQBS instructions, so we should check they >>> result in an exception. >> >> I somewhat fail to see the exact purpose of this patch... QEMU still doesn't >> emulate a lot of other instructions, too, so why are we checking now these >> QBS instructions? Why not all the others? Why do we need a test to verify >> that there is an exception in this case - was there a bug somewhere that >> didn't cause an exception in certain circumstances? > > Looking at the patch that introduced the QEMU handlers (1eecf41b) I wonder > why those two cases were added. From my point of view it makes sense to > remove the special handling for those two instructions. Ah, there are handlers for these instructions in QEMU - that's what I was missing. But I agree with you, these do not look very useful and should just be dropped on the QEMU side. Thomas
Am 24.08.22 um 09:40 schrieb Janosch Frank: > On 8/4/22 00:17, Thomas Huth wrote: >> On 03/08/2022 15.58, Nico Boehr wrote: >>> QEMU doesn't provide EQBS/SQBS instructions, so we should check they >>> result in an exception. >> >> I somewhat fail to see the exact purpose of this patch... QEMU still doesn't >> emulate a lot of other instructions, too, so why are we checking now these >> QBS instructions? Why not all the others? Why do we need a test to verify >> that there is an exception in this case - was there a bug somewhere that >> didn't cause an exception in certain circumstances? > > Looking at the patch that introduced the QEMU handlers (1eecf41b) I wonder why those two cases were added. From my point of view it makes sense to remove the special handling for those two instructions. > > @Christian: Any idea why this was added? Can we remove it? No idea why it was added (in an always fail way). Yes we could remove it until we need it.
diff --git a/s390x/intercept.c b/s390x/intercept.c index 9e826b6c79ad..48eb2d22a2cc 100644 --- a/s390x/intercept.c +++ b/s390x/intercept.c @@ -197,6 +197,34 @@ static void test_diag318(void) } +static void test_qbs(void) +{ + report_prefix_push("qbs"); + if (!host_is_qemu()) { + report_skip("QEMU-only test"); + report_prefix_pop(); + return; + } + + report_prefix_push("sqbs"); + expect_pgm_int(); + asm volatile( + " .insn rsy,0xeb000000008a,0,0,0(0)\n" + : : : "memory", "cc"); + check_pgm_int_code(PGM_INT_CODE_OPERATION); + report_prefix_pop(); + + report_prefix_push("eqbs"); + expect_pgm_int(); + asm volatile( + " .insn rrf,0xb99c0000,0,0,0,0\n" + : : : "memory", "cc"); + check_pgm_int_code(PGM_INT_CODE_OPERATION); + report_prefix_pop(); + + report_prefix_pop(); +} + struct { const char *name; void (*func)(void); @@ -208,6 +236,7 @@ struct { { "stidp", test_stidp, false }, { "testblock", test_testblock, false }, { "diag318", test_diag318, false }, + { "qbs", test_qbs, false }, { NULL, NULL, false } };
QEMU doesn't provide EQBS/SQBS instructions, so we should check they result in an exception. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- s390x/intercept.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)