Re: [Qemu-devel] [PATCH v1 for-2.12 1/5] s390x/kvm: factor out build_channel_report_mcic() into cpu.h

[Cornelia Huck <cohuck@redhat.com>]

On Mon, 4 Dec 2017 18:16:15 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 04.12.2017 18:03, Cornelia Huck wrote:
> > On Mon,  4 Dec 2017 13:55:01 +0100
> > David Hildenbrand <david@redhat.com> wrote:
> >   
> >> We'll need it later on in two places. Refactor it to just indicate the
> >> valid bit. While at it, introduce a define for the used CR14 bit (we'll  
> > 
> > s/valid bit/validity bits/  
> 
> In the PoP they are defined as "validity" (e.g. 11-15)
> 
> Vector-register validity (VR)
> External-damage-code validity (EC)
> Floating-point-register validity (FP)
> General-register validity (GR)
> Control-register validity (CR)
> 
> So I am not sure if using a slightly different terminology here helps.

I don't know, "validity" seems to be more in line with the doc?

> 
> >   
> >> also need later on).
> >>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  target/s390x/cpu.h | 20 ++++++++++++++++++++
> >>  target/s390x/kvm.c | 25 ++-----------------------
> >>  2 files changed, 22 insertions(+), 23 deletions(-)
> >>
> >> diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
> >> index 9cfbbbac04..4d3e7920f5 100644
> >> --- a/target/s390x/cpu.h
> >> +++ b/target/s390x/cpu.h
> >> @@ -351,6 +351,9 @@ extern const struct VMStateDescription vmstate_s390_cpu;
> >>  #define CR0_CPU_TIMER_SC        0x0000000000000400ULL
> >>  #define CR0_SERVICE_SC          0x0000000000000200ULL
> >>  
> >> +/* Control register 14 bits */
> >> +#define CR14_CHANNEL_REPORT_SC  0x0000000010000000ULL
> >> +
> >>  /* MMU */
> >>  #define MMU_PRIMARY_IDX         0
> >>  #define MMU_SECONDARY_IDX       1
> >> @@ -674,6 +677,23 @@ struct sysib_322 {
> >>  #define MCIC_VB_CT 0x0000000000020000ULL
> >>  #define MCIC_VB_CC 0x0000000000010000ULL
> >>  
> >> +static inline uint64_t s390_build_validity_mcic(void)
> >> +{
> >> +    uint64_t mcic;
> >> +
> >> +    /* indicate all valid bits (no damage) */  
> > 
> > s/valid/validity/  
> 
> dito

dito :)

> 
> >   
> >> +    mcic = MCIC_VB_WP | MCIC_VB_MS | MCIC_VB_PM | MCIC_VB_IA | MCIC_VB_FP |
> >> +           MCIC_VB_GR | MCIC_VB_CR | MCIC_VB_ST | MCIC_VB_AR | MCIC_VB_PR |
> >> +           MCIC_VB_FC | MCIC_VB_CT | MCIC_VB_CC;
> >> +    if (s390_has_feat(S390_FEAT_VECTOR)) {
> >> +        mcic |= MCIC_VB_VR;
> >> +    }
> >> +    if (s390_has_feat(S390_FEAT_GUARDED_STORAGE)) {
> >> +        mcic |= MCIC_VB_GS;
> >> +    }
> >> +    return mcic;
> >> +}
> >> +  
> > 
> > Not sure why this should go into cpu.h - isn't there a better header
> > for it?  
> 
> internal.h maybe, but we also have the MCIC_* defines in cpu.h (esp.
> direct above this function :) ). So I guess this is best kept here.
> Opinions?

Let's keep it there for now. It's not like it has to stay there for all
eternity.

> 
> >   
> >>  
> >>  /* cpu.c */
> >>  int s390_get_clock(uint8_t *tod_high, uint64_t *tod_low);
> >> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> >> index 97c45d5537..9b8b59f2a2 100644
> >> --- a/target/s390x/kvm.c
> >> +++ b/target/s390x/kvm.c
> >> @@ -1852,33 +1852,12 @@ void kvm_s390_io_interrupt(uint16_t subchannel_id,
> >>      kvm_s390_floating_interrupt(&irq);
> >>  }
> >>  
> >> -static uint64_t build_channel_report_mcic(void)
> >> -{
> >> -    uint64_t mcic;
> >> -
> >> -    /* subclass: indicate channel report pending */
> >> -    mcic = MCIC_SC_CP |
> >> -    /* subclass modifiers: none */
> >> -    /* storage errors: none */  
> > 
> > I find these additional comments quite useful - can they be preserved
> > in any way?  
> 
> With the new abstraction ("validity" only) / function name most of them
> don't make sense anymore (esp. to be handled by the caller) in the
> context of this function. All other bits would be special for other
> machine check types we want to inject.

OK, as it seems we have #defines for all bits, I think we can live with
that.

> 
> >   
> >> -    /* validity bits: no damage */
> >> -        MCIC_VB_WP | MCIC_VB_MS | MCIC_VB_PM | MCIC_VB_IA | MCIC_VB_FP |
> >> -        MCIC_VB_GR | MCIC_VB_CR | MCIC_VB_ST | MCIC_VB_AR | MCIC_VB_PR |
> >> -        MCIC_VB_FC | MCIC_VB_CT | MCIC_VB_CC;
> >> -    if (s390_has_feat(S390_FEAT_VECTOR)) {
> >> -        mcic |= MCIC_VB_VR;
> >> -    }
> >> -    if (s390_has_feat(S390_FEAT_GUARDED_STORAGE)) {
> >> -        mcic |= MCIC_VB_GS;
> >> -    }
> >> -    return mcic;
> >> -}  
> 
> 
>