From: Julien Grall <julien.grall@arm.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org
Subject: Re: [PATCH] xen/arm: fix rank/vgic locks inversion bug
Date: Tue, 20 Dec 2016 12:28:27 +0100 [thread overview]
Message-ID: <1a04b05f-a79d-a1f9-32dc-84e57cba990a@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.10.1612191504290.13831@sstabellini-ThinkPad-X260>
Hi Stefano,
On 20/12/2016 00:22, Stefano Stabellini wrote:
> On Mon, 19 Dec 2016, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 19/12/2016 23:30, Stefano Stabellini wrote:
>>> On Mon, 19 Dec 2016, Julien Grall wrote:
>>>>>> 2) We run gic_update_one_lr and vgic_store_itargetsr in parallel
>>>>>> safely
>>>>>> and locklessly. There might be a way to do it, but it is not easy I
>>>>>> haven't found it yet.
>>>>
>>>> Correct me if I am wrong. There are no restriction to write into
>>>> IROUTER/ITARGETSR while an IRQ is pending. So the irq_set_affinity could
>>>> be
>>>> called once at the beginning of vgic_irq_migrate.
>>>>
>>>> We may receive the interrupt on the wrong physical CPU at the beginning.
>>>> But
>>>> it would be the same when writing into IROUTER/ITARGETSR.
>>>>
>>>> This would remove the need to get the rank lock in gic_update_one_lr.
>>>>
>>>> Did I miss anything?
>>>
>>> I am not sure if we can do that: the virtual interrupt might not be
>>> EOI'd yet at that time. The guest EOI is used to deactivate the
>>> corresponding physical interrupt. Migrating the physical IRQ at that
>>> time, could have unintended consequences? I am not sure what the spec
>>> says about this, but even if it is correct on paper, I would prefer not
>>> to put it to the test: I bet that not many interrupt controllers have
>>> been heavily tested in this scenario. What do you think?
>>
>> I don't think this is an issue. An interrupt could have the priority drop
>> happening on pCPU A and be deactivated on pCPU B if the vCPU A is been
>> migrated when the interrupt is inflight. So if it is fine here, why would not
>> it be when the guest is specifically requesting the routing?
>
> That is true, but this is not exactly the same.
My example was to show you that an IRQ can have its priority dropped in
pCPU A and been deactivated to pCPU B. Another example is when only the
IRQ is been migrated. The spec does not promise you to receive the next
interrupt on the CPU you asked because it may take time to update the
GIC state. So the priority drop and deactivation could be done on
separate physical CPU here too.
> This is changing the
> physical irq affinity while both physical and virtual irqs are still
> active.
Physical IRQ state and virtual IRQ state are completely dissociated in
the GIC. The only interaction possible is the virtual interface to send
a deactivate request to the distributor when the virtual interrupt has
been deactivated and correspond to a hardware interrupt.
> As I wrote, usually operating systems only change affinity after
> deactivating an irq, so I thought it would be wise in Xen to wait at
> least for the EOI.
I looked at the Linux code and did not see a such requirement when
setting the affinity (see irq_set_affinity) of an IRQ.
> If we tried to inject the same virtual interrupt on a
> different vcpu, while the interrupt is still inflight, we could get in
> troubles. But we could avoid that by testing for GIC_IRQ_GUEST_MIGRATING
> in vgic_vcpu_inject_irq. Maybe I am worrying about nothing.
The only interrupt that can be routed to a guest in Xen are SPI which
are shared between all CPUs. The active bit is handled by the
distributor. It is not possible to receive the same SPI until it has
been deactivated.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
next prev parent reply other threads:[~2016-12-20 11:28 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-15 1:04 [PATCH] xen/arm: fix rank/vgic locks inversion bug Stefano Stabellini
2016-12-15 22:55 ` Julien Grall
2016-12-16 0:08 ` Stefano Stabellini
2016-12-16 11:51 ` Julien Grall
2016-12-17 1:08 ` Stefano Stabellini
2016-12-17 1:13 ` Stefano Stabellini
2016-12-19 17:46 ` Julien Grall
2016-12-19 22:30 ` Stefano Stabellini
2016-12-19 22:56 ` Julien Grall
2016-12-19 23:22 ` Stefano Stabellini
2016-12-20 11:28 ` Julien Grall [this message]
2016-12-20 19:38 ` Stefano Stabellini
2016-12-22 2:12 ` Stefano Stabellini
2016-12-28 15:48 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1a04b05f-a79d-a1f9-32dc-84e57cba990a@arm.com \
--to=julien.grall@arm.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.