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

[David Hildenbrand]

On 04.12.2017 18:03, Cornelia Huck wrote:
> On Mon,  4 Dec 2017 13:55:01 +0100
> David Hildenbrand <address@hidden> 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 <address@hidden>
>> ---
>>  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;
>> -}



-- 

Thanks,

David / dhildenb