Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Fri, 04 Mar 2022 16:44:32 +0000
Message-ID: <87k0d91xcf.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Linu Cherian <lcherian@marvell.com>
Cc:     "tglx@linutronix.de" <tglx@linutronix.de>,
        "catalin.marinas@arm.com" <catalin.marinas@arm.com>,
        "will@kernel.org" <will@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linuc.decode@gmail.com" <linuc.decode@gmail.com>
Subject: Re: [EXT] Re: [PATCH V2] irqchip/gic-v3: Workaround Marvell erratum 38545 when reading IAR
In-Reply-To: <PH0PR18MB50021922303A495B997648FACE059@PH0PR18MB5002.namprd18.prod.outlook.com>
References: <20220304014301.2515-1-lcherian@marvell.com>
        <87ilsutb6w.wl-maz@kernel.org>
        <PH0PR18MB50021922303A495B997648FACE059@PH0PR18MB5002.namprd18.prod.outlook.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

Hi Linu,

On Fri, 04 Mar 2022 13:25:42 +0000,
Linu Cherian <lcherian@marvell.com> wrote:
> 
> Hi Marc,
> 
> > >  static inline u64 gic_read_iar_cavium_thunderx(void)
> > >  {
> > > -	u64 irqstat;
> > > +	u64 irqstat, apr;
> > >
> > > +	apr = read_sysreg_s(SYS_ICC_AP1R0_EL1);
> > 
> > Why only AP1R0? Does the HW only support 5 bits of priority? If it supports
> > more, you need to check all the registers that may contain an active priority
> > (0xa0 for a standard interrupt, 0x20 for a pNMI).
> > 
> 
> Yes correct. HW supports only 5 bits of priority groups.
> Will note this in the comment.

Thanks.

> 
> > >  	nops(8);
> > >  	irqstat = read_sysreg_s(SYS_ICC_IAR1_EL1);
> > >  	nops(4);
> > >  	mb();
> > >
> > > -	return irqstat;
> > > +	if (likely(apr != read_sysreg_s(SYS_ICC_AP1R0_EL1)))
> > > +		return irqstat;
> > > +
> > > +	return 0x3ff;
> > 
> > This should be ICC_IAR1_EL1_SPURIOUS.
> 
> Looks like we need fixes like below in couple of files to make use
> of this macro.
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 5bc01e62c08a..d02b7339d21a 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -18,7 +18,7 @@
>  #include <linux/kvm_types.h>
>  #include <linux/percpu.h>
>  #include <linux/psci.h>
> -#include <asm/arch_gicv3.h>
> +#include <linux/irqchip/arm-gic-v3.h>
> 
> Should I consider fixing these ? 
> At least  its builds fine for me with similar header fixes.

Ah, crap. I'd like to avoid dragging too much of the linux/*.h into
asm/*.h, as this eventually leads to a pretty terrible mess. Never
mind then. I'll look into fixing it independently, and we'll live with
the 0x3ff for now.

> > > +#define CAVIUM_CPU_PART_THUNDERX_OTX_GEN 0x0A0
> > 
> > Is this an actual part number? What does 'GEN' stand for?
> > 
> 
> No, this is not an actual part number. GEN was meant to be generic
> to cover a group of part numbers.

The problem with that is that it eventually clashes with part numbers
that are allocated later, and your old kernel tries to apply a
workaround on the new HW... Sticking to the actual parts is a lot
safer.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.