| Message ID | 20230601070202.152094-2-nrb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: Add support for running guests without MSO/MSL | expand |
On 6/1/23 09:01, Nico Boehr wrote: > Changing the PSW mask is currently little clumsy, since there is only the > PSW_MASK_* defines. This makes it hard to change e.g. only the address > space in the current PSW without a lot of bit fiddling. > > Introduce a bitfield for the PSW mask. This makes this kind of > modifications much simpler and easier to read. > > Signed-off-by: Nico Boehr <nrb@linux.ibm.com> > --- > lib/s390x/asm/arch_def.h | 25 ++++++++++++++++++++++++- > s390x/selftest.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+), 1 deletion(-) > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > index bb26e008cc68..84f6996c4d8c 100644 > --- a/lib/s390x/asm/arch_def.h > +++ b/lib/s390x/asm/arch_def.h > @@ -37,12 +37,35 @@ struct stack_frame_int { > }; > > struct psw { > - uint64_t mask; > + union { > + uint64_t mask; > + struct { > + uint8_t reserved00:1; > + uint8_t per:1; > + uint8_t reserved02:3; > + uint8_t dat:1; > + uint8_t io:1; > + uint8_t ext:1; > + uint8_t key:4; > + uint8_t reserved12:1; > + uint8_t mchk:1; > + uint8_t wait:1; > + uint8_t pstate:1; > + uint8_t as:2; > + uint8_t cc:2; > + uint8_t prg_mask:4; > + uint8_t reserved24:7; > + uint8_t ea:1; > + uint8_t ba:1; > + uint32_t reserved33:31; Hrm, since I already made the mistake of introducing bitfields with and without spaces between the ":" I'm in no position to complain here. I'm also not sure what the consensus is. > + }; > + }; > uint64_t addr; > }; I've come to like static asserts for huge structs and bitfields since they can safe you from a *lot* of headaches. > > #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) }) > > + Whitespace damage > struct short_psw { > uint32_t mask; > uint32_t addr; > diff --git a/s390x/selftest.c b/s390x/selftest.c > index 13fd36bc06f8..8d81ba312279 100644 > --- a/s390x/selftest.c > +++ b/s390x/selftest.c > @@ -74,6 +74,45 @@ static void test_malloc(void) > report_prefix_pop(); > } > > +static void test_psw_mask(void) > +{ > + uint64_t expected_key = 0xF; We're using lowercase chars for hex constants > + struct psw test_psw = PSW(0, 0); > + > + report_prefix_push("PSW mask"); > + test_psw.dat = 1; > + report(test_psw.mask == PSW_MASK_DAT, "DAT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_DAT, test_psw.mask); > + > + test_psw.mask = 0; > + test_psw.io = 1; > + report(test_psw.mask == PSW_MASK_IO, "IO matches expected=0x%016lx actual=0x%016lx", PSW_MASK_IO, test_psw.mask); > + > + test_psw.mask = 0; > + test_psw.ext = 1; > + report(test_psw.mask == PSW_MASK_EXT, "EXT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_EXT, test_psw.mask); > + > + test_psw.mask = expected_key << (63 - 11); > + report(test_psw.key == expected_key, "PSW Key matches expected=0x%lx actual=0x%x", expected_key, test_psw.key); > + > + test_psw.mask = 1UL << (63 - 13); > + report(test_psw.mchk, "MCHK matches"); > + > + test_psw.mask = 0; > + test_psw.wait = 1; > + report(test_psw.mask == PSW_MASK_WAIT, "Wait matches expected=0x%016lx actual=0x%016lx", PSW_MASK_WAIT, test_psw.mask); > + > + test_psw.mask = 0; > + test_psw.pstate = 1; > + report(test_psw.mask == PSW_MASK_PSTATE, "Pstate matches expected=0x%016lx actual=0x%016lx", PSW_MASK_PSTATE, test_psw.mask); > + > + test_psw.mask = 0; > + test_psw.ea = 1; > + test_psw.ba = 1; > + report(test_psw.mask == PSW_MASK_64, "BA/EA matches expected=0x%016lx actual=0x%016lx", PSW_MASK_64, test_psw.mask); > + > + report_prefix_pop(); > +} > + > int main(int argc, char**argv) > { > report_prefix_push("selftest"); > @@ -89,6 +128,7 @@ int main(int argc, char**argv) > test_fp(); > test_pgm_int(); > test_malloc(); > + test_psw_mask(); > > return report_summary(); > }
On Thu, 1 Jun 2023 09:42:48 +0200 Janosch Frank <frankja@linux.ibm.com> wrote: [...] > Hrm, since I already made the mistake of introducing bitfields with and > without spaces between the ":" I'm in no position to complain here. > > I'm also not sure what the consensus is. tbh I don't really have an opinion, I don't mind either, to the point that I don't even care if we mix them > > > + }; > > + }; > > uint64_t addr; > > }; > > I've come to like static asserts for huge structs and bitfields since > they can safe you from a *lot* of headaches. you mean statically asserting that the size is what it should be? in that case fully agree [...]
On 6/5/23 12:35, Claudio Imbrenda wrote: > On Thu, 1 Jun 2023 09:42:48 +0200 > Janosch Frank <frankja@linux.ibm.com> wrote: > > [...] > >> Hrm, since I already made the mistake of introducing bitfields with and >> without spaces between the ":" I'm in no position to complain here. >> >> I'm also not sure what the consensus is. > > tbh I don't really have an opinion, I don't mind either, to the point > that I don't even care if we mix them > >> >>> + }; >>> + }; >>> uint64_t addr; >>> }; >> >> I've come to like static asserts for huge structs and bitfields since >> they can safe you from a *lot* of headaches. > > you mean statically asserting that the size is what it should be? > in that case fully agree > Yes, asserting the size.
Quoting Janosch Frank (2023-06-01 09:42:48) [...] > I've come to like static asserts for huge structs and bitfields since > they can safe you from a *lot* of headaches. I generally agree and I add a _Static_assert but I want to mention the usefulness is a bit limited in this case, since we have a bitfield inside a union. So it only really helps if you manage to exceed the size of mask. There really is no way around the stuff I put in the selftests. I could of course try to make that code _Static_asserts but it will not be pretty.
On Wed, 07 Jun 2023 17:56:13 +0200 Nico Boehr <nrb@linux.ibm.com> wrote: > Quoting Janosch Frank (2023-06-01 09:42:48) > [...] > > I've come to like static asserts for huge structs and bitfields since > > they can safe you from a *lot* of headaches. > > I generally agree and I add a _Static_assert but I want to mention the > usefulness is a bit limited in this case, since we have a bitfield inside a > union. So it only really helps if you manage to exceed the size of mask. better than nothing :) if the struct becomes too big, the assert will catch it > > There really is no way around the stuff I put in the selftests. > > I could of course try to make that code _Static_asserts but it will not be > pretty. I think just a couple of asserts to make sure things aren't __too__ crazy would be good enough
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h index bb26e008cc68..84f6996c4d8c 100644 --- a/lib/s390x/asm/arch_def.h +++ b/lib/s390x/asm/arch_def.h @@ -37,12 +37,35 @@ struct stack_frame_int { }; struct psw { - uint64_t mask; + union { + uint64_t mask; + struct { + uint8_t reserved00:1; + uint8_t per:1; + uint8_t reserved02:3; + uint8_t dat:1; + uint8_t io:1; + uint8_t ext:1; + uint8_t key:4; + uint8_t reserved12:1; + uint8_t mchk:1; + uint8_t wait:1; + uint8_t pstate:1; + uint8_t as:2; + uint8_t cc:2; + uint8_t prg_mask:4; + uint8_t reserved24:7; + uint8_t ea:1; + uint8_t ba:1; + uint32_t reserved33:31; + }; + }; uint64_t addr; }; #define PSW(m, a) ((struct psw){ .mask = (m), .addr = (uint64_t)(a) }) + struct short_psw { uint32_t mask; uint32_t addr; diff --git a/s390x/selftest.c b/s390x/selftest.c index 13fd36bc06f8..8d81ba312279 100644 --- a/s390x/selftest.c +++ b/s390x/selftest.c @@ -74,6 +74,45 @@ static void test_malloc(void) report_prefix_pop(); } +static void test_psw_mask(void) +{ + uint64_t expected_key = 0xF; + struct psw test_psw = PSW(0, 0); + + report_prefix_push("PSW mask"); + test_psw.dat = 1; + report(test_psw.mask == PSW_MASK_DAT, "DAT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_DAT, test_psw.mask); + + test_psw.mask = 0; + test_psw.io = 1; + report(test_psw.mask == PSW_MASK_IO, "IO matches expected=0x%016lx actual=0x%016lx", PSW_MASK_IO, test_psw.mask); + + test_psw.mask = 0; + test_psw.ext = 1; + report(test_psw.mask == PSW_MASK_EXT, "EXT matches expected=0x%016lx actual=0x%016lx", PSW_MASK_EXT, test_psw.mask); + + test_psw.mask = expected_key << (63 - 11); + report(test_psw.key == expected_key, "PSW Key matches expected=0x%lx actual=0x%x", expected_key, test_psw.key); + + test_psw.mask = 1UL << (63 - 13); + report(test_psw.mchk, "MCHK matches"); + + test_psw.mask = 0; + test_psw.wait = 1; + report(test_psw.mask == PSW_MASK_WAIT, "Wait matches expected=0x%016lx actual=0x%016lx", PSW_MASK_WAIT, test_psw.mask); + + test_psw.mask = 0; + test_psw.pstate = 1; + report(test_psw.mask == PSW_MASK_PSTATE, "Pstate matches expected=0x%016lx actual=0x%016lx", PSW_MASK_PSTATE, test_psw.mask); + + test_psw.mask = 0; + test_psw.ea = 1; + test_psw.ba = 1; + report(test_psw.mask == PSW_MASK_64, "BA/EA matches expected=0x%016lx actual=0x%016lx", PSW_MASK_64, test_psw.mask); + + report_prefix_pop(); +} + int main(int argc, char**argv) { report_prefix_push("selftest"); @@ -89,6 +128,7 @@ int main(int argc, char**argv) test_fp(); test_pgm_int(); test_malloc(); + test_psw_mask(); return report_summary(); }
Changing the PSW mask is currently little clumsy, since there is only the PSW_MASK_* defines. This makes it hard to change e.g. only the address space in the current PSW without a lot of bit fiddling. Introduce a bitfield for the PSW mask. This makes this kind of modifications much simpler and easier to read. Signed-off-by: Nico Boehr <nrb@linux.ibm.com> --- lib/s390x/asm/arch_def.h | 25 ++++++++++++++++++++++++- s390x/selftest.c | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-)