| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <7f33ff97-9503-9b32-b244-1e8104d0408e@linux.ibm.com>
Date: Wed, 30 Jan 2019 17:24:49 +0100
From: Pierre Morel <pmorel@...ux.ibm.com>
To: mimu@...ux.ibm.com, Halil Pasic <pasic@...ux.ibm.com>
Cc: KVM Mailing List <kvm@...r.kernel.org>,
Linux-S390 Mailing List <linux-s390@...r.kernel.org>,
linux-kernel@...r.kernel.org,
Martin Schwidefsky <schwidefsky@...ibm.com>,
Heiko Carstens <heiko.carstens@...ibm.com>,
Christian Borntraeger <borntraeger@...ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>,
David Hildenbrand <david@...hat.com>,
Cornelia Huck <cohuck@...hat.com>
Subject: Re: [PATCH v6 12/13] KVM: s390: add gib_alert_irq_handler()
On 29/01/2019 16:29, Michael Mueller wrote:
>
>
> On 29.01.19 14:26, Halil Pasic wrote:
>> On Thu, 24 Jan 2019 13:59:38 +0100
>> Michael Mueller <mimu@...ux.ibm.com> wrote:
>>
>>> The patch implements a handler for GIB alert interruptions
>>> on the host. Its task is to alert guests that interrupts are
>>> pending for them.
>>>
>>> A GIB alert interrupt statistic counter is added as well:
>>>
>>> $ cat /proc/interrupts
>>> CPU0 CPU1
>>> ...
>>> GAL: 23 37 [I/O] GIB Alert
>>> ...
>>>
>>> Signed-off-by: Michael Mueller <mimu@...ux.ibm.com>
>> [..]
>>> +/**
>>> + * gisa_get_ipm_or_restore_iam - return IPM or restore GISA IAM
>>> + *
>>> + * @gi: gisa interrupt struct to work on
>>> + *
>>> + * Atomically restores the interruption alert mask if none of the
>>> + * relevant ISCs are pending and return the IPM.
>>
>> The word 'relevant' probably reflects some previous state. It does not
>> bother me too much.
>
> "relevant" refers to the ISCs handled by the GAL mechanism, i.e those
> registered in the kvm->arch.gisa_int.alert.mask.
>
>>
>> [..]
>>
>>> +static void __airqs_kick_single_vcpu(struct kvm *kvm, u8
>>> deliverable_mask)
>>> +{
>>> + int vcpu_id, online_vcpus = atomic_read(&kvm->online_vcpus);
>>> + struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
>>> + struct kvm_vcpu *vcpu;
>>> +
>>> + for_each_set_bit(vcpu_id, kvm->arch.idle_mask, online_vcpus) {
>>> + vcpu = kvm_get_vcpu(kvm, vcpu_id);
>>> + if (psw_ioint_disabled(vcpu))
>>> + continue;
>>> + deliverable_mask &= (u8)(vcpu->arch.sie_block->gcr[6] >> 24);
>>> + if (deliverable_mask) {
>>> + /* lately kicked but not yet running */
>>
>> How about /* was kicked but didn't run yet */?
>
> what is the difference here...
>
>>
>>> + if (test_and_set_bit(vcpu_id, gi->kicked_mask))
>>> + return;
>>> + kvm_s390_vcpu_wakeup(vcpu);
>>> + return;
>>> + }
>>> + }
>>> +}
>>> +
>>
>> [..]
>>
>>> +static void process_gib_alert_list(void)
>>> +{
>>> + struct kvm_s390_gisa_interrupt *gi;
>>> + struct kvm_s390_gisa *gisa;
>>> + struct kvm *kvm;
>>> + u32 final, origin = 0UL;
>>> +
>>> + do {
>>> + /*
>>> + * If the NONE_GISA_ADDR is still stored in the alert list
>>> + * origin, we will leave the outer loop. No further GISA has
>>> + * been added to the alert list by millicode while processing
>>> + * the current alert list.
>>> + */
>>> + final = (origin & NONE_GISA_ADDR);
>>> + /*
>>> + * Cut off the alert list and store the NONE_GISA_ADDR in the
>>> + * alert list origin to avoid further GAL interruptions.
>>> + * A new alert list can be build up by millicode in parallel
>>> + * for guests not in the yet cut-off alert list. When in the
>>> + * final loop, store the NULL_GISA_ADDR instead. This will re-
>>> + * enable GAL interruptions on the host again.
>>> + */
>>> + origin = xchg(&gib->alert_list_origin,
>>> + (!final) ? NONE_GISA_ADDR : NULL_GISA_ADDR);
>>> + /*
>>> + * Loop through the just cut-off alert list and start the
>>> + * gisa timers to kick idle vcpus to consume the pending
>>> + * interruptions asap.
>>> + */
>>> + while (origin & GISA_ADDR_MASK) {
>>> + gisa = (struct kvm_s390_gisa *)(u64)origin;
>>> + origin = gisa->next_alert;
>>> + gisa->next_alert = (u32)(u64)gisa;
>>> + kvm = container_of(gisa, struct sie_page2, gisa)->kvm;
>>> + gi = &kvm->arch.gisa_int;
>>> + if (hrtimer_active(&gi->timer))
>>> + hrtimer_cancel(&gi->timer);
>>> + hrtimer_start(&gi->timer, 0, HRTIMER_MODE_REL);
>>> + }
>>> + } while (!final);
>>> +
>>> +}
>>> +
>>> void kvm_s390_gisa_clear(struct kvm *kvm)
>>> {
>>> struct kvm_s390_gisa_interrupt *gi = &kvm->arch.gisa_int;
>>> if (!gi->origin)
>>> return;
>>> - memset(gi->origin, 0, sizeof(struct kvm_s390_gisa));
>>> - gi->origin->next_alert = (u32)(u64)gi->origin;
>>> + gisa_clear_ipm(gi->origin);
>>
>> This could be a separate patch. I would like little more explanation
>> to this.
>
> I can break at out as I had before... ;)
>
>>
>>> VM_EVENT(kvm, 3, "gisa 0x%pK cleared", gi->origin);
>>> }
>>> @@ -2940,13 +3078,25 @@ void kvm_s390_gisa_init(struct kvm *kvm)
>>> gi->origin = &kvm->arch.sie_page2->gisa;
>>> gi->alert.mask = 0;
>>> spin_lock_init(&gi->alert.ref_lock);
>>> - kvm_s390_gisa_clear(kvm);
>>> + gi->expires = 50 * 1000; /* 50 usec */
>>
>> I blindly trust your choice here ;)
>
> You know I will increase it to 1 ms together with the change that I
> proposed. (gisa_get_ipm_or_restore_iam() in kvm_s390_handle_wait()).
With this.
Reviewed-by: Pierre Morel<pmorel@...ux.ibm.com>
--
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany
Powered by blists - more mailing lists