| Message ID | 1401041192-20424-1-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
Hi Stefano, On 25/05/14 19:06, Stefano Stabellini wrote: > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > xen/arch/arm/vgic.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 4869b87..e4f38a0 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -581,6 +581,11 @@ 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; > + if ( *r ) This is wrong, ITARGETSR store a 4 bitmask of CPUs, one per interrupt. Each bit of the mask correspond to a CPU (see Table 4-17 in the GICv2 manual). Furthermore, I think it's safe to just ignore write. The manual says: "It is IMPLEMENTATION DEFINED which, if any, SPIs are statically configured in hardware. The CPU targets field for such an SPI is read-only, and returns a value that indicates the CPU targets for the interrupt." With the former comment, your patch #2 is also wrong. > + { > + gdprintk(XENLOG_DEBUG, "SPI delivery to seconday cpus is unimplemented\n"); s/seconday/secondary/ Regards,
On Sun, 25 May 2014, Julien Grall wrote: > Hi Stefano, > > On 25/05/14 19:06, Stefano Stabellini wrote: > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > xen/arch/arm/vgic.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > > index 4869b87..e4f38a0 100644 > > --- a/xen/arch/arm/vgic.c > > +++ b/xen/arch/arm/vgic.c > > @@ -581,6 +581,11 @@ 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; > > + if ( *r ) > > This is wrong, ITARGETSR store a 4 bitmask of CPUs, one per interrupt. Each > bit of the mask correspond to a CPU (see Table 4-17 in the GICv2 manual). you are right > Furthermore, I think it's safe to just ignore write. The manual says: > "It is IMPLEMENTATION DEFINED which, if any, SPIs are statically configured in > hardware. The CPU targets field for such an SPI is read-only, and returns a > value that indicates the CPU targets for the interrupt." Good point > With the former comment, your patch #2 is also wrong. Patch #2 is valid regardless of the specific itarget setting > > + { > > + gdprintk(XENLOG_DEBUG, "SPI delivery to seconday cpus is > > unimplemented\n"); > > s/seconday/secondary/ > > Regards, > > -- > Julien Grall >
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 4869b87..e4f38a0 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -581,6 +581,11 @@ 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; + if ( *r ) + { + gdprintk(XENLOG_DEBUG, "SPI delivery to seconday cpus is unimplemented\n"); + return 1; + } vgic_lock_rank(v, rank); if ( dabt.size == 2 ) rank->itargets[REG_RANK_INDEX(8, gicd_reg - GICD_ITARGETSR)] = *r;
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- xen/arch/arm/vgic.c | 5 +++++ 1 file changed, 5 insertions(+)