On Thu, 17 Dec 2015, Ian Campbell wrote:
> > > + /* Save PPI and SGI states (per-CPU) */
> > > + spin_lock(&v->arch.vgic.lock);
> >
> > vgic_lock
>
> Ah, yes, this function was originally in save.c so didn't have access, but
> now it is in the right place I should use all the correct functions.
>
> > Is the domain already paused at this point? If so, maybe we could get
> > away without locking?
>
> Maybe we could think about it hard enough to rationalise that, but it seems
> safer and easier to just follow the locking rules regardless.
>
> > > +int vgic_irq_save_core(struct vcpu *v, unsigned irq, struct hvm_hw_irq
> > > *s)
> > > +{
> > > + struct pending_irq *p = irq_to_pending(v, irq);
> > > + struct vgic_irq_rank *rank = vgic_rank_irq(v, p->irq);
> > > +
> > > + const bool enabled = test_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> > > + const bool queued = test_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> > > + const bool visible = test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > > +
> > > + spin_lock(&rank->lock);
> >
> > vgic_rank_lock. Maybe we need to take the vgic lock because there might
> > be incoming hardware interrupts for the domain but I don't think we need
> > the rank lock, do we?
>
> As above I think it is simpler and safer to just follow the usual rules
> rather than to special case things.
I don't think we take the rank lock with the vgic lock held anywhere in
the code though. It would be good to avoid nested locking unless
necessary.
> > + {
> > > + /*
> > > + * This uses the current IPRIORITYR, which may differ from
> > > + * when the IRQ was actually made pending. h/w spec probably
> > > + * allows this, XXXX check
> > > + */
> >
> > From this VM point of view the IPRIORITYR is s->priority. I would remove
> > the comment.
>
> If you have an IRQ<A> with priority 55 which fires and then while it is
> pending the priority is changed to 75 then I'm not sure what impact that
> has on the priority which the hardware considers the already pending
> interrupt to have.
>
> To make it more complex consider if there was a second interrupt IRQ<B>
> with priority 65, if the handler for that was the one changed IRQ<A>'s
> priority should it expect to get immediately preempted by IRQ<A>
>
> The comment is there because I'm not sure what behaviour the spec requires.
The spec says:
It is IMPLEMENTATION DEFINED whether changing the value of a priority
field changes the priority of an active interrupt.
We could infer that changing the priority of pending interrupts is
supposed to be supported.
I would only write:
* This uses the current IPRIORITYR, which may differ from
* when the IRQ was actually made pending.
> In terms of our code without the save restore I think the pending IRQ<A>
> would remain at priority 55 and the guest would get IRQ<A> when IRQ<B>
> EOIs. Whereas upon restore we would reraise it with priority 75 and IRQ<A>
> would appear to preempt IRQ<B>, which would be an interesting difference in
> behaviour from the PoV of the guest.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
