Re: [PATCH v2] KVM: s390: take care of clock-comparator sign control

[Christian Borntraeger <borntraeger@de.ibm.com>]

Really adding Collin...


On 02/05/2018 01:46 PM, Christian Borntraeger wrote:
> Adding Collin. 
> 
> On 02/05/2018 11:40 AM, David Hildenbrand wrote:
>> Missed when enabling the Multiple-epoch facility. If the facility is
>> installed and the control is set, a sign based comaprison has to be
>> performed.
>>
>> Right now we would inject wrong interrupts and ignore interrupt
>> conditions. Also the sleep time is calculated in a wrong way.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>
>> We might be able to drop the checks for "test_kvm_facility(vcpu->kvm, 139)",
>> as the architecture states:
>>
>> "When the multiple-epoch facility is not installed in the configuration
>> and the clock-comparator sign control is one, it is unpredictable whether
>> the comparison follows the rules of unsigned or signed binary arithmetic."
>>
>> Have no machine to test this with :(
> 
> It will be somewhat hard to test anyway since the compare only differs for the
> case where one value (TOD or CKC) is before the 2042 date and the other one is
> after. have to think about a good test without needing an LPAR that is close
> to the wraparound.
> 
>>
>>  arch/s390/kvm/interrupt.c | 32 ++++++++++++++++++++++++++------
>>  1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 024ad8bcc516..6566a853c0b8 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -170,7 +170,16 @@ static int ckc_interrupts_enabled(struct kvm_vcpu *vcpu)
>>
>>  static int ckc_irq_pending(struct kvm_vcpu *vcpu)
>>  {
>> -	if (vcpu->arch.sie_block->ckc >= kvm_s390_get_tod_clock_fast(vcpu->kvm))
>> +	int64_t ckc, tod;
>> +
>> +	if (vcpu->arch.sie_block->gcr[0] & 0x0020000000000000ul &&
>> +	    test_kvm_facility(vcpu->kvm, 139)) {
>> +		ckc = vcpu->arch.sie_block->ckc;
>> +		tod = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>> +		if (ckc >= tod)
>> +			return 0;
>> +	} else if (vcpu->arch.sie_block->ckc >=
>> +		   kvm_s390_get_tod_clock_fast(vcpu->kvm))
>>  		return 0;
> 
> Instead of changing the compare depending on another compare, maybe adding
> 0x8000000000000000 to the unsigned values makes the change easier to grasp. 
> On the other hand your code is closer to POP.
>