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

Message ID 20171204125505.29203-2-david@redhat.com
State New
Headers show
Series
  • s390x/tcg: CCW hotplug support
Related show

Commit Message

David Hildenbrand Dec. 4, 2017, 12:55 p.m.
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
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(-)

Comments

Cornelia Huck Dec. 4, 2017, 5:03 p.m. | #1
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/

> 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/

> +    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?

>  
>  /* 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?

> -    /* 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;
> -}
> -
>  void kvm_s390_crw_mchk(void)
>  {
>      struct kvm_s390_irq irq = {
>          .type = KVM_S390_MCHK,
> -        .u.mchk.cr14 = 1 << 28,
> -        .u.mchk.mcic = build_channel_report_mcic(),
> +        .u.mchk.cr14 = CR14_CHANNEL_REPORT_SC,

I like this new define.

> +        .u.mchk.mcic = s390_build_validity_mcic() | MCIC_SC_CP,
>      };
>      kvm_s390_floating_interrupt(&irq);
>  }
David Hildenbrand Dec. 4, 2017, 5:16 p.m. | #2
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.

> 
>> 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

> 
>> +    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?

> 
>>  
>>  /* 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.

> 
>> -    /* 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;
>> -}
Cornelia Huck Dec. 5, 2017, 10:20 a.m. | #3
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;
> >> -}  
> 
> 
>
David Hildenbrand Dec. 5, 2017, 11:29 a.m. | #4
On 05.12.2017 11:20, Cornelia Huck wrote:
> 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?
> 

Yes, will use "validity" consistently here. Thanks!

Patch

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) */
+    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;
+}
+
 
 /* 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 */
-    /* 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;
-}
-
 void kvm_s390_crw_mchk(void)
 {
     struct kvm_s390_irq irq = {
         .type = KVM_S390_MCHK,
-        .u.mchk.cr14 = 1 << 28,
-        .u.mchk.mcic = build_channel_report_mcic(),
+        .u.mchk.cr14 = CR14_CHANNEL_REPORT_SC,
+        .u.mchk.mcic = s390_build_validity_mcic() | MCIC_SC_CP,
     };
     kvm_s390_floating_interrupt(&irq);
 }