| Message ID | 20171204125505.29203-3-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x/tcg: CCW hotplug support | expand |
On Mon, 4 Dec 2017 13:55:02 +0100 David Hildenbrand <david@redhat.com> wrote: > The architecture mode indication wasn't stored. The split of certain > 64bit fields was unnecessary. Also, the complete clock comparator, not > just bit 0-55 (starting at byte 1) was stored. > > We now generate a proper MCIC via the same helper we use for KVM. > > While at it, also get rid of two local variables. There is be more to > clean up, but we will change the other parts later on either way. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/excp_helper.c | 18 ++++++++---------- > target/s390x/internal.h | 6 +++--- > 2 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c > index d831537544..840cf7641a 100644 > --- a/target/s390x/excp_helper.c > +++ b/target/s390x/excp_helper.c > @@ -369,7 +369,6 @@ static void do_io_interrupt(CPUS390XState *env) > static void do_mchk_interrupt(CPUS390XState *env) > { > S390CPU *cpu = s390_env_get_cpu(env); > - uint64_t mask, addr; > LowCore *lowcore; > MchkQueue *q; > int i; [BTW, there's also a CR 14 check in here that probably could use the new #define.] > @@ -395,6 +394,9 @@ static void do_mchk_interrupt(CPUS390XState *env) > > lowcore = cpu_map_lowcore(env); > > + /* we are always in z/Architecture mode */ > + lowcore->ar_access_id = 1; > + > for (i = 0; i < 16; i++) { > lowcore->floating_pt_save_area[i] = cpu_to_be64(get_freg(env, i)->ll); > lowcore->gpregs_save_area[i] = cpu_to_be64(env->regs[i]); > @@ -404,17 +406,12 @@ static void do_mchk_interrupt(CPUS390XState *env) > lowcore->prefixreg_save_area = cpu_to_be32(env->psa); > lowcore->fpt_creg_save_area = cpu_to_be32(env->fpc); > lowcore->tod_progreg_save_area = cpu_to_be32(env->todpr); > - lowcore->cpu_timer_save_area[0] = cpu_to_be32(env->cputm >> 32); > - lowcore->cpu_timer_save_area[1] = cpu_to_be32((uint32_t)env->cputm); > - lowcore->clock_comp_save_area[0] = cpu_to_be32(env->ckc >> 32); > - lowcore->clock_comp_save_area[1] = cpu_to_be32((uint32_t)env->ckc); > + lowcore->cpu_timer_save_area = cpu_to_be64(env->cputm); > + lowcore->clock_comp_save_area = cpu_to_be64(env->ckc >> 8); > > - lowcore->mcck_interruption_code[0] = cpu_to_be32(0x00400f1d); > - lowcore->mcck_interruption_code[1] = cpu_to_be32(0x40330000); > + lowcore->mcic = cpu_to_be64(s390_build_validity_mcic() | MCIC_SC_CP); Hm... I'm not sure that is a good idea (the nature of the helper, not that you remove the magic values). This function is called do_mchk_interrupt(), which sounds like a more generic thing. Maybe we need to enhance the machine check code to save the mcic etc. somewhere (after it has been generated) and just inject it here? Similar to what the kernel does. If you want to avoid rework, just add a TODO here? > lowcore->mcck_old_psw.mask = cpu_to_be64(get_psw_mask(env)); > lowcore->mcck_old_psw.addr = cpu_to_be64(env->psw.addr); > - mask = be64_to_cpu(lowcore->mcck_new_psw.mask); > - addr = be64_to_cpu(lowcore->mcck_new_psw.addr); > > cpu_unmap_lowcore(lowcore); >
On 04.12.2017 18:20, Cornelia Huck wrote: > On Mon, 4 Dec 2017 13:55:02 +0100 > David Hildenbrand <david@redhat.com> wrote: > >> The architecture mode indication wasn't stored. The split of certain >> 64bit fields was unnecessary. Also, the complete clock comparator, not >> just bit 0-55 (starting at byte 1) was stored. >> >> We now generate a proper MCIC via the same helper we use for KVM. >> >> While at it, also get rid of two local variables. There is be more to >> clean up, but we will change the other parts later on either way. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> target/s390x/excp_helper.c | 18 ++++++++---------- >> target/s390x/internal.h | 6 +++--- >> 2 files changed, 11 insertions(+), 13 deletions(-) >> >> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c >> index d831537544..840cf7641a 100644 >> --- a/target/s390x/excp_helper.c >> +++ b/target/s390x/excp_helper.c >> @@ -369,7 +369,6 @@ static void do_io_interrupt(CPUS390XState *env) >> static void do_mchk_interrupt(CPUS390XState *env) >> { >> S390CPU *cpu = s390_env_get_cpu(env); >> - uint64_t mask, addr; >> LowCore *lowcore; >> MchkQueue *q; >> int i; > > [BTW, there's also a CR 14 check in here that probably could use the > new #define.] Yes, will be gone with my floating IRQ rework, that's why I am not touching it here. (We will no longer track machine checks in a list but per cr14 sublcass - initially only crw). > >> @@ -395,6 +394,9 @@ static void do_mchk_interrupt(CPUS390XState *env) >> >> lowcore = cpu_map_lowcore(env); >> >> + /* we are always in z/Architecture mode */ >> + lowcore->ar_access_id = 1; >> + >> for (i = 0; i < 16; i++) { >> lowcore->floating_pt_save_area[i] = cpu_to_be64(get_freg(env, i)->ll); >> lowcore->gpregs_save_area[i] = cpu_to_be64(env->regs[i]); >> @@ -404,17 +406,12 @@ static void do_mchk_interrupt(CPUS390XState *env) >> lowcore->prefixreg_save_area = cpu_to_be32(env->psa); >> lowcore->fpt_creg_save_area = cpu_to_be32(env->fpc); >> lowcore->tod_progreg_save_area = cpu_to_be32(env->todpr); >> - lowcore->cpu_timer_save_area[0] = cpu_to_be32(env->cputm >> 32); >> - lowcore->cpu_timer_save_area[1] = cpu_to_be32((uint32_t)env->cputm); >> - lowcore->clock_comp_save_area[0] = cpu_to_be32(env->ckc >> 32); >> - lowcore->clock_comp_save_area[1] = cpu_to_be32((uint32_t)env->ckc); >> + lowcore->cpu_timer_save_area = cpu_to_be64(env->cputm); >> + lowcore->clock_comp_save_area = cpu_to_be64(env->ckc >> 8); >> >> - lowcore->mcck_interruption_code[0] = cpu_to_be32(0x00400f1d); >> - lowcore->mcck_interruption_code[1] = cpu_to_be32(0x40330000); >> + lowcore->mcic = cpu_to_be64(s390_build_validity_mcic() | MCIC_SC_CP); > > Hm... I'm not sure that is a good idea (the nature of the helper, not > that you remove the magic values). > > This function is called do_mchk_interrupt(), which sounds like a more > generic thing. Maybe we need to enhance the machine check code to save > the mcic etc. somewhere (after it has been generated) and just inject > it here? Similar to what the kernel does. > > If you want to avoid rework, just add a TODO here? Will also be gone with my floating IRQ rework ;) I can add a TODO if that helps, but 99.99996% it will be gone within 2-4 weeks. > >> lowcore->mcck_old_psw.mask = cpu_to_be64(get_psw_mask(env)); >> lowcore->mcck_old_psw.addr = cpu_to_be64(env->psw.addr); >> - mask = be64_to_cpu(lowcore->mcck_new_psw.mask); >> - addr = be64_to_cpu(lowcore->mcck_new_psw.addr); >> >> cpu_unmap_lowcore(lowcore); >>
On Mon, 4 Dec 2017 18:27:06 +0100 David Hildenbrand <david@redhat.com> wrote: > On 04.12.2017 18:20, Cornelia Huck wrote: > > On Mon, 4 Dec 2017 13:55:02 +0100 > > David Hildenbrand <david@redhat.com> wrote: > > > >> The architecture mode indication wasn't stored. The split of certain > >> 64bit fields was unnecessary. Also, the complete clock comparator, not > >> just bit 0-55 (starting at byte 1) was stored. > >> > >> We now generate a proper MCIC via the same helper we use for KVM. > >> > >> While at it, also get rid of two local variables. There is be more to > >> clean up, but we will change the other parts later on either way. > >> > >> Signed-off-by: David Hildenbrand <david@redhat.com> > >> --- > >> target/s390x/excp_helper.c | 18 ++++++++---------- > >> target/s390x/internal.h | 6 +++--- > >> 2 files changed, 11 insertions(+), 13 deletions(-) > >> > >> diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c > >> index d831537544..840cf7641a 100644 > >> --- a/target/s390x/excp_helper.c > >> +++ b/target/s390x/excp_helper.c > >> @@ -369,7 +369,6 @@ static void do_io_interrupt(CPUS390XState *env) > >> static void do_mchk_interrupt(CPUS390XState *env) > >> { > >> S390CPU *cpu = s390_env_get_cpu(env); > >> - uint64_t mask, addr; > >> LowCore *lowcore; > >> MchkQueue *q; > >> int i; > > > > [BTW, there's also a CR 14 check in here that probably could use the > > new #define.] > > Yes, will be gone with my floating IRQ rework, that's why I am not > touching it here. (We will no longer track machine checks in a list but > per cr14 sublcass - initially only crw). Sounds good. So let's just do the minimum to get this going and wait for the rework. > > > > >> @@ -395,6 +394,9 @@ static void do_mchk_interrupt(CPUS390XState *env) > >> > >> lowcore = cpu_map_lowcore(env); > >> > >> + /* we are always in z/Architecture mode */ > >> + lowcore->ar_access_id = 1; > >> + > >> for (i = 0; i < 16; i++) { > >> lowcore->floating_pt_save_area[i] = cpu_to_be64(get_freg(env, i)->ll); > >> lowcore->gpregs_save_area[i] = cpu_to_be64(env->regs[i]); > >> @@ -404,17 +406,12 @@ static void do_mchk_interrupt(CPUS390XState *env) > >> lowcore->prefixreg_save_area = cpu_to_be32(env->psa); > >> lowcore->fpt_creg_save_area = cpu_to_be32(env->fpc); > >> lowcore->tod_progreg_save_area = cpu_to_be32(env->todpr); > >> - lowcore->cpu_timer_save_area[0] = cpu_to_be32(env->cputm >> 32); > >> - lowcore->cpu_timer_save_area[1] = cpu_to_be32((uint32_t)env->cputm); > >> - lowcore->clock_comp_save_area[0] = cpu_to_be32(env->ckc >> 32); > >> - lowcore->clock_comp_save_area[1] = cpu_to_be32((uint32_t)env->ckc); > >> + lowcore->cpu_timer_save_area = cpu_to_be64(env->cputm); > >> + lowcore->clock_comp_save_area = cpu_to_be64(env->ckc >> 8); > >> > >> - lowcore->mcck_interruption_code[0] = cpu_to_be32(0x00400f1d); > >> - lowcore->mcck_interruption_code[1] = cpu_to_be32(0x40330000); > >> + lowcore->mcic = cpu_to_be64(s390_build_validity_mcic() | MCIC_SC_CP); > > > > Hm... I'm not sure that is a good idea (the nature of the helper, not > > that you remove the magic values). > > > > This function is called do_mchk_interrupt(), which sounds like a more > > generic thing. Maybe we need to enhance the machine check code to save > > the mcic etc. somewhere (after it has been generated) and just inject > > it here? Similar to what the kernel does. > > > > If you want to avoid rework, just add a TODO here? > > Will also be gone with my floating IRQ rework ;) I can add a TODO if > that helps, but 99.99996% it will be gone within 2-4 weeks. Whatever floats your interrupt. > > > > >> lowcore->mcck_old_psw.mask = cpu_to_be64(get_psw_mask(env)); > >> lowcore->mcck_old_psw.addr = cpu_to_be64(env->psw.addr); > >> - mask = be64_to_cpu(lowcore->mcck_new_psw.mask); > >> - addr = be64_to_cpu(lowcore->mcck_new_psw.addr); > >> > >> cpu_unmap_lowcore(lowcore); > >> > >
On 04.12.2017 13:55, David Hildenbrand wrote: > The architecture mode indication wasn't stored. The split of certain > 64bit fields was unnecessary. Also, the complete clock comparator, not > just bit 0-55 (starting at byte 1) was stored. > > We now generate a proper MCIC via the same helper we use for KVM. > > While at it, also get rid of two local variables. There is be more to > clean up, but we will change the other parts later on either way. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > target/s390x/excp_helper.c | 18 ++++++++---------- > target/s390x/internal.h | 6 +++--- > 2 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c > index d831537544..840cf7641a 100644 > --- a/target/s390x/excp_helper.c > +++ b/target/s390x/excp_helper.c > @@ -369,7 +369,6 @@ static void do_io_interrupt(CPUS390XState *env) > static void do_mchk_interrupt(CPUS390XState *env) > { > S390CPU *cpu = s390_env_get_cpu(env); > - uint64_t mask, addr; > LowCore *lowcore; > MchkQueue *q; > int i; > @@ -395,6 +394,9 @@ static void do_mchk_interrupt(CPUS390XState *env) > > lowcore = cpu_map_lowcore(env); > > + /* we are always in z/Architecture mode */ > + lowcore->ar_access_id = 1; > + > for (i = 0; i < 16; i++) { > lowcore->floating_pt_save_area[i] = cpu_to_be64(get_freg(env, i)->ll); > lowcore->gpregs_save_area[i] = cpu_to_be64(env->regs[i]); > @@ -404,17 +406,12 @@ static void do_mchk_interrupt(CPUS390XState *env) > lowcore->prefixreg_save_area = cpu_to_be32(env->psa); > lowcore->fpt_creg_save_area = cpu_to_be32(env->fpc); > lowcore->tod_progreg_save_area = cpu_to_be32(env->todpr); > - lowcore->cpu_timer_save_area[0] = cpu_to_be32(env->cputm >> 32); > - lowcore->cpu_timer_save_area[1] = cpu_to_be32((uint32_t)env->cputm); > - lowcore->clock_comp_save_area[0] = cpu_to_be32(env->ckc >> 32); > - lowcore->clock_comp_save_area[1] = cpu_to_be32((uint32_t)env->ckc); > + lowcore->cpu_timer_save_area = cpu_to_be64(env->cputm); > + lowcore->clock_comp_save_area = cpu_to_be64(env->ckc >> 8); > > - lowcore->mcck_interruption_code[0] = cpu_to_be32(0x00400f1d); > - lowcore->mcck_interruption_code[1] = cpu_to_be32(0x40330000); > + lowcore->mcic = cpu_to_be64(s390_build_validity_mcic() | MCIC_SC_CP); > lowcore->mcck_old_psw.mask = cpu_to_be64(get_psw_mask(env)); > lowcore->mcck_old_psw.addr = cpu_to_be64(env->psw.addr); > - mask = be64_to_cpu(lowcore->mcck_new_psw.mask); > - addr = be64_to_cpu(lowcore->mcck_new_psw.addr); > > cpu_unmap_lowcore(lowcore); Lowcore gets unmapped here... > @@ -426,7 +423,8 @@ static void do_mchk_interrupt(CPUS390XState *env) > DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__, > env->psw.mask, env->psw.addr); > > - load_psw(env, mask, addr); > + load_psw(env, be64_to_cpu(lowcore->mcck_new_psw.mask), > + be64_to_cpu(lowcore->mcck_new_psw.addr)); ... but here you still touch it? Looks like a bug to me...? Thomas
>> @@ -426,7 +423,8 @@ static void do_mchk_interrupt(CPUS390XState *env) >> DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__, >> env->psw.mask, env->psw.addr); >> >> - load_psw(env, mask, addr); >> + load_psw(env, be64_to_cpu(lowcore->mcck_new_psw.mask), >> + be64_to_cpu(lowcore->mcck_new_psw.addr)); > > ... but here you still touch it? Looks like a bug to me...? Indeed, didn't notice as the addresses won't change. Thanks! > > Thomas >
diff --git a/target/s390x/excp_helper.c b/target/s390x/excp_helper.c index d831537544..840cf7641a 100644 --- a/target/s390x/excp_helper.c +++ b/target/s390x/excp_helper.c @@ -369,7 +369,6 @@ static void do_io_interrupt(CPUS390XState *env) static void do_mchk_interrupt(CPUS390XState *env) { S390CPU *cpu = s390_env_get_cpu(env); - uint64_t mask, addr; LowCore *lowcore; MchkQueue *q; int i; @@ -395,6 +394,9 @@ static void do_mchk_interrupt(CPUS390XState *env) lowcore = cpu_map_lowcore(env); + /* we are always in z/Architecture mode */ + lowcore->ar_access_id = 1; + for (i = 0; i < 16; i++) { lowcore->floating_pt_save_area[i] = cpu_to_be64(get_freg(env, i)->ll); lowcore->gpregs_save_area[i] = cpu_to_be64(env->regs[i]); @@ -404,17 +406,12 @@ static void do_mchk_interrupt(CPUS390XState *env) lowcore->prefixreg_save_area = cpu_to_be32(env->psa); lowcore->fpt_creg_save_area = cpu_to_be32(env->fpc); lowcore->tod_progreg_save_area = cpu_to_be32(env->todpr); - lowcore->cpu_timer_save_area[0] = cpu_to_be32(env->cputm >> 32); - lowcore->cpu_timer_save_area[1] = cpu_to_be32((uint32_t)env->cputm); - lowcore->clock_comp_save_area[0] = cpu_to_be32(env->ckc >> 32); - lowcore->clock_comp_save_area[1] = cpu_to_be32((uint32_t)env->ckc); + lowcore->cpu_timer_save_area = cpu_to_be64(env->cputm); + lowcore->clock_comp_save_area = cpu_to_be64(env->ckc >> 8); - lowcore->mcck_interruption_code[0] = cpu_to_be32(0x00400f1d); - lowcore->mcck_interruption_code[1] = cpu_to_be32(0x40330000); + lowcore->mcic = cpu_to_be64(s390_build_validity_mcic() | MCIC_SC_CP); lowcore->mcck_old_psw.mask = cpu_to_be64(get_psw_mask(env)); lowcore->mcck_old_psw.addr = cpu_to_be64(env->psw.addr); - mask = be64_to_cpu(lowcore->mcck_new_psw.mask); - addr = be64_to_cpu(lowcore->mcck_new_psw.addr); cpu_unmap_lowcore(lowcore); @@ -426,7 +423,8 @@ static void do_mchk_interrupt(CPUS390XState *env) DPRINTF("%s: %" PRIx64 " %" PRIx64 "\n", __func__, env->psw.mask, env->psw.addr); - load_psw(env, mask, addr); + load_psw(env, be64_to_cpu(lowcore->mcck_new_psw.mask), + be64_to_cpu(lowcore->mcck_new_psw.addr)); } void s390_cpu_do_interrupt(CPUState *cs) diff --git a/target/s390x/internal.h b/target/s390x/internal.h index 6817b2c432..1a88e4beb4 100644 --- a/target/s390x/internal.h +++ b/target/s390x/internal.h @@ -43,7 +43,7 @@ typedef struct LowCore { uint8_t pad3[0xc8 - 0xc4]; /* 0x0c4 */ uint32_t stfl_fac_list; /* 0x0c8 */ uint8_t pad4[0xe8 - 0xcc]; /* 0x0cc */ - uint32_t mcck_interruption_code[2]; /* 0x0e8 */ + uint64_t mcic; /* 0x0e8 */ uint8_t pad5[0xf4 - 0xf0]; /* 0x0f0 */ uint32_t external_damage_code; /* 0x0f4 */ uint64_t failing_storage_address; /* 0x0f8 */ @@ -118,8 +118,8 @@ typedef struct LowCore { uint32_t fpt_creg_save_area; /* 0x131c */ uint8_t pad16[0x1324 - 0x1320]; /* 0x1320 */ uint32_t tod_progreg_save_area; /* 0x1324 */ - uint32_t cpu_timer_save_area[2]; /* 0x1328 */ - uint32_t clock_comp_save_area[2]; /* 0x1330 */ + uint64_t cpu_timer_save_area; /* 0x1328 */ + uint64_t clock_comp_save_area; /* 0x1330 */ uint8_t pad17[0x1340 - 0x1338]; /* 0x1338 */ uint32_t access_regs_save_area[16]; /* 0x1340 */ uint64_t cregs_save_area[16]; /* 0x1380 */
The architecture mode indication wasn't stored. The split of certain 64bit fields was unnecessary. Also, the complete clock comparator, not just bit 0-55 (starting at byte 1) was stored. We now generate a proper MCIC via the same helper we use for KVM. While at it, also get rid of two local variables. There is be more to clean up, but we will change the other parts later on either way. Signed-off-by: David Hildenbrand <david@redhat.com> --- target/s390x/excp_helper.c | 18 ++++++++---------- target/s390x/internal.h | 6 +++--- 2 files changed, 11 insertions(+), 13 deletions(-)