|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 1/4] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs
On Fri, 2014-06-06 at 18:48 +0100, Stefano Stabellini wrote:
> vgic_enable_irqs should enable irq delivery to the vcpu specified by
> GICD_ITARGETSR, rather than the vcpu that wrote to GICD_ISENABLER.
> Similarly vgic_disable_irqs should use the target vcpu specified by
> itarget to disable irqs.
>
> itargets can be set to a mask but vgic_get_target_vcpu always returns
> the lower vcpu in the mask.
>
> Correctly initialize itargets for SPIs.
>
> Validate writes to GICD_ITARGETSR.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
>
> ---
>
> Changes in v4:
> - remove assert that could allow a guest to crash Xen;
> - add itargets validation to vgic_distr_mmio_write;
> - export vgic_get_target_vcpu.
>
> Changes in v3:
> - add assert in get_target_vcpu;
> - rename get_target_vcpu to vgic_get_target_vcpu.
>
> Changes in v2:
> - refactor the common code in get_target_vcpu;
> - unify PPI and SPI paths;
> - correctly initialize itargets for SPI;
> - use byte_read.
> ---
> xen/arch/arm/vgic.c | 60
> +++++++++++++++++++++++++++++++++++++++------
> xen/include/asm-arm/gic.h | 2 ++
> 2 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index cb8df3a..e527892 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -106,7 +106,15 @@ int domain_vgic_init(struct domain *d)
> INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue);
> }
> for (i=0; i<DOMAIN_NR_RANKS(d); i++)
> + {
> + int j;
> +
> spin_lock_init(&d->arch.vgic.shared_irqs[i].lock);
> + /* Only delivery to CPU0 */
s/delivery/deliver/.
And I think you should prefix it with "By default..."
> + for ( j = 0 ; j < 8 ; j++ )
> + d->arch.vgic.shared_irqs[i].itargets[j] =
> + (1<<0) | (1<<8) | (1<<16) | (1<<24);
Since these are bytes I think you could do:
memset(d->arch.vgic.shared_irqs[i].itargets, 0x1, sizeof(...))
> + }
> return 0;
> }
>
> @@ -369,6 +377,22 @@ read_as_zero:
> return 1;
> }
>
> +struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> +{
> + int target;
> + struct vgic_irq_rank *rank;
> + struct vcpu *v_target;
> +
> + rank = vgic_irq_rank(v, 1, irq/32);
Please can you do what vgic_vcpu_inject_irq does to look up the rank. Or
even better add a helper function which goes from an actual irq number
to the rank instead from a register offset to a bank (which is what
vgic_irq_rank actually is, i.e. vigc_irq_rank_from_gicd_offset or
something)
> + vgic_lock_rank(v, rank);
> + target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> + /* just return the first vcpu in the mask */
Is this a valid model for delivering an interrupt? I couldn't find
anything explicit in the GIC spec saying that it is valid for the
implementation to arbitrarily deliver to a single CPU.
The stuff in there about the 1-N case only deals with what happens when
one processor of the many does the IAR and what the others then see
(spurious IRQ).
To be clear: I don't object to this implementation but I think it should
either be backed up by reference to the spec or it should be explicitly
mentioned in a comment how/where/why we deviate from it.
> + target = find_next_bit((const unsigned long *) &target, 8, 0);
> + v_target = v->domain->vcpu[target];
> + vgic_unlock_rank(v, rank);
> + return v_target;
> +}
> +
> static void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
> {
> const unsigned long mask = r;
> @@ -376,12 +400,14 @@ static void vgic_disable_irqs(struct vcpu *v, uint32_t
> r, int n)
> unsigned int irq;
> unsigned long flags;
> int i = 0;
> + struct vcpu *v_target;
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> irq = i + (32 * n);
> - p = irq_to_pending(v, irq);
> + v_target = vgic_get_target_vcpu(v, irq);
> + p = irq_to_pending(v_target, irq);
> clear_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
> - gic_remove_from_queues(v, irq);
> + gic_remove_from_queues(v_target, irq);
What locks are required to poke at another vcpu's state in this way? You
don't seem to take any locks relating to v_target, and I don't think any
of the callers take any global lock e.g. you have already dropped the
rank's lock in the caller (although I confess I'm looking at the current
tree not one with all your patches applied).
WRT dropping the rank lock -- shouldn't this be inside that anyway to
handle races between different vcpus enabling/disabling a single IRQ?
Which also needs care when v==v_target if you already hold any locks
relating to v.
> if ( p->desc != NULL )
> {
> spin_lock_irqsave(&p->desc->lock, flags);
> @@ -399,24 +425,26 @@ static void vgic_enable_irqs(struct vcpu *v, uint32_t
> r, int n)
> unsigned int irq;
> unsigned long flags;
> int i = 0;
> + struct vcpu *v_target;
>
> while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
> irq = i + (32 * n);
> - p = irq_to_pending(v, irq);
> + v_target = vgic_get_target_vcpu(v, irq);
Locking for some of this too (although I see in one case you do take the
v_target's gic lock).
> @@ -502,6 +530,7 @@ static int vgic_distr_mmio_write(struct vcpu *v,
> mmio_info_t *info)
> int offset = (int)(info->gpa - v->domain->arch.vgic.dbase);
> int gicd_reg = REG(offset);
> uint32_t tr;
> + int i;
>
> switch ( gicd_reg )
> {
> @@ -585,6 +614,21 @@ static int vgic_distr_mmio_write(struct vcpu *v,
> mmio_info_t *info)
> rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
> if ( rank == NULL) goto write_ignore;
> vgic_lock_rank(v, rank);
> + tr = *r & ~(rank->itargets[REG_RANK_INDEX(8, gicd_reg -
> GICD_ITARGETSR)]);
> + i = 0;
> + /* validate writes */
> + while ( (i = find_next_bit((const unsigned long *) &tr, 32, i)) < 32
> )
> + {
> + unsigned int target = i % 8;
> + if ( target > v->domain->max_vcpus )
> + {
> + gdprintk(XENLOG_WARNING, "vGICD: GICD_ITARGETSR write
> invalid target vcpu %u\n",
> + target);
The spec says:
A CPU targets field bit that corresponds to an unimplemented CPU
interface is RAZ/WI.
So I think you can just implement the write with the existing code and
then mask the result in rank->itargets[] to clear any invalid CPUs,
which will be a lot simpler than this I think.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |