| Message ID | 20230712114149.1291580-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 12/07/2023 13.41, 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 | 26 +++++++++++++++++++++++++- > s390x/selftest.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 65 insertions(+), 1 deletion(-) > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > index bb26e008cc68..53279572a9ee 100644 > --- a/lib/s390x/asm/arch_def.h > +++ b/lib/s390x/asm/arch_def.h > @@ -37,12 +37,36 @@ 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; > }; > +_Static_assert(sizeof(struct psw) == 16, "PSW size"); > > #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); Patch looks basically fine to me, but here my mind stumbled a little bit. This test is written the other way round than the others. Nothing wrong with that, it just feels a little bit inconsistent. I'd suggest to either do: test_psw.mask = 0; test_psw.key = expected_key; report(test_psw.mask == expected_key << (63 - 11), ...); or maybe even switch all the other tests around instead, so you could get rid of the "test_psw.mask = 0" lines, e.g. : test_psw.mask == PSW_MASK_IO; report(test_psw.io, "IO matches ..."); etc. Thomas
On 13/07/2023 08.56, Thomas Huth wrote: > On 12/07/2023 13.41, 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 | 26 +++++++++++++++++++++++++- >> s390x/selftest.c | 40 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 65 insertions(+), 1 deletion(-) >> >> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h >> index bb26e008cc68..53279572a9ee 100644 >> --- a/lib/s390x/asm/arch_def.h >> +++ b/lib/s390x/asm/arch_def.h >> @@ -37,12 +37,36 @@ 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; >> }; >> +_Static_assert(sizeof(struct psw) == 16, "PSW size"); >> #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); > > Patch looks basically fine to me, but here my mind stumbled a little bit. > This test is written the other way round than the others. Nothing wrong with > that, it just feels a little bit inconsistent. I'd suggest to either do: > > test_psw.mask = 0; > test_psw.key = expected_key; > report(test_psw.mask == expected_key << (63 - 11), ...); > > or maybe even switch all the other tests around instead, so you could get > rid of the "test_psw.mask = 0" lines, e.g. : > > test_psw.mask == PSW_MASK_IO; s/==/=/ of course! Sorry for that typo. Thomas > report(test_psw.io, "IO matches ..."); > > etc. > > Thomas >
On Wed, 12 Jul 2023 13:41:44 +0200 Nico Boehr <nrb@linux.ibm.com> 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 | 26 +++++++++++++++++++++++++- > s390x/selftest.c | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 65 insertions(+), 1 deletion(-) > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > index bb26e008cc68..53279572a9ee 100644 > --- a/lib/s390x/asm/arch_def.h > +++ b/lib/s390x/asm/arch_def.h > @@ -37,12 +37,36 @@ 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; since you will need to respin this anyway: can you use uint64_t everywhere in the bitfield? it would look more consistent and it would avoid some potential pitfalls of using bitfields. In our case we're lucky because all fields are properly aligned, but using uint64_t makes it easier to understand. > + }; > + }; > uint64_t addr; > }; > +_Static_assert(sizeof(struct psw) == 16, "PSW size"); > > #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; I think we prefer lowercase hex > + 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(); > }
Quoting Thomas Huth (2023-07-13 08:56:41) > On 12/07/2023 13.41, 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 | 26 +++++++++++++++++++++++++- > > s390x/selftest.c | 40 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 65 insertions(+), 1 deletion(-) > > > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > > index bb26e008cc68..53279572a9ee 100644 > > --- a/lib/s390x/asm/arch_def.h > > +++ b/lib/s390x/asm/arch_def.h > > @@ -37,12 +37,36 @@ 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; > > }; > > +_Static_assert(sizeof(struct psw) == 16, "PSW size"); > > > > #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); > > Patch looks basically fine to me, but here my mind stumbled a little bit. > This test is written the other way round than the others. Nothing wrong with > that, it just feels a little bit inconsistent. I'd suggest to either do: > > test_psw.mask = 0; > test_psw.key = expected_key; > report(test_psw.mask == expected_key << (63 - 11), ...); > > or maybe even switch all the other tests around instead, so you could get > rid of the "test_psw.mask = 0" lines, e.g. : > > test_psw.mask == PSW_MASK_IO; > report(test_psw.io, "IO matches ..."); > > etc. I like the latter option, thanks.
Quoting Claudio Imbrenda (2023-07-13 10:20:13) [...] > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > > index bb26e008cc68..53279572a9ee 100644 > > --- a/lib/s390x/asm/arch_def.h > > +++ b/lib/s390x/asm/arch_def.h > > @@ -37,12 +37,36 @@ 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; > > since you will need to respin this anyway: > > can you use uint64_t everywhere in the bitfield? it would look more > consistent and it would avoid some potential pitfalls of using > bitfields. In our case we're lucky because all fields are properly > aligned, but using uint64_t makes it easier to understand. I will do that for consistency, I'd be interested to understand what pitfalls you're talking about and how uint64_t avoids them, since we have a few bitfields in the codebase.
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h index bb26e008cc68..53279572a9ee 100644 --- a/lib/s390x/asm/arch_def.h +++ b/lib/s390x/asm/arch_def.h @@ -37,12 +37,36 @@ 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; }; +_Static_assert(sizeof(struct psw) == 16, "PSW size"); #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 | 26 +++++++++++++++++++++++++- s390x/selftest.c | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+), 1 deletion(-)