| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAF6AEGvBU8mB5HHNK4nP5h9ouKfLNhCMFgF+PhrC77ZMfSOTkQ@mail.gmail.com>
Date: Mon, 9 Dec 2024 13:56:44 -0800
From: Rob Clark <robdclark@...il.com>
To: Akhil P Oommen <quic_akhilpo@...cinc.com>
Cc: Pavan Kondeti <quic_pkondeti@...cinc.com>, Sean Paul <sean@...rly.run>,
Konrad Dybcio <konradybcio@...nel.org>, Abhinav Kumar <quic_abhinavk@...cinc.com>,
Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Marijn Suijten <marijn.suijten@...ainline.org>, David Airlie <airlied@...il.com>,
Simona Vetter <simona@...ll.ch>, Elliot Berman <quic_eberman@...cinc.com>,
linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] drm/msm/a6xx: Skip gpu secure fw load in EL2 mode
On Mon, Dec 9, 2024 at 12:52 PM Akhil P Oommen <quic_akhilpo@...cinc.com> wrote:
>
> On 12/10/2024 1:24 AM, Rob Clark wrote:
> > On Mon, Dec 9, 2024 at 12:20 AM Akhil P Oommen <quic_akhilpo@...cinc.com> wrote:
> >>
> >> When kernel is booted in EL2, SECVID registers are accessible to the
> >> KMD. So we can use that to switch GPU's secure mode to avoid dependency
> >> on Zap firmware. Also, we can't load a secure firmware without a
> >> hypervisor that supports it.
> >
> > Shouldn't we do this based on whether zap node is in dtb (and not disabled)?
>
> This is better, isn't it? Otherwise, multiple overlays should be
> maintained for each soc/board since EL2 can be toggled from bootloader.
> And this feature is likely going to be more widely available.
I guess the first question is what the dt should look like. I think
it makes sense to not have a zap node when booting in EL2 (or at least
disabling it) because that describes the hw+fw situation. And in any
case, so far it seems like we often need unrelated changes[1]. But
maybe others have differing opinions.
And depending on how much cooperation we get from the bootloader, it
could be that our hand is forced. I figured I should at least point
out how we currently handle this.
A further point, I suppose it is in theory possible that a device
could have no secure playback support, despite booting in EL1? So
tying this to EL2 seems a bit contrived.
BR,
-R
[1] https://github.com/TravMurav/slbounce/blob/main/dtbo/x1e-el2.dtso
> -Akhil.
>
> >
> > slbounce applies some dtb overlays to disable the zap node when
> > booting in EL2 (and make some other changes due to kernel being in
> > control of the pci smmuv3, or something along those lines).
> >
> > BR,
> > -R
> >
> >>
> >> Tested following configurations on sa8775p chipset (Adreno 663 gpu):
> >>
> >> 1. Gunyah (No KVM) - Loads zap shader based on DT
> >> 2. KVM in VHE - Skips zap shader load and programs SECVID register
> >> 3. KVM in nVHE - Loads zap shader based on DT
> >> 4. Kernel in EL2 with CONFIG_KVM=n - Skips zap shader load and
> >> programs SECVID register
> >>
> >> For (1) and (3) configuration, this patch doesn't have any impact.
> >> Driver loads secure firmware based on other existing hints.
> >>
> >> Signed-off-by: Akhil P Oommen <quic_akhilpo@...cinc.com>
> >> ---
> >> ---
> >> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 82 +++++++++++++++++++++++------------
> >> 1 file changed, 54 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >> index 019610341df1..9dcaa8472430 100644
> >> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> >> @@ -14,6 +14,10 @@
> >> #include <linux/pm_domain.h>
> >> #include <linux/soc/qcom/llcc-qcom.h>
> >>
> >> +#ifdef CONFIG_ARM64
> >> +#include <asm/virt.h>
> >> +#endif
> >> +
> >> #define GPU_PAS_ID 13
> >>
> >> static inline bool _a6xx_check_idle(struct msm_gpu *gpu)
> >> @@ -998,6 +1002,54 @@ static int a6xx_zap_shader_init(struct msm_gpu *gpu)
> >> return ret;
> >> }
> >>
> >> +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;
> >> +#endif
> >> +
> >> + /*
> >> + * Try to load a zap shader into the secure world. If successful
> >> + * we can use the CP to switch out of secure mode. If not then we
> >> + * have no resource but to try to switch ourselves out manually. If we
> >> + * guessed wrong then access to the RBBM_SECVID_TRUST_CNTL register will
> >> + * be blocked and a permissions violation will soon follow.
> >> + */
> >> + ret = a6xx_zap_shader_init(gpu);
> >> + if (ret == -ENODEV) {
> >> + /*
> >> + * This device does not use zap shader (but print a warning
> >> + * just in case someone got their dt wrong.. hopefully they
> >> + * have a debug UART to realize the error of their ways...
> >> + * if you mess this up you are about to crash horribly)
> >> + */
> >> + dev_warn_once(gpu->dev->dev,
> >> + "Zap shader not enabled - using SECVID_TRUST_CNTL instead\n");
> >> + goto direct_switch;
> >> + } else if (ret)
> >> + return ret;
> >> +
> >> + OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
> >> + OUT_RING(gpu->rb[0], 0x00000000);
> >> +
> >> + a6xx_flush(gpu, gpu->rb[0]);
> >> + if (!a6xx_idle(gpu, gpu->rb[0]))
> >> + return -EINVAL;
> >> +
> >> + return 0;
> >> +
> >> +direct_switch:
> >> + gpu_write(gpu, REG_A6XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> >> + return 0;
> >> +}
> >> +
> >> #define A6XX_INT_MASK (A6XX_RBBM_INT_0_MASK_CP_AHB_ERROR | \
> >> A6XX_RBBM_INT_0_MASK_RBBM_ATB_ASYNCFIFO_OVERFLOW | \
> >> A6XX_RBBM_INT_0_MASK_CP_HW_ERROR | \
> >> @@ -1341,35 +1393,9 @@ static int hw_init(struct msm_gpu *gpu)
> >> if (ret)
> >> goto out;
> >>
> >> - /*
> >> - * Try to load a zap shader into the secure world. If successful
> >> - * we can use the CP to switch out of secure mode. If not then we
> >> - * have no resource but to try to switch ourselves out manually. If we
> >> - * guessed wrong then access to the RBBM_SECVID_TRUST_CNTL register will
> >> - * be blocked and a permissions violation will soon follow.
> >> - */
> >> - ret = a6xx_zap_shader_init(gpu);
> >> - if (!ret) {
> >> - OUT_PKT7(gpu->rb[0], CP_SET_SECURE_MODE, 1);
> >> - OUT_RING(gpu->rb[0], 0x00000000);
> >> -
> >> - a6xx_flush(gpu, gpu->rb[0]);
> >> - if (!a6xx_idle(gpu, gpu->rb[0]))
> >> - return -EINVAL;
> >> - } else if (ret == -ENODEV) {
> >> - /*
> >> - * This device does not use zap shader (but print a warning
> >> - * just in case someone got their dt wrong.. hopefully they
> >> - * have a debug UART to realize the error of their ways...
> >> - * if you mess this up you are about to crash horribly)
> >> - */
> >> - dev_warn_once(gpu->dev->dev,
> >> - "Zap shader not enabled - using SECVID_TRUST_CNTL instead\n");
> >> - gpu_write(gpu, REG_A6XX_RBBM_SECVID_TRUST_CNTL, 0x0);
> >> - ret = 0;
> >> - } else {
> >> + ret = a6xx_switch_secure_mode(gpu);
> >> + if (!ret)
> >> return ret;
> >> - }
> >>
> >> out:
> >> if (adreno_has_gmu_wrapper(adreno_gpu))
> >>
> >> ---
> >> base-commit: f4a867a46862c1743501bbe8c813238456ec8699
> >> change-id: 20241120-drm-msm-kvm-support-cd6e6744ced6
> >>
> >> Best regards,
> >> --
> >> Akhil P Oommen <quic_akhilpo@...cinc.com>
> >>
>
Powered by blists - more mailing lists