On Mon, 4 Dec 2017 18:16:15 +0100
David Hildenbrand <da...@redhat.com> wrote:
> On 04.12.2017 18:03, Cornelia Huck wrote:
> > On Mon, 4 Dec 2017 13:55:01 +0100
> > David Hildenbrand <da...@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 <da...@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;
> >> -}
>
>
>
