* [PATCH v2] KVM: s390: take care of clock-comparator sign control
@ 2018-02-05 10:40 David Hildenbrand
2018-02-05 12:46 ` Christian Borntraeger
2018-02-06 16:34 ` Collin L. Walling
0 siblings, 2 replies; 6+ messages in thread
From: David Hildenbrand @ 2018-02-05 10:40 UTC (permalink / raw)
To: linux-s390, kvm
Cc: Christian Borntraeger, Cornelia Huck, Janosch Frank,
David Hildenbrand
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 :(
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;
return ckc_interrupts_enabled(vcpu);
}
@@ -1011,13 +1020,24 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
{
- u64 now, cputm, sltime = 0;
+ u64 now, cputm, ckc, sltime = 0;
+ int64_t ckc_signed, now_signed;
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 &&
+ test_kvm_facility(vcpu->kvm, 139)) {
+ now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
+ ckc = vcpu->arch.sie_block->ckc;
+ if (ckc < now)
+ sltime = tod_to_ns(now - ckc);
+ } else {
+ now_signed = kvm_s390_get_tod_clock_fast(vcpu->kvm);
+ ckc_signed = vcpu->arch.sie_block->ckc;
+ if (ckc_signed < now_signed)
+ sltime = tod_to_ns(now_signed - ckc_signed);
+ }
+ /* already expired */
+ if (!sltime)
return 0;
if (cpu_timer_interrupts_enabled(vcpu)) {
cputm = kvm_s390_get_cpu_timer(vcpu);
--
2.14.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: take care of clock-comparator sign control
2018-02-05 10:40 [PATCH v2] KVM: s390: take care of clock-comparator sign control David Hildenbrand
@ 2018-02-05 12:46 ` Christian Borntraeger
2018-02-05 13:22 ` Christian Borntraeger
2018-02-05 13:49 ` David Hildenbrand
2018-02-06 16:34 ` Collin L. Walling
1 sibling, 2 replies; 6+ messages in thread
From: Christian Borntraeger @ 2018-02-05 12:46 UTC (permalink / raw)
To: David Hildenbrand, linux-s390, kvm
Cc: Cornelia Huck, Janosch Frank, Collin L. Walling
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: take care of clock-comparator sign control
2018-02-05 12:46 ` Christian Borntraeger
@ 2018-02-05 13:22 ` Christian Borntraeger
2018-02-05 13:49 ` David Hildenbrand
1 sibling, 0 replies; 6+ messages in thread
From: Christian Borntraeger @ 2018-02-05 13:22 UTC (permalink / raw)
To: David Hildenbrand, linux-s390, kvm
Cc: Cornelia Huck, Janosch Frank, Collin L. Walling
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: take care of clock-comparator sign control
2018-02-05 12:46 ` Christian Borntraeger
2018-02-05 13:22 ` Christian Borntraeger
@ 2018-02-05 13:49 ` David Hildenbrand
1 sibling, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2018-02-05 13:49 UTC (permalink / raw)
To: Christian Borntraeger, linux-s390, kvm
Cc: Cornelia Huck, Janosch Frank, Collin L. Walling
On 05.02.2018 13:46, 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.
>
You can set the TOD in the guest to a certain value just before/after
overflowing. But you would also need a guest that actually makes use of
the overflow bit. Could be done in a unit test (e.g. kvm-unit-tests).
>>
>> 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.
Not sure if that is easier to understand, but would also work.
> On the other hand your code is closer to POP.
>
Thanks!
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: take care of clock-comparator sign control
2018-02-05 10:40 [PATCH v2] KVM: s390: take care of clock-comparator sign control David Hildenbrand
2018-02-05 12:46 ` Christian Borntraeger
@ 2018-02-06 16:34 ` Collin L. Walling
2018-02-06 17:02 ` David Hildenbrand
1 sibling, 1 reply; 6+ messages in thread
From: Collin L. Walling @ 2018-02-06 16:34 UTC (permalink / raw)
To: David Hildenbrand, linux-s390, kvm
Cc: Christian Borntraeger, Cornelia Huck, Janosch Frank
On 02/05/2018 05: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."
I would drop the MEF check. We only compare the ckc with the 64-bit
TOD-Clock
regardless if the facility is present or not.
>
> Have no machine to test this with :(
>
> 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;
> return ckc_interrupts_enabled(vcpu);
> }
> @@ -1011,13 +1020,24 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
>
> static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
> {
> - u64 now, cputm, sltime = 0;
> + u64 now, cputm, ckc, sltime = 0;
> + int64_t ckc_signed, now_signed;
>
> 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 &&
> + test_kvm_facility(vcpu->kvm, 139)) {
> + now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
> + ckc = vcpu->arch.sie_block->ckc;
Shouldn't you be using now_signed and ckc_signed here?
> + if (ckc < now)
> + sltime = tod_to_ns(now - ckc);
> + } else {
> + now_signed = kvm_s390_get_tod_clock_fast(vcpu->kvm);
> + ckc_signed = vcpu->arch.sie_block->ckc;
and the unsigned ones here?
Also you could just compare vcpu->arch.sie_block->ckc and
kvm_s390_get_tod_clock_fast(vcpu->kvm)
> + if (ckc_signed < now_signed)
> + sltime = tod_to_ns(now_signed - ckc_signed);
Shouldn't we only calculate sleep time if ckc is greater than now (in
both cases)?
> + }
> + /* already expired */
> + if (!sltime)
> return 0;
> if (cpu_timer_interrupts_enabled(vcpu)) {
> cputm = kvm_s390_get_cpu_timer(vcpu);
Other than that, this is a heck of a lot easier to read than what we had
before.
--
- Collin L Walling
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] KVM: s390: take care of clock-comparator sign control
2018-02-06 16:34 ` Collin L. Walling
@ 2018-02-06 17:02 ` David Hildenbrand
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2018-02-06 17:02 UTC (permalink / raw)
To: Collin L. Walling, linux-s390, kvm
Cc: Christian Borntraeger, Cornelia Huck, Janosch Frank
>> static u64 __calculate_sltime(struct kvm_vcpu *vcpu)
>> {
>> - u64 now, cputm, sltime = 0;
>> + u64 now, cputm, ckc, sltime = 0;
>> + int64_t ckc_signed, now_signed;
>>
>> 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 &&
>> + test_kvm_facility(vcpu->kvm, 139)) {
>> + now = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>> + ckc = vcpu->arch.sie_block->ckc;
>
>
> Shouldn't you be using now_signed and ckc_signed here?
Yes indeed.
>
>
>> + if (ckc < now)
>> + sltime = tod_to_ns(now - ckc);
>> + } else {
>> + now_signed = kvm_s390_get_tod_clock_fast(vcpu->kvm);
>> + ckc_signed = vcpu->arch.sie_block->ckc;
>
> and the unsigned ones here?
>
> Also you could just compare vcpu->arch.sie_block->ckc and
> kvm_s390_get_tod_clock_fast(vcpu->kvm)
That leads to ugly long lines (for the comparison and the calculation)
>
>> + if (ckc_signed < now_signed)
>> + sltime = tod_to_ns(now_signed - ckc_signed);
>
>
> Shouldn't we only calculate sleep time if ckc is greater than now (in
> both cases)?
>
I think you're right!
(this was a 5min hack originally titled RFC, but I screwed up my command
line because I was sending a QEMU patch in between)
Will resend, thanks for having a look! :)
>
>> + }
>> + /* already expired */
>> + if (!sltime)
>> return 0;
>> if (cpu_timer_interrupts_enabled(vcpu)) {
>> cputm = kvm_s390_get_cpu_timer(vcpu);
>
> Other than that, this is a heck of a lot easier to read than what we had
> before.
>
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-02-06 17:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-05 10:40 [PATCH v2] KVM: s390: take care of clock-comparator sign control David Hildenbrand
2018-02-05 12:46 ` Christian Borntraeger
2018-02-05 13:22 ` Christian Borntraeger
2018-02-05 13:49 ` David Hildenbrand
2018-02-06 16:34 ` Collin L. Walling
2018-02-06 17:02 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox