Re: [PATCH] drm/msm/a6xx: Skip gpu secure fw load in EL2 mode

[Marc Zyngier <maz@kernel.org>]

On Thu, 12 Dec 2024 05:31:00 +0000,
Pavan Kondeti <quic_pkondeti@quicinc.com> wrote:
> 
> On Wed, Dec 11, 2024 at 10:40:02AM +0000, Marc Zyngier wrote:
> > On Wed, 11 Dec 2024 00:37:34 +0000,
> > Pavan Kondeti <quic_pkondeti@quicinc.com> wrote:
> > > 
> > > On Tue, Dec 10, 2024 at 09:24:03PM +0000, Marc Zyngier wrote:
> > > > > +static int a6xx_switch_secure_mode(struct msm_gpu *gpu)
> > > > > +{
> > > > > +	int ret;
> > > > > +
> > > > > +#ifdef CONFIG_ARM64
> > > > > +	/*
> > > > > +	 * We can access SECVID_TRUST_CNTL register when kernel is booted in EL2 mode. So, use it
> > > > > +	 * to switch the secure mode to avoid the dependency on zap shader.
> > > > > +	 */
> > > > > +	if (is_kernel_in_hyp_mode())
> > > > > +		goto direct_switch;
> > > > 
> > > > No, please. To check whether you are *booted* at EL2, you need to
> > > > check for is_hyp_available(). Whether the kernel runs at EL1 or EL2 is
> > > > none of the driver's business, really. This is still absolutely
> > > > disgusting from an abstraction perspective, but I guess we don't have
> > > > much choice here.
> > > > 
> > > 
> > > Thanks Marc. Any suggestions on how we can make is_hyp_mode_available()
> > > available for modules? Do you prefer exporting
> > > kvm_protected_mode_initialized and __boot_cpu_mode symbols directly or
> > > try something like [1]?
> > 
> > Ideally, neither. These were bad ideas nine years ago, and they still
> > are. The least ugly hack I can come up with is the patch below, and
> > you'd write something like:
> > 
> > 	if (cpus_have_cap(ARM64_HAS_EL2_OWNERSHIP))
> > 		blah();
> > 
> > This is obviously completely untested.
> > 
> 
> I have tested your patch. It works as intended. Thanks Marc.

Note that you will probably get some push-back from the arm64
maintainers on this front, because this is a fairly incomplete (and
fragile) solution.

It would be much better if the discriminant came from the device tree.
After all, the hypervisor is fscking-up^W^Wchanging the programming
model of the GPU, and that should be reflected in the DT. Because for
all intent and purposes, this is not the same hardware anymore.

The GPU isn't the only device that needs fixing in that way: the
SMMUv3 needs to be exposed to the OS, and the PCIe ports need to be
linked to it and the ITS. So at the end of the day, detecting EL2 only
serves a limited purpose. You need to handle these cases, and might as
well put the GPU in the same bag.

Which means that you'd either have a pair of static DTs (one that
exposes the brokenness of the firmware, and one that doesn't), or you
go the dtbhack route to compose the DT at boot time.

Thanks,

	M.

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