Hi,
....
>>>>>> + if (irq->vcpu || !(irq->pending && irq->enabled) || !vcpu) {
>>>>>> + /*
>>>>>> + * If this IRQ is already on a VCPU's ap_list, then it
>>>>>> + * cannot be moved or modified and there is no more work for
>>>>>> + * us to do.
>>>>>> + *
>>>>>> + * Otherwise, if the irq is not pending and enabled, it does
>>>>>> + * not need to be inserted into an ap_list and there is also
>>>>>> + * no more work for us to do.
>>>>>> + */
>>>>>
>>>>> is the !vcpu check here not redundant because if you ever get to
>>>>> evaluating it, then irq->vcpu is null, and pending and enabled are set,
>>>>> which means the oracle couldn't have returned null, could it?
>>>>
>>>> In this case vcpu is always irq->target_vcpu, if I did the math
>>>> correctly. So can this be NULL?
>>>> Even if this is correct reasoning, I wonder if we optimize something
>>>> prematurely here and rely on the current implementation of
>>>> vgic_target_oracle(). I think the check for "!vcpu" is here to avoid a
>>>> NULL pointer deference below (in the first spin_lock after the retry:
>>>> label), so I'd rather keep this explicit check in here.
>>>
>>> I'm really not a fan of building the correctness of one of the most
>>> crucial parts of our code based on "let's add a few extra checks which
>>> may not be necessary, just in case" kind of logic.
>>>
>>> So let's be clear on why we have an if-statement here exactly:
>>>
>>> As the comment says, if we can't move the IRQ, because it's already
>>> assigned to somebody or if this IRQ is not pending or active, then it's
>>> shouldn't be queued.
>>>
>>> So the simple and all-encompassing check here is simply:
>>>
>>> if (irq->vcpu || !vcpu) {
>>> spin_unlock(&irq->irq_lock);
>>> return false;
>>> }
>>>
>>> The only requirement for this to be correct is that the MMIO handler for
>>> ISACTIVER to both set the active bit and the irq->vcpu pointer (and put
>>> it on the AP list), without calling this function...). That was my
>>> quesiton below.
>>>
>>> Because if that's not the case, you could end up here with irq->active
>>> set, but irq->vcpu == NULL and !(pending && enabled) and you'd error
>>> out, which means you would have to check explicitly for the active state
>>> here as well, but I think that just becomes too messy.
>>>
>>> So, just change this to what I propose and we can deal with the active
>>> state MMIO handler separately.
>>
>> I agree that setting the active state via MMIO is a mess in general and
>> stuffing this case into this function here gets hairy.
>> I am tempted to not support it in the first version, I guess it never
>> really worked reliably before ...
>
> I'm pretty sure it did, because we ran into migration breaking when this
> wasn't supported for the save/restore userspace interface.
Well, I was more concerned about the reliability part in there and all
the corner cases. Not sure if anyone actually tested this from within a
guest.
>>
>> At the moment I am trying to code this explicitly into the SACTIVER
>> handler and it's messy, too (because of the corner cases).
>> Let's see how this will look like ...
>
> ok.
>
> If you want, you can focus on getting a new version out, and I can take
> a stab at the SACTIVER together with the priority stuff. OTOH, if you
> already have something, then it may be worth following through with
> that.
Yeah, so by now I have something which doesn't look too bad. Copied your
style with many comments ;-)
I will now clean up the patches and try to send something out still
today. I think by now there are significantly enough changes to justify
a new revision, even if I haven't addressed every single bit of the
comments yet.
Cheers,
Andre.
P.S. There be dragons:
char device redirected to /dev/pts/0 (label serial0)
[ 193.035693] Kernel panic - not syncing: HYP panic:
....
Probably due to the ->nr_lr rework, about to investigate now.
>>
>>>>
>>>>> that would also explain why we don't have to re-check the same
>>>>> conditions below...
>>>>>
>>>>> or am I getting this wrong, because you could also have someone
>>>>> explicitly setting the IRQ to active via trapped MMIO, in which case we
>>>>> should be able to queue it without it being pending && enabled, which
>>>>> would indicate that it's the other way around, you should only evaluate
>>>>> !vcpu and kup the !(pending && enabled) part....?
>>>>
>>>> You lost me here, which hints at the fragility of this optimization ;-)
>>>>
>>>>>> + spin_unlock(&irq->irq_lock);
>>>>>> + return false;
>>>>>> + }
>>>>>> +
>>>>>> + /*
>>>>>> + * We must unlock the irq lock to take the ap_list_lock where
>>>>>> + * we are going to insert this new pending interrupt.
>>>>>> + */
>>>>>> + spin_unlock(&irq->irq_lock);
>>>>>> +
>>>>>> + /* someone can do stuff here, which we re-check below */
>>>>>> +retry:
>>>>>> + spin_lock(&vcpu->arch.vgic_cpu.ap_list_lock);
>>>>>> + spin_lock(&irq->irq_lock);
>>>>>> +
>>>>>> + /*
>>>>>> + * Did something change behind our backs?
>>>>>> + *
>>>>>> + * There are two cases:
>>>>>> + * 1) The irq became pending or active behind our backs and/or
>>>>>> + * the irq->vcpu field was set correspondingly when putting
>>>>>> + * the irq on an ap_list. Then drop the locks and return.
>>>>>> + * 2) Someone changed the affinity on this irq behind our
>>>>>> + * backs and we are now holding the wrong ap_list_lock.
>>>>>> + * Then drop the locks and try the new VCPU.
>>>>>> + */
>>>>>> + if (irq->vcpu || !(irq->pending && irq->enabled)) {
>>>>>
>>>>> here I'm concerned about the active state again.
>>>>
>>>> Mmmh, can you elaborate and sketch a case where the active state would
>>>> cause trouble? This check is just here to avoid iterating on a no longer
>>>> pending or enabled IRQ. I wonder if an active IRQ can really sneak into
>>>> this function here in the first place?
>>>
>>> After having gone through the series I think we should deal with
>>> the active state queing directly in the vgic_mmio_write_sactive()
>>> function.
>>>
>>> But I still prefer to move the retry label to the very top of this
>>> function, and simplify these two statemtns to the condition I suggested:
>>>
>>> if (unlinkely(irq->vcpu || vcpu != vgic_target_oracle(irq)))
>>> goto retry;
>>>
>>> The cost is that we perform a few additional checks at runtime in the
>>> case where the IRQ was migrated while we released a lock (rare), but I
>>> think it simplifies the code.
>>
>> OK, I made this change. Also the shorter check after asking the oracle
>> above.
>> This should also better work in the case where target_vcpu is NULL
>> (because either no bit in TARGETSR is set or a non-existent MPIDR has
>> been written into IROUTER).
>>
> right.
>
> Thanks,
> -Christoffer
>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
