Alexandre Courbot
2016-Jun-08 08:32 UTC
[Nouveau] [PATCH 0/4] secboot: be more resilient on errors
This series fixes two cases where behavior on secure boot errors could be improved: 1) Patch 2 propages secure-boot errors from GR init, making sure initialization fails as it should. Failure to do so results in a black screen during boot, as reported in FD bug 94990. 2) Patches 3-4 make the absence of required secure firmware files a non-fatal error. The previous behavior was to give up the whole driver init altogether. Now users can at least enjoy a display even if they are missing the firmware files. On top of that, firmware loading is re-attempted the next time a secure boot function is invoked, also addressing the case where firmware is not available at the time of boot (the first GR init attempt will fail, but the next one will succeed provided the files are now available). Ben, I know we discussed solving problem 2) by moving firmware loading into the secboot constructor, but this is impossible due to the fact firmware loading requires the creation of gpuobj, a feature that is not available at that time. For this reason I have adopted this lazy and actually more resilient strategy. Alexandre Courbot (4): secboot: fix kerneldoc for secure boot structures gr/gf100: handle secure boot errors secboot/gm200: make firmware loading re-callable secboot: lazy-load firmware and be more resilient drm/nouveau/nvkm/engine/gr/gf100.c | 10 ++++- drm/nouveau/nvkm/subdev/secboot/base.c | 9 +---- drm/nouveau/nvkm/subdev/secboot/gm200.c | 72 ++++++++++++++++++++++++--------- drm/nouveau/nvkm/subdev/secboot/gm20b.c | 54 ++++++++++++------------- drm/nouveau/nvkm/subdev/secboot/priv.h | 18 ++++++--- 5 files changed, 102 insertions(+), 61 deletions(-) -- 2.8.3
Alexandre Courbot
2016-Jun-08 08:32 UTC
[Nouveau] [PATCH 1/4] secboot: fix kerneldoc for secure boot structures
Some members were documented in the wrong structure.
Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
---
drm/nouveau/nvkm/subdev/secboot/priv.h | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drm/nouveau/nvkm/subdev/secboot/priv.h
b/drm/nouveau/nvkm/subdev/secboot/priv.h
index f2b09dee7c5d..110ce64888b4 100644
--- a/drm/nouveau/nvkm/subdev/secboot/priv.h
+++ b/drm/nouveau/nvkm/subdev/secboot/priv.h
@@ -147,10 +147,7 @@ struct hsflcn_acr_desc {
* @inst: instance block for HS falcon
* @pgd: page directory for the HS falcon
* @vm: address space used by the HS falcon
- * @bl_desc_size: size of the BL descriptor used by this chip.
- * @fixup_bl_desc: hook that generates the proper BL descriptor format from
- * the generic GM200 format into a data array of size
- * bl_desc_size
+ * @falcon_state: current state of the managed falcons
*/
struct gm200_secboot {
struct nvkm_secboot base;
@@ -199,6 +196,14 @@ struct gm200_secboot {
};
#define gm200_secboot(sb) container_of(sb, struct gm200_secboot, base)
+/**
+ * Contains functions we wish to abstract between GM200-like implementations
+ * @bl_desc_size: size of the BL descriptor used by this chip.
+ * @fixup_bl_desc: hook that generates the proper BL descriptor format from
+ * the generic GM200 format into a data array of size
+ * bl_desc_size
+ * @fixup_hs_desc: hook that twiddles the HS descriptor before it is used
+ */
struct gm200_secboot_func {
/*
* Size of the bootloader descriptor for this chip. A block of this
--
2.8.3
Alexandre Courbot
2016-Jun-08 08:32 UTC
[Nouveau] [PATCH 2/4] gr/gf100: handle secure boot errors
Handle and propagate secure boot errors. Failure to do so results in
Nouveau incorrectly believing init has succeeded and a completely
black display during boot. If we propagate the error, GR init will fail
and the user will at least have a working display.
Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
---
drm/nouveau/nvkm/engine/gr/gf100.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drm/nouveau/nvkm/engine/gr/gf100.c
b/drm/nouveau/nvkm/engine/gr/gf100.c
index 7d14d058b745..157919c788e6 100644
--- a/drm/nouveau/nvkm/engine/gr/gf100.c
+++ b/drm/nouveau/nvkm/engine/gr/gf100.c
@@ -1457,6 +1457,7 @@ gf100_gr_init_ctxctl(struct gf100_gr *gr)
struct nvkm_device *device = subdev->device;
struct nvkm_secboot *sb = device->secboot;
int i;
+ int ret = 0;
if (gr->firmware) {
/* load fuc microcode */
@@ -1464,15 +1465,20 @@ gf100_gr_init_ctxctl(struct gf100_gr *gr)
/* securely-managed falcons must be reset using secure boot */
if (nvkm_secboot_is_managed(sb, NVKM_SECBOOT_FALCON_FECS))
- nvkm_secboot_reset(sb, NVKM_SECBOOT_FALCON_FECS);
+ ret = nvkm_secboot_reset(sb, NVKM_SECBOOT_FALCON_FECS);
else
gf100_gr_init_fw(gr, 0x409000, &gr->fuc409c,
&gr->fuc409d);
+ if (ret)
+ return ret;
+
if (nvkm_secboot_is_managed(sb, NVKM_SECBOOT_FALCON_GPCCS))
- nvkm_secboot_reset(sb, NVKM_SECBOOT_FALCON_GPCCS);
+ ret = nvkm_secboot_reset(sb, NVKM_SECBOOT_FALCON_GPCCS);
else
gf100_gr_init_fw(gr, 0x41a000, &gr->fuc41ac,
&gr->fuc41ad);
+ if (ret)
+ return ret;
nvkm_mc_unk260(device, 1);
--
2.8.3
Alexandre Courbot
2016-Jun-08 08:32 UTC
[Nouveau] [PATCH 3/4] secboot/gm200: make firmware loading re-callable
Make it possible to call gm20x_secboot_prepare_blobs() several times
after either success or failure without re-building already existing
blobs. The function will now try to load firmware files that have
previously failed before returning success.
Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
---
drm/nouveau/nvkm/subdev/secboot/gm200.c | 42 ++++++++++++++++++++-------------
1 file changed, 26 insertions(+), 16 deletions(-)
diff --git a/drm/nouveau/nvkm/subdev/secboot/gm200.c
b/drm/nouveau/nvkm/subdev/secboot/gm200.c
index cc100dc940ea..c6871127c70d 100644
--- a/drm/nouveau/nvkm/subdev/secboot/gm200.c
+++ b/drm/nouveau/nvkm/subdev/secboot/gm200.c
@@ -860,6 +860,8 @@ gm200_secboot_prepare_ls_blob(struct gm200_secboot *gsb)
/* Write LS blob */
ret = ls_ucode_mgr_write_wpr(gsb, &mgr, gsb->ls_blob);
+ if (ret)
+ nvkm_gpuobj_del(&gsb->ls_blob);
cleanup:
ls_ucode_mgr_cleanup(&mgr);
@@ -1023,21 +1025,27 @@ gm20x_secboot_prepare_blobs(struct gm200_secboot *gsb)
int ret;
/* Load and prepare the managed falcon's firmwares */
- ret = gm200_secboot_prepare_ls_blob(gsb);
- if (ret)
- return ret;
+ if (!gsb->ls_blob) {
+ ret = gm200_secboot_prepare_ls_blob(gsb);
+ if (ret)
+ return ret;
+ }
/* Load the HS firmware that will load the LS firmwares */
- ret = gm200_secboot_prepare_hs_blob(gsb, "acr/ucode_load",
- &gsb->acr_load_blob,
- &gsb->acr_load_bl_desc, true);
- if (ret)
- return ret;
+ if (!gsb->acr_load_blob) {
+ ret = gm200_secboot_prepare_hs_blob(gsb, "acr/ucode_load",
+ &gsb->acr_load_blob,
+ &gsb->acr_load_bl_desc, true);
+ if (ret)
+ return ret;
+ }
/* Load the HS firmware bootloader */
- ret = gm200_secboot_prepare_hsbl_blob(gsb);
- if (ret)
- return ret;
+ if (!gsb->hsbl_blob) {
+ ret = gm200_secboot_prepare_hsbl_blob(gsb);
+ if (ret)
+ return ret;
+ }
return 0;
}
@@ -1053,11 +1061,13 @@ gm200_secboot_prepare_blobs(struct nvkm_secboot *sb)
return ret;
/* dGPU only: load the HS firmware that unprotects the WPR region */
- ret = gm200_secboot_prepare_hs_blob(gsb, "acr/ucode_unload",
- &gsb->acr_unload_blob,
- &gsb->acr_unload_bl_desc, false);
- if (ret)
- return ret;
+ if (!gsb->acr_unload_blob) {
+ ret = gm200_secboot_prepare_hs_blob(gsb, "acr/ucode_unload",
+ &gsb->acr_unload_blob,
+ &gsb->acr_unload_bl_desc, false);
+ if (ret)
+ return ret;
+ }
return 0;
}
--
2.8.3
Alexandre Courbot
2016-Jun-08 08:32 UTC
[Nouveau] [PATCH 4/4] secboot: lazy-load firmware and be more resilient
Defer the loading of firmware files to the chip-specific part of secure
boot. This allows implementations to retry loading firmware if the first
attempt failed ; for the GM200 implementation, this happens when trying
to reset a falcon, typically in reaction to GR init.
Firmware loading may fail for a variety of reasons, such as the
filesystem where they reside not being ready at init time. This new
behavior allows GR to be initialized the next time we try to use it if
the firmware has become available.
Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
---
drm/nouveau/nvkm/subdev/secboot/base.c | 9 +-----
drm/nouveau/nvkm/subdev/secboot/gm200.c | 30 ++++++++++++++++--
drm/nouveau/nvkm/subdev/secboot/gm20b.c | 54 ++++++++++++++++-----------------
drm/nouveau/nvkm/subdev/secboot/priv.h | 5 ++-
4 files changed, 59 insertions(+), 39 deletions(-)
diff --git a/drm/nouveau/nvkm/subdev/secboot/base.c
b/drm/nouveau/nvkm/subdev/secboot/base.c
index 57c6a58d1141..314be2192b7d 100644
--- a/drm/nouveau/nvkm/subdev/secboot/base.c
+++ b/drm/nouveau/nvkm/subdev/secboot/base.c
@@ -214,14 +214,7 @@ nvkm_secboot_oneinit(struct nvkm_subdev *subdev)
return ret;
}
- /*
- * Build all blobs - the same blobs can be used to perform secure boot
- * multiple times
- */
- if (sb->func->prepare_blobs)
- ret = sb->func->prepare_blobs(sb);
-
- return ret;
+ return 0;
}
static int
diff --git a/drm/nouveau/nvkm/subdev/secboot/gm200.c
b/drm/nouveau/nvkm/subdev/secboot/gm200.c
index c6871127c70d..d26af8967118 100644
--- a/drm/nouveau/nvkm/subdev/secboot/gm200.c
+++ b/drm/nouveau/nvkm/subdev/secboot/gm200.c
@@ -1051,9 +1051,8 @@ gm20x_secboot_prepare_blobs(struct gm200_secboot *gsb)
}
static int
-gm200_secboot_prepare_blobs(struct nvkm_secboot *sb)
+gm200_secboot_prepare_blobs(struct gm200_secboot *gsb)
{
- struct gm200_secboot *gsb = gm200_secboot(sb);
int ret;
ret = gm20x_secboot_prepare_blobs(gsb);
@@ -1072,6 +1071,26 @@ gm200_secboot_prepare_blobs(struct nvkm_secboot *sb)
return 0;
}
+static int
+gm200_secboot_blobs_ready(struct gm200_secboot *gsb)
+{
+ struct nvkm_subdev *subdev = &gsb->base.subdev;
+ int ret;
+
+ /* firmware already loaded, nothing to do... */
+ if (gsb->firmware_ok)
+ return 0;
+
+ ret = gsb->func->prepare_blobs(gsb);
+ if (ret) {
+ nvkm_error(subdev, "failed to load secure firmware\n");
+ return ret;
+ }
+
+ gsb->firmware_ok = true;
+
+ return 0;
+}
/*
@@ -1244,6 +1263,11 @@ gm200_secboot_reset(struct nvkm_secboot *sb, enum
nvkm_secboot_falcon falcon)
struct gm200_secboot *gsb = gm200_secboot(sb);
int ret;
+ /* Make sure all blobs are ready */
+ ret = gm200_secboot_blobs_ready(gsb);
+ if (ret)
+ return ret;
+
/*
* Dummy GM200 implementation: perform secure boot each time we are
* called on FECS. Since only FECS and GPCCS are managed and started
@@ -1383,7 +1407,6 @@ gm200_secboot = {
.dtor = gm200_secboot_dtor,
.init = gm200_secboot_init,
.fini = gm200_secboot_fini,
- .prepare_blobs = gm200_secboot_prepare_blobs,
.reset = gm200_secboot_reset,
.start = gm200_secboot_start,
.managed_falcons = BIT(NVKM_SECBOOT_FALCON_FECS) |
@@ -1425,6 +1448,7 @@ gm200_secboot_func = {
.bl_desc_size = sizeof(struct gm200_flcn_bl_desc),
.fixup_bl_desc = gm200_secboot_fixup_bl_desc,
.fixup_hs_desc = gm200_secboot_fixup_hs_desc,
+ .prepare_blobs = gm200_secboot_prepare_blobs,
};
int
diff --git a/drm/nouveau/nvkm/subdev/secboot/gm20b.c
b/drm/nouveau/nvkm/subdev/secboot/gm20b.c
index 684320484b70..d5395ebfe8d3 100644
--- a/drm/nouveau/nvkm/subdev/secboot/gm20b.c
+++ b/drm/nouveau/nvkm/subdev/secboot/gm20b.c
@@ -42,6 +42,32 @@ struct gm20b_flcn_bl_desc {
u32 data_size;
};
+static int
+gm20b_secboot_prepare_blobs(struct gm200_secboot *gsb)
+{
+ struct nvkm_subdev *subdev = &gsb->base.subdev;
+ int acr_size;
+ int ret;
+
+ ret = gm20x_secboot_prepare_blobs(gsb);
+ if (ret)
+ return ret;
+
+ acr_size = gsb->acr_load_blob->size;
+ /*
+ * On Tegra the WPR region is set by the bootloader. It is illegal for
+ * the HS blob to be larger than this region.
+ */
+ if (acr_size > gsb->wpr_size) {
+ nvkm_error(subdev, "WPR region too small for FW blob!\n");
+ nvkm_error(subdev, "required: %dB\n", acr_size);
+ nvkm_error(subdev, "WPR size: %dB\n", gsb->wpr_size);
+ return -ENOSPC;
+ }
+
+ return 0;
+}
+
/**
* gm20b_secboot_fixup_bl_desc - adapt BL descriptor to format used by GM20B FW
*
@@ -88,6 +114,7 @@ gm20b_secboot_func = {
.bl_desc_size = sizeof(struct gm20b_flcn_bl_desc),
.fixup_bl_desc = gm20b_secboot_fixup_bl_desc,
.fixup_hs_desc = gm20b_secboot_fixup_hs_desc,
+ .prepare_blobs = gm20b_secboot_prepare_blobs,
};
@@ -147,32 +174,6 @@ gm20b_tegra_read_wpr(struct gm200_secboot *gsb)
#endif
static int
-gm20b_secboot_prepare_blobs(struct nvkm_secboot *sb)
-{
- struct gm200_secboot *gsb = gm200_secboot(sb);
- int acr_size;
- int ret;
-
- ret = gm20x_secboot_prepare_blobs(gsb);
- if (ret)
- return ret;
-
- acr_size = gsb->acr_load_blob->size;
- /*
- * On Tegra the WPR region is set by the bootloader. It is illegal for
- * the HS blob to be larger than this region.
- */
- if (acr_size > gsb->wpr_size) {
- nvkm_error(&sb->subdev, "WPR region too small for FW
blob!\n");
- nvkm_error(&sb->subdev, "required: %dB\n", acr_size);
- nvkm_error(&sb->subdev, "WPR size: %dB\n",
gsb->wpr_size);
- return -ENOSPC;
- }
-
- return 0;
-}
-
-static int
gm20b_secboot_init(struct nvkm_secboot *sb)
{
struct gm200_secboot *gsb = gm200_secboot(sb);
@@ -189,7 +190,6 @@ static const struct nvkm_secboot_func
gm20b_secboot = {
.dtor = gm200_secboot_dtor,
.init = gm20b_secboot_init,
- .prepare_blobs = gm20b_secboot_prepare_blobs,
.reset = gm200_secboot_reset,
.start = gm200_secboot_start,
.managed_falcons = BIT(NVKM_SECBOOT_FALCON_FECS),
diff --git a/drm/nouveau/nvkm/subdev/secboot/priv.h
b/drm/nouveau/nvkm/subdev/secboot/priv.h
index 110ce64888b4..a9a8a0e1017e 100644
--- a/drm/nouveau/nvkm/subdev/secboot/priv.h
+++ b/drm/nouveau/nvkm/subdev/secboot/priv.h
@@ -30,7 +30,6 @@ struct nvkm_secboot_func {
int (*init)(struct nvkm_secboot *);
int (*fini)(struct nvkm_secboot *, bool suspend);
void *(*dtor)(struct nvkm_secboot *);
- int (*prepare_blobs)(struct nvkm_secboot *);
int (*reset)(struct nvkm_secboot *, enum nvkm_secboot_falcon);
int (*start)(struct nvkm_secboot *, enum nvkm_secboot_falcon);
@@ -148,6 +147,7 @@ struct hsflcn_acr_desc {
* @pgd: page directory for the HS falcon
* @vm: address space used by the HS falcon
* @falcon_state: current state of the managed falcons
+ * @firmware_ok: whether the firmware blobs have been created
*/
struct gm200_secboot {
struct nvkm_secboot base;
@@ -193,6 +193,7 @@ struct gm200_secboot {
RUNNING,
} falcon_state[NVKM_SECBOOT_FALCON_END];
+ bool firmware_ok;
};
#define gm200_secboot(sb) container_of(sb, struct gm200_secboot, base)
@@ -203,6 +204,7 @@ struct gm200_secboot {
* the generic GM200 format into a data array of size
* bl_desc_size
* @fixup_hs_desc: hook that twiddles the HS descriptor before it is used
+ * @prepare_blobs: prepares the various blobs needed for secure booting
*/
struct gm200_secboot_func {
/*
@@ -219,6 +221,7 @@ struct gm200_secboot_func {
* we want the HS FW to set up.
*/
void (*fixup_hs_desc)(struct gm200_secboot *, struct hsflcn_acr_desc *);
+ int (*prepare_blobs)(struct gm200_secboot *);
};
int gm200_secboot_init(struct nvkm_secboot *);
--
2.8.3