Re: [Xen-devel] [PATCH v2 1/2] xen/arm: ignore guest writes to GICD_ITARGETSR for SPIs

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-06-03 at 17:07 +0100, Stefano Stabellini wrote:
> Ignore guest writes to GICD_ITARGETSR that set the target cpu to a cpu
> other than cpu0 for SPIs.
> 
> Also ignore guest writes to GICD_ITARGETSR for PPIs and SGIs as they can
> only be delivered to the same cpu and that has already been configured
> at initialization time.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> ---
> 
> Changes in v2:
> - ignore writes to rank 0;
> - don't print a warning for ignoring writes to GICD_ITARGETSR;
> - add a comment in the code to remember that we don't implement writes
> to GICD_ITARGETSR.
> ---
>  xen/arch/arm/vgic.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index cb8df3a..1304b5e 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -584,6 +584,13 @@ static int vgic_distr_mmio_write(struct vcpu *v, 
> mmio_info_t *info)
>          if ( dabt.size != 0 && dabt.size != 2 ) goto bad_width;
>          rank = vgic_irq_rank(v, 8, gicd_reg - GICD_ITARGETSR);
>          if ( rank == NULL) goto write_ignore;
> +        /* only same vcpu delivery can be allowed for PPIs and SGIs */
> +        if ( REG_RANK_NR(8, gicd_reg - GICD_ITARGETSR) == 0 ) 
> +            return 1;

This is handled by a different case of the switch statement.

> +        /* SPI delivery to secondary vcpus is unimplemented */
> +        if ( REG_RANK_NR(8, gicd_reg - GICD_ITARGETSR) > 0 &&
> +             *r != (1|1<<8|1<<16|1<<24) )
> +            return 1;

Given that itargets is (about to be, in the next patch) initialised to
the same value isn't this entire hunk just a complicated way of saying
"goto write_ignore"?

(and if not please use goto write_ignore with the condition, since it is
self documenting)

And the REG_RANK_NR check here is unnecessary, because you checked for
== 0 already (and as I say it's handled further up anyway).

But -- doesn't your second patch actually make writing to ITARGETSR
work?

>          vgic_lock_rank(v, rank);
>          if ( dabt.size == 2 )
>              rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = 
> *r;



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