Re: [kvm-unit-tests PATCH 4/8] lib: s390x: Start using bitops instead of magic constants
From: Thomas Huth <hidden>
Date: 2021-08-18 09:24:57
Also in:
kvm
On 13/08/2021 09.36, Janosch Frank wrote:
quoted hunk ↗ jump to hunk
TEID data is specified in the Principles of Operation as bits so it makes more sens to test the bits instead of anding the mask. We need to set -Wno-address-of-packed-member since for test bit we take an address of a struct lowcore member. Signed-off-by: Janosch Frank <frankja@linux.ibm.com> --- lib/s390x/interrupt.c | 5 +++-- s390x/Makefile | 1 + 2 files changed, 4 insertions(+), 2 deletions(-)diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c index 1248bceb..e05c212e 100644 --- a/lib/s390x/interrupt.c +++ b/lib/s390x/interrupt.c@@ -8,6 +8,7 @@ * David Hildenbrand <david@redhat.com> */ #include <libcflat.h> +#include <bitops.h> #include <asm/barrier.h> #include <sclp.h> #include <interrupt.h>@@ -77,8 +78,8 @@ static void fixup_pgm_int(struct stack_frame_int *stack) break; case PGM_INT_CODE_PROTECTION: /* Handling for iep.c test case. */ - if (lc->trans_exc_id & 0x80UL && lc->trans_exc_id & 0x04UL && - !(lc->trans_exc_id & 0x08UL)) + if (test_bit_inv(56, &lc->trans_exc_id) && test_bit_inv(61, &lc->trans_exc_id) && + !test_bit_inv(60, &lc->trans_exc_id))
I'd rather prefer:
if ((lc->trans_exc_id & 0x8c) == 0x84)
... or maybe you could add a helper function for these checks a la:
bool check_teid_prot_cause(uint64_t teid, bool bit56, bool bit60,
bool bit61)
then you could replace the if statement with:
if (check_teid_prot_cause(lc->trans_exc_id, 1, 0, 1))
which would be way more readable, IMHO.
quoted hunk ↗ jump to hunk
/* * We branched to the instruction that caused * the exception so we can use the returndiff --git a/s390x/Makefile b/s390x/Makefile index ef8041a6..d260b336 100644 --- a/s390x/Makefile +++ b/s390x/Makefile@@ -45,6 +45,7 @@ CFLAGS += -O2 CFLAGS += -march=zEC12 CFLAGS += -mbackchain CFLAGS += -fno-delete-null-pointer-checks +CFLAGS += -Wno-address-of-packed-member
I think we should avoid this since this also affects the common code, doesn't it? And in common code, we might need to deal with this. Thomas