Re: [Xen-devel] [PATCH] xen/arm: fix rank/vgic locks inversion bug

[Julien Grall <julien.grall@xxxxxxx>]

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?
> Thinking outside the box, another way to solve this problem is to reject
> any interrupt affinity changes while an interrupt migration is still in
> progress. In fact, that scenario is very uncommon. As far as I know
> operating systems deactivate interrupts before migrating them.
I would rather avoid to differ from the specification, even it looks 
sensible for a guest to migrate the interrupt whilst it is not active.
> Otherwise, I think it is very reasonable to store the vcpu id (or the
> pcpu id) in struct pending_irq: we already store the lr register number
> there. The current code can tell in which lr register an interrupt has
> been written to, but it cannot tell to which cpu the lr register belongs
> to. It's a paradox. Once we know the vcpu id for any inflight irqs, then
> we can make sure to take the right vcpu.vgic lock from vgic_migrate_irq.
This is a good point. I was concerned about the size of pending_irq (we 
have to allocate it per-IRQ) but it looks like we have some padding in 
the structure between priority and inflight.
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel