On 29.05.2017 18:35, Thomas Huth wrote:
> On 29.05.2017 14:17, David Hildenbrand wrote:
>> The pgm irq handler will detect unexpected pgm irqs and allows to
>> expect pgm irqs + verify that the pgm irq was triggered.
>>
>> We need "-fno-delete-null-pointer-checks", otherwise trying to access the
>> lowcore at address 0 makes GCC generate very weird code.
>
> I wonder whether you could get rid of that by using a global variable
> for lc instead?
As far as I remember, I tried that and it didn't change a thing. Will do
a quick test.
>
>> Add two tests to test for simple operation and addressing exception.
>>
>> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
>> ---
>> Note: This will require TCG patch "target/s390x: addressing exceptions are
>> suppressing" in order to pass.
>>
>> Patch "[kvm-unit-tests PATCH v1] s390x: generate asm offsets for the lowcore"
>> is requires as a prerequisite.
>
> Maybe send them together as a series next time?
Yes, will send them out in one series for the next round.
>
>> lib/s390x/asm-offsets.c | 1 +
>> lib/s390x/asm/arch_def.h | 59 +++++++++++++++++++++++++++++++-
>> lib/s390x/asm/irq.h | 18 ++++++++++
>> lib/s390x/irq.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>> s390x/Makefile | 2 ++
>> s390x/cstart64.S | 16 +++++++++
>> s390x/selftest.c | 14 ++++++++
>> 7 files changed, 198 insertions(+), 1 deletion(-)
>> create mode 100644 lib/s390x/asm/irq.h
>> create mode 100644 lib/s390x/irq.c
>>
>> diff --git a/lib/s390x/asm-offsets.c b/lib/s390x/asm-offsets.c
>> index 28915e5..cc357e9 100644
>> --- a/lib/s390x/asm-offsets.c
>> +++ b/lib/s390x/asm-offsets.c
>> @@ -54,6 +54,7 @@ int main(void)
>> OFFSET(GEN_LC_PGM_NEW_PSW, lowcore, pgm_new_psw);
>> OFFSET(GEN_LC_MCCK_NEW_PSW, lowcore, mcck_new_psw);
>> OFFSET(GEN_LC_IO_NEW_PSW, lowcore, io_new_psw);
>> + OFFSET(GEN_LC_SW_IRQ_AREA, lowcore, sw_irq_area);
>> OFFSET(GEN_LC_FPRS_SA, lowcore, fprs_sa);
>> OFFSET(GEN_LC_GRS_SA, lowcore, grs_sa);
>> OFFSET(GEN_LC_PSW_SA, lowcore, psw_sa);
>> diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h
>> index 25c9516..c49261a 100644
>> --- a/lib/s390x/asm/arch_def.h
>> +++ b/lib/s390x/asm/arch_def.h
>> @@ -64,7 +64,9 @@ struct lowcore {
>> struct psw pgm_new_psw; /* 0x01d0 */
>> struct psw mcck_new_psw; /* 0x01e0 */
>> struct psw io_new_psw; /* 0x01f0 */
>> - uint8_t pad_0x0200[0x1200 - 0x0200]; /* 0x0200 */
>> + /* sw definition: large enough to save all grs in irq handlers */
>> + uint8_t sw_irq_area[0x280 - 0x0200]; /* 0x0200 */
>
> Since you're saving 64-bit registers into this array, I'd suggest to
> make it an array of uint64_t values instead of uint8_t.
Ack. Also added save areas for fpc + fprs.
[...]>> /* entry point - for KVM + TCG we directly start in 64 bit mode */
>> @@ -21,6 +23,12 @@ start:
>> larl %r1, initital_psw
>> lpswe 0(%r1)
>> init_psw_cont:
>> + /* setup pgm irq handler */
>> + larl %r1, pgm_irq_mask
>> + stg %r1, GEN_LC_PGM_NEW_PSW
>> + larl %r1, pgm_irq
>> + stg %r1, GEN_LC_PGM_NEW_PSW + 8
>
> Not sure whether it's better/nicer, but I think you could also do:
I now do
larl %r1, pgm_irq_psw
mvc GEN_LC_PGM_NEW_PSW(16), 0(%r1)
>
> lmg %r0, %r1, pgm_irq_psw
> stmg %r0, %r1, GEN_LC_PGM_NEW_PSW
> ...
> pgm_irq_psw:
> .quad 0x0000000180000000, pgm_irq
>
>> + /* setup the initital PSW and enable 64bit addressing */
>
> That comment does not really match the code below?
Yes, wonder how that slipped in.
/* setup cr0, enabling e.g. AFP-register control */
>
>> larl %r1, initital_cr0
>> lctlg %c0, %c0, 0(%r1)
>> /* call setup() */
>> @@ -36,9 +44,17 @@ init_psw_cont:
>> /* call exit() */
>> j exit
>>
>> +pgm_irq:
>> + stmg %r0, %r15, GEN_LC_SW_IRQ_AREA
>> + brasl %r14, handle_pgm_irq
>> + lmg %r0, %r15, GEN_LC_SW_IRQ_AREA
>> + lpswe GEN_LC_PGM_OLD_PSW
>
> Use a TAB instead of space after lpswe ?
Ack.
>
> I also wonder whether you've got to save the floating point registers
> here, too, in case the compiler emits some floating point code in the
> interrupt handlers (since we're not using -msoftfloat anymore)?
Yes you're right. Saving/restoring fprs + fpc now. Unfortunately these
can't be saved/loaded in one shot like grs. For vector registers, there
is again an instruction to save/restore them in one shot.
>
>> .align 8
>> initital_psw:
>> .quad 0x0000000180000000, init_psw_cont
>> +pgm_irq_mask:
>> + .quad 0x0000000180000000
>> initital_cr0:
>> /* enable AFP-register control, so FP regs (+BFP instr) can be used */
>> .quad 0x0000000000040000
>> diff --git a/s390x/selftest.c b/s390x/selftest.c
>> index 4558e47..a67b87a 100644
>> --- a/s390x/selftest.c
>> +++ b/s390x/selftest.c
>> @@ -10,6 +10,7 @@
>> */
>> #include <libcflat.h>
>> #include <util.h>
>> +#include <asm/irq.h>
>>
>> static void test_fp(void)
>> {
>> @@ -25,6 +26,18 @@ static void test_fp(void)
>> report("3.0/2.0 == 1.5", c == 1.5);
>> }
>>
>> +static void test_pgm_irq(void)
>> +{
>> + expect_pgm_irq();
>> + asm volatile(
>> + " .insn e,0x0001");
>
> I'd put that into one line. And please add a comment - not everybody
> knows what opcode 0x0001 is good for.
Makes sense!
>
>> + report("OPERATION pgm irq", received_pgm_irq() == PGM_CODE_OPERATION);
>> +
>> + expect_pgm_irq();
>> + *((unsigned int*)-1) = 1;
>> + report("ADDRESSING pgm irq", received_pgm_irq() == PGM_CODE_ADDRESSING);
>> +}
>> +
>> int main(int argc, char**argv)
>> {
>> report_prefix_push("selftest");
>> @@ -36,6 +49,7 @@ int main(int argc, char**argv)
>> report("argv[2] == 123", !strcmp(argv[2], "123"));
>>
>> test_fp();
>> + test_pgm_irq();
>>
>> return report_summary();
>> }
>>
>
> Thomas
>
Thanks a lot Thomas!
--
Thanks,
David
