On 07.02.2018 14:47, Collin L. Walling wrote:
> On 02/07/2018 06:46 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@xxxxxxxxxx>
>> ---
>> arch/s390/kvm/interrupt.c | 25 +++++++++++++++++++------
>> 1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
>> index 3ea9cfa31b16..a616e9b65f91 100644
>> --- a/arch/s390/kvm/interrupt.c
>> +++ b/arch/s390/kvm/interrupt.c
>> @@ -169,8 +169,15 @@ 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))
>> + const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>> + const u64 ckc = vcpu->arch.sie_block->ckc;
>> +
>> + if (vcpu->arch.sie_block->gcr[0] & 0x0020000000000000ul) {
>> + if ((s64)ckc >= (s64)now)
>> + return 0;
>> + } else if (ckc >= now) {
>> return 0;
>> + }
>> return ckc_interrupts_enabled(vcpu);
>> }
>>
>> @@ -1042,13 +1049,19 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
>>
>> static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
>> {
>> - u64 now, cputm, sltime = 0;
>> + const u64 now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>> + const u64 ckc = vcpu->arch.sie_block->ckc;
>> + u64 cputm, sltime = 0;
>>
>> if (ckc_interrupts_enabled(vcpu)) {
>> - now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>> - sltime = tod_to_ns(vcpu->arch.sie_block->ckc - now);
>> - /* already expired or overflow? */
>> - if (!sltime || vcpu->arch.sie_block->ckc <= now)
>> + if (vcpu->arch.sie_block->gcr[0] & 0x0020000000000000ul) {
>> + if ((s64)now < (s64)ckc)
>> + sltime = tod_to_ns((s64)ckc - (s64)now);
>> + } else if (now < ckc) {
>> + sltime = tod_to_ns(ckc - now);
>> + }
>> + /* already expired */
>> + if (!sltime)
>> return 0;
>> if (cpu_timer_interrupts_enabled(vcpu)) {
>> cputm = kvm_s390_get_cpu_timer(vcpu);
> I think it would assist with readability if you defined the sign
> comparison bit. Seeing
> something that yells "SIGNED" would make sense as to what's going on here.
If we want that than I suggest introducing defines for all control
registers we use in kvm code in a separate patch.
>
> Other than that, I don't see anything wrong.
>
Thanks!
> I'll get to reviewing the rest of these patches throughout the day. I
> have to revisit
> the docs :)
>
--
Thanks,
David / dhildenb
