Hi Peter,
Many thanks for your detailed explanation. Upon further reflection, it
seems that I have misinterpreted some of the explanations in
the manual. Sorry for the confusion, the original implementation is
correct.
Thanks,
Tianrui
On Fri, Jul 16, 2021 at 4:32 PM Peter Maydell <peter.mayd...@linaro.org>
wrote:
> On Wed, 14 Jul 2021 at 20:46, Tianrui Wei <tianrui-...@outlook.com> wrote:
> >
> > For redistributor to send sgi, we must test NSACR bits in secure mode.
> > However, current implementation inverts the security check, wrongly
> > skipping this it when the CPU is in secure state, and only carrying out
> > the check when the CPU is not secure or security is not implemented.
> > This patch corrects this problem by correcting the inversion of CPU
> > secure state checking. It has been tested to work with Linux version 5.11
> > in both aarch64 and arm version of qemu.
> >
> > According to “Arm Generic Interrupt Controller Architecture
> > Specification GIC architecture version 3 and version 4,” p. 930, 2008.
> > Chapter 12, page 530, when there is only one security state implemented,
> > GICD.CTLR.DS is always 0, thus checking NSACR in non-secure state. When
> > cpu is in secure state, ns = 0, thus the NSACR check is never performed.
> >
> > Signed-off-by: Tianrui Wei <tianrui-...@outlook.com>
> > Signed-off-by: Jonathan Balkind <jbalk...@ucsb.edu>
> > Tested-by: Tianrui Wei <tianrui-...@outlook.com>
> > ---
> > hw/intc/arm_gicv3_redist.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/intc/arm_gicv3_redist.c b/hw/intc/arm_gicv3_redist.c
> > index 53da703ed8..84cfcfd18f 100644
> > --- a/hw/intc/arm_gicv3_redist.c
> > +++ b/hw/intc/arm_gicv3_redist.c
> > @@ -564,7 +564,7 @@ void gicv3_redist_send_sgi(GICv3CPUState *cs, int
> grp, int irq, bool ns)
> > return;
> > }
> >
> > - if (ns && !(cs->gic->gicd_ctlr & GICD_CTLR_DS)) {
> > + if (!ns && !(cs->gic->gicd_ctlr & GICD_CTLR_DS)) {
> > /* If security is enabled we must test the NSACR bits */
> > int nsaccess = gicr_ns_access(cs, irq);
>
> So, before this change:
> * if the access to ICC_SGI[01]R_EL1 attempting to kick off this SGI
> is done in Secure state, we allow it
> * if the GIC has security disabled (GICD_CTLR.DS is 1), we allow it
> * if the access is from NonSecure and the GIC does not have security
> disabled, we check the NSACR bits to see if we should allow it
>
> With this change, we check the NSACR bits for accesses from Secure
> state, and we don't check them for accesses from NonSecure.
> This doesn't seem to me to match what the spec requires: in version
> IHI0069G of the GICv3 spec, section 12.1.10, the tables show that
> only accesses from NonSecure are subject to NSACR checks. (This makes
> intuitive sense: the GICR_NSACR is the NonSecure Access Control
> Register and it is controls NonSecure accesses, not Secure accesses.)
>
> What bug are you trying to fix with this patch ?
>
> thanks
> -- PMM
>
