Nouveau - Sep 2019 - [PATCH 03/11] drm/nouveau: secboot: Read WPR configuration from GPU registers

On Tue, 17 Sep 2019 at 01:04, Thierry Reding <thierry.reding at gmail.com>
wrote:
>
> From: Thierry Reding <treding at nvidia.com>
>
> The GPUs found on Tegra SoCs have registers that can be used to read the
> WPR configuration. Use these registers instead of reaching into the
> memory controller's register space to read the same information.
>
> Signed-off-by: Thierry Reding <treding at nvidia.com>
> ---
>  .../drm/nouveau/nvkm/subdev/secboot/gm200.h   |  2 +-
>  .../drm/nouveau/nvkm/subdev/secboot/gm20b.c   | 81 ++++++++++++-------
>  .../drm/nouveau/nvkm/subdev/secboot/gp10b.c   |  4 +-
>  3 files changed, 53 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> index 62c5e162099a..280b1448df88 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> @@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *, struct
nvkm_gpuobj *,
>                            struct nvkm_falcon *);
>
>  /* Tegra-only */
> -int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32);
> +int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
>
>  #endif
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> index df8b919dcf09..f8a543122219 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> @@ -23,39 +23,65 @@
>  #include "acr.h"
>  #include "gm200.h"
>
> -#define TEGRA210_MC_BASE                       0x70019000
> -
>  #ifdef CONFIG_ARCH_TEGRA
> -#define MC_SECURITY_CARVEOUT2_CFG0             0xc58
> -#define MC_SECURITY_CARVEOUT2_BOM_0            0xc5c
> -#define MC_SECURITY_CARVEOUT2_BOM_HI_0         0xc60
> -#define MC_SECURITY_CARVEOUT2_SIZE_128K                0xc64
> -#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED  (1 << 1)
>  /**
>   * gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra
>   *
> - * On dGPU, we can manage the WPR region ourselves, but on Tegra the WPR
region
> - * is reserved from system memory by the bootloader and irreversibly
locked.
> - * This function reads the address and size of the pre-configured WPR
region.
> + * On dGPU, we can manage the WPR region ourselves, but on Tegra this
region
> + * is allocated from system memory by the secure firmware. The region is
then
> + * marked as a "secure carveout" and irreversibly locked.
Furthermore, the WPR
> + * secure carveout is also configured to be sent to the GPU via a
dedicated
> + * serial bus between the memory controller and the GPU. The GPU requests
this
> + * information upon leaving reset and exposes it through a FIFO register
at
> + * offset 0x100cd4.
> + *
> + * The FIFO register's lower 4 bits can be used to set the read index
into the
> + * FIFO. After each read of the FIFO register, the read index is
incremented.
> + *
> + * Indices 2 and 3 contain the lower and upper addresses of the WPR. These
are
> + * stored in units of 256 B. The WPR is inclusive of both addresses.
> + *
> + * Unfortunately, for some reason the WPR info register doesn't
contain the
> + * correct values for the secure carveout. It seems like the upper address
is
> + * always too small by 128 KiB - 1. Given that the secure carvout size in
the
> + * memory controller configuration is specified in units of 128 KiB,
it's
> + * possible that the computation of the upper address of the WPR is wrong
and
> + * causes this difference.
>   */
>  int
> -gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base)
> +gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb)
>  {
> +       struct nvkm_device *device = gsb->base.subdev.device;
>         struct nvkm_secboot *sb = &gsb->base;
> -       void __iomem *mc;
> -       u32 cfg;
> +       u64 base, limit;
> +       u32 value;
>
> -       mc = ioremap(mc_base, 0xd00);
> -       if (!mc) {
> -               nvkm_error(&sb->subdev, "Cannot map Tegra MC
registers\n");
> -               return -ENOMEM;
> -       }
> -       sb->wpr_addr = ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_0)
|
> -             ((u64)ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_HI_0)
<< 32);
> -       sb->wpr_size = ioread32_native(mc +
MC_SECURITY_CARVEOUT2_SIZE_128K)
> -               << 17;
> -       cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);
> -       iounmap(mc);
> +       /* set WPR info register to point at WPR base address register */
> +       value = nvkm_rd32(device, 0x100cd4);
> +       value &= ~0xf;
> +       value |= 0x2;
> +       nvkm_wr32(device, 0x100cd4, value);
> +
> +       /* read base address */
> +       value = nvkm_rd32(device, 0x100cd4);
> +       base = (u64)(value >> 4) << 12;
> +
> +       /* read limit */
> +       value = nvkm_rd32(device, 0x100cd4);
> +       limit = (u64)(value >> 4) << 12;
acr_r352_wpr_is_set() does a similar readout to confirm the HS
firmware did its job on dGPU, perhaps this part of it could be
factored out into a function that could be used in both places?
> +
> +       /*
> +        * The upper address of the WPR seems to be computed wrongly and is
> +        * actually SZ_128K - 1 bytes lower than it should be. Adjust the
> +        * value accordingly.
> +        */
> +       limit += SZ_128K - 1;
> +
> +       sb->wpr_size = limit - base + 1;
> +       sb->wpr_addr = base;
> +
> +       nvkm_info(&sb->subdev, "WPR: %016llx-%016llx\n",
sb->wpr_addr,
> +                 sb->wpr_addr + sb->wpr_size - 1);
>
>         /* Check that WPR settings are valid */
>         if (sb->wpr_size == 0) {
> @@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb,
u32 mc_base)
>                 return -EINVAL;
>         }
>
> -       if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {
> -               nvkm_error(&sb->subdev, "WPR region not
locked\n");
> -               return -EINVAL;
> -       }
> -
>         return 0;
>  }
>  #else
> @@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb)
>         struct gm200_secboot *gsb = gm200_secboot(sb);
>         int ret;
>
> -       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE);
> +       ret = gm20b_secboot_tegra_read_wpr(gsb);
>         if (ret)
>                 return ret;
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> index 28ca29d0eeee..d84e85825995 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> @@ -23,15 +23,13 @@
>  #include "acr.h"
>  #include "gm200.h"
>
> -#define TEGRA186_MC_BASE                       0x02c10000
> -
>  static int
>  gp10b_secboot_oneinit(struct nvkm_secboot *sb)
>  {
>         struct gm200_secboot *gsb = gm200_secboot(sb);
>         int ret;
>
> -       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE);
> +       ret = gm20b_secboot_tegra_read_wpr(gsb);
>         if (ret)
>                 return ret;
>
> --
> 2.23.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Tue, Sep 17, 2019 at 01:49:57PM +1000, Ben Skeggs
wrote:
> On Tue, 17 Sep 2019 at 01:04, Thierry Reding <thierry.reding at
gmail.com> wrote:
> >
> > From: Thierry Reding <treding at nvidia.com>
> >
> > The GPUs found on Tegra SoCs have registers that can be used to read
the
> > WPR configuration. Use these registers instead of reaching into the
> > memory controller's register space to read the same information.
> >
> > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > ---
> >  .../drm/nouveau/nvkm/subdev/secboot/gm200.h   |  2 +-
> >  .../drm/nouveau/nvkm/subdev/secboot/gm20b.c   | 81
++++++++++++-------
> >  .../drm/nouveau/nvkm/subdev/secboot/gp10b.c   |  4 +-
> >  3 files changed, 53 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> > index 62c5e162099a..280b1448df88 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> > @@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *,
struct nvkm_gpuobj *,
> >                            struct nvkm_falcon *);
> >
> >  /* Tegra-only */
> > -int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32);
> > +int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
> >
> >  #endif
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> > index df8b919dcf09..f8a543122219 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> > @@ -23,39 +23,65 @@
> >  #include "acr.h"
> >  #include "gm200.h"
> >
> > -#define TEGRA210_MC_BASE                       0x70019000
> > -
> >  #ifdef CONFIG_ARCH_TEGRA
> > -#define MC_SECURITY_CARVEOUT2_CFG0             0xc58
> > -#define MC_SECURITY_CARVEOUT2_BOM_0            0xc5c
> > -#define MC_SECURITY_CARVEOUT2_BOM_HI_0         0xc60
> > -#define MC_SECURITY_CARVEOUT2_SIZE_128K                0xc64
> > -#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED  (1 << 1)
> >  /**
> >   * gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra
> >   *
> > - * On dGPU, we can manage the WPR region ourselves, but on Tegra the
WPR region
> > - * is reserved from system memory by the bootloader and irreversibly
locked.
> > - * This function reads the address and size of the pre-configured WPR
region.
> > + * On dGPU, we can manage the WPR region ourselves, but on Tegra this
region
> > + * is allocated from system memory by the secure firmware. The region
is then
> > + * marked as a "secure carveout" and irreversibly locked.
Furthermore, the WPR
> > + * secure carveout is also configured to be sent to the GPU via a
dedicated
> > + * serial bus between the memory controller and the GPU. The GPU
requests this
> > + * information upon leaving reset and exposes it through a FIFO
register at
> > + * offset 0x100cd4.
> > + *
> > + * The FIFO register's lower 4 bits can be used to set the read
index into the
> > + * FIFO. After each read of the FIFO register, the read index is
incremented.
> > + *
> > + * Indices 2 and 3 contain the lower and upper addresses of the WPR.
These are
> > + * stored in units of 256 B. The WPR is inclusive of both addresses.
> > + *
> > + * Unfortunately, for some reason the WPR info register doesn't
contain the
> > + * correct values for the secure carveout. It seems like the upper
address is
> > + * always too small by 128 KiB - 1. Given that the secure carvout
size in the
> > + * memory controller configuration is specified in units of 128 KiB,
it's
> > + * possible that the computation of the upper address of the WPR is
wrong and
> > + * causes this difference.
> >   */
> >  int
> > -gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base)
> > +gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb)
> >  {
> > +       struct nvkm_device *device = gsb->base.subdev.device;
> >         struct nvkm_secboot *sb = &gsb->base;
> > -       void __iomem *mc;
> > -       u32 cfg;
> > +       u64 base, limit;
> > +       u32 value;
> >
> > -       mc = ioremap(mc_base, 0xd00);
> > -       if (!mc) {
> > -               nvkm_error(&sb->subdev, "Cannot map Tegra
MC registers\n");
> > -               return -ENOMEM;
> > -       }
> > -       sb->wpr_addr = ioread32_native(mc +
MC_SECURITY_CARVEOUT2_BOM_0) |
> > -             ((u64)ioread32_native(mc +
MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32);
> > -       sb->wpr_size = ioread32_native(mc +
MC_SECURITY_CARVEOUT2_SIZE_128K)
> > -               << 17;
> > -       cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);
> > -       iounmap(mc);
> > +       /* set WPR info register to point at WPR base address register
*/
> > +       value = nvkm_rd32(device, 0x100cd4);
> > +       value &= ~0xf;
> > +       value |= 0x2;
> > +       nvkm_wr32(device, 0x100cd4, value);
> > +
> > +       /* read base address */
> > +       value = nvkm_rd32(device, 0x100cd4);
> > +       base = (u64)(value >> 4) << 12;
> > +
> > +       /* read limit */
> > +       value = nvkm_rd32(device, 0x100cd4);
> > +       limit = (u64)(value >> 4) << 12;
> acr_r352_wpr_is_set() does a similar readout to confirm the HS
> firmware did its job on dGPU, perhaps this part of it could be
> factored out into a function that could be used in both places?
I hadn't seen that function. It looks to me like these are now both
doing exactly the same thing. The acr_r352_wpr_is_set() also seems to
serve only to check that what's read from these WPR info registers
matches what was configured as the WPR region previously. This is now
rather pointless because, unless the computations differ, the result
must be true.

Honestly, I'm not sure we really need to check this. My understanding is
that these WPR info registers are the canonical way of obtaining the WPR
region base and limit. The way that this works is that the GPU has a
dedicated bus to the memory controller and it fetches these parameters
from the MC when it leaves reset.

One oddity here, though, is that the code in acr_r352_wpr_is_set() does
not account for the strange way that the limit is encoded in these
registers. I suspect that this is some weird hardware bug that happens
during the transfer of the WPR information to the GPU. I came across
some documentation that specifically defines the limit register to
contain the upper limit of the WPR in units of 256 B with the WPR being
inclusive of both the base and the limit. I'm not exactly sure why this
has gone unnoticed, but I think nvgpu doesn't actually test for the WPR
size when it loads the firmware. I only ran into this when implementing
the WPR info register readout because Nouveau would refuse to load the
firmware because it didn't fit into what it thought was the WPR.

Anyway, I've tested this on all of gm20b, gp10b and gv11b and the above
computations give me the same values that the (SoC) firmware claims that
it configured the WPR with.

Given the above, do you see any further use for acr_r352_wpr_is_set()?
Should I follow up with a patch removing it?

Thierry
> 
> > +
> > +       /*
> > +        * The upper address of the WPR seems to be computed wrongly
and is
> > +        * actually SZ_128K - 1 bytes lower than it should be. Adjust
the
> > +        * value accordingly.
> > +        */
> > +       limit += SZ_128K - 1;
> > +
> > +       sb->wpr_size = limit - base + 1;
> > +       sb->wpr_addr = base;
> > +
> > +       nvkm_info(&sb->subdev, "WPR:
%016llx-%016llx\n", sb->wpr_addr,
> > +                 sb->wpr_addr + sb->wpr_size - 1);
> >
> >         /* Check that WPR settings are valid */
> >         if (sb->wpr_size == 0) {
> > @@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot
*gsb, u32 mc_base)
> >                 return -EINVAL;
> >         }
> >
> > -       if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {
> > -               nvkm_error(&sb->subdev, "WPR region not
locked\n");
> > -               return -EINVAL;
> > -       }
> > -
> >         return 0;
> >  }
> >  #else
> > @@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb)
> >         struct gm200_secboot *gsb = gm200_secboot(sb);
> >         int ret;
> >
> > -       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE);
> > +       ret = gm20b_secboot_tegra_read_wpr(gsb);
> >         if (ret)
> >                 return ret;
> >
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> > index 28ca29d0eeee..d84e85825995 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> > @@ -23,15 +23,13 @@
> >  #include "acr.h"
> >  #include "gm200.h"
> >
> > -#define TEGRA186_MC_BASE                       0x02c10000
> > -
> >  static int
> >  gp10b_secboot_oneinit(struct nvkm_secboot *sb)
> >  {
> >         struct gm200_secboot *gsb = gm200_secboot(sb);
> >         int ret;
> >
> > -       ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE);
> > +       ret = gm20b_secboot_tegra_read_wpr(gsb);
> >         if (ret)
> >                 return ret;
> >
> > --
> > 2.23.0
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20190917/fe36fe0d/attachment.sig>

On Tue, 17 Sep 2019 at 18:40, Thierry Reding <thierry.reding at gmail.com>
wrote:
>
> On Tue, Sep 17, 2019 at 01:49:57PM +1000, Ben Skeggs wrote:
> > On Tue, 17 Sep 2019 at 01:04, Thierry Reding <thierry.reding at
gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding at nvidia.com>
> > >
> > > The GPUs found on Tegra SoCs have registers that can be used to
read the
> > > WPR configuration. Use these registers instead of reaching into
the
> > > memory controller's register space to read the same
information.
> > >
> > > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > > ---
> > >  .../drm/nouveau/nvkm/subdev/secboot/gm200.h   |  2 +-
> > >  .../drm/nouveau/nvkm/subdev/secboot/gm20b.c   | 81
++++++++++++-------
> > >  .../drm/nouveau/nvkm/subdev/secboot/gp10b.c   |  4 +-
> > >  3 files changed, 53 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> > > index 62c5e162099a..280b1448df88 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h
> > > @@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot
*, struct nvkm_gpuobj *,
> > >                            struct nvkm_falcon *);
> > >
> > >  /* Tegra-only */
> > > -int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32);
> > > +int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
> > >
> > >  #endif
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> > > index df8b919dcf09..f8a543122219 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c
> > > @@ -23,39 +23,65 @@
> > >  #include "acr.h"
> > >  #include "gm200.h"
> > >
> > > -#define TEGRA210_MC_BASE                       0x70019000
> > > -
> > >  #ifdef CONFIG_ARCH_TEGRA
> > > -#define MC_SECURITY_CARVEOUT2_CFG0             0xc58
> > > -#define MC_SECURITY_CARVEOUT2_BOM_0            0xc5c
> > > -#define MC_SECURITY_CARVEOUT2_BOM_HI_0         0xc60
> > > -#define MC_SECURITY_CARVEOUT2_SIZE_128K                0xc64
> > > -#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED  (1 << 1)
> > >  /**
> > >   * gm20b_secboot_tegra_read_wpr() - read the WPR registers on
Tegra
> > >   *
> > > - * On dGPU, we can manage the WPR region ourselves, but on Tegra
the WPR region
> > > - * is reserved from system memory by the bootloader and
irreversibly locked.
> > > - * This function reads the address and size of the
pre-configured WPR region.
> > > + * On dGPU, we can manage the WPR region ourselves, but on Tegra
this region
> > > + * is allocated from system memory by the secure firmware. The
region is then
> > > + * marked as a "secure carveout" and irreversibly
locked. Furthermore, the WPR
> > > + * secure carveout is also configured to be sent to the GPU via
a dedicated
> > > + * serial bus between the memory controller and the GPU. The GPU
requests this
> > > + * information upon leaving reset and exposes it through a FIFO
register at
> > > + * offset 0x100cd4.
> > > + *
> > > + * The FIFO register's lower 4 bits can be used to set the
read index into the
> > > + * FIFO. After each read of the FIFO register, the read index is
incremented.
> > > + *
> > > + * Indices 2 and 3 contain the lower and upper addresses of the
WPR. These are
> > > + * stored in units of 256 B. The WPR is inclusive of both
addresses.
> > > + *
> > > + * Unfortunately, for some reason the WPR info register
doesn't contain the
> > > + * correct values for the secure carveout. It seems like the
upper address is
> > > + * always too small by 128 KiB - 1. Given that the secure
carvout size in the
> > > + * memory controller configuration is specified in units of 128
KiB, it's
> > > + * possible that the computation of the upper address of the WPR
is wrong and
> > > + * causes this difference.
> > >   */
> > >  int
> > > -gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32
mc_base)
> > > +gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb)
> > >  {
> > > +       struct nvkm_device *device = gsb->base.subdev.device;
> > >         struct nvkm_secboot *sb = &gsb->base;
> > > -       void __iomem *mc;
> > > -       u32 cfg;
> > > +       u64 base, limit;
> > > +       u32 value;
> > >
> > > -       mc = ioremap(mc_base, 0xd00);
> > > -       if (!mc) {
> > > -               nvkm_error(&sb->subdev, "Cannot map
Tegra MC registers\n");
> > > -               return -ENOMEM;
> > > -       }
> > > -       sb->wpr_addr = ioread32_native(mc +
MC_SECURITY_CARVEOUT2_BOM_0) |
> > > -             ((u64)ioread32_native(mc +
MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32);
> > > -       sb->wpr_size = ioread32_native(mc +
MC_SECURITY_CARVEOUT2_SIZE_128K)
> > > -               << 17;
> > > -       cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);
> > > -       iounmap(mc);
> > > +       /* set WPR info register to point at WPR base address
register */
> > > +       value = nvkm_rd32(device, 0x100cd4);
> > > +       value &= ~0xf;
> > > +       value |= 0x2;
> > > +       nvkm_wr32(device, 0x100cd4, value);
> > > +
> > > +       /* read base address */
> > > +       value = nvkm_rd32(device, 0x100cd4);
> > > +       base = (u64)(value >> 4) << 12;
> > > +
> > > +       /* read limit */
> > > +       value = nvkm_rd32(device, 0x100cd4);
> > > +       limit = (u64)(value >> 4) << 12;
> > acr_r352_wpr_is_set() does a similar readout to confirm the HS
> > firmware did its job on dGPU, perhaps this part of it could be
> > factored out into a function that could be used in both places?
>
> I hadn't seen that function. It looks to me like these are now both
> doing exactly the same thing. The acr_r352_wpr_is_set() also seems to
> serve only to check that what's read from these WPR info registers
> matches what was configured as the WPR region previously. This is now
> rather pointless because, unless the computations differ, the result
> must be true.
Yeah, I believe its purpose is simply to confirm the HS firmware
executed correctly.
>
> Honestly, I'm not sure we really need to check this. My understanding
is
> that these WPR info registers are the canonical way of obtaining the WPR
> region base and limit. The way that this works is that the GPU has a
> dedicated bus to the memory controller and it fetches these parameters
> from the MC when it leaves reset.
>
> One oddity here, though, is that the code in acr_r352_wpr_is_set() does
> not account for the strange way that the limit is encoded in these
> registers. I suspect that this is some weird hardware bug that happens
> during the transfer of the WPR information to the GPU. I came across
> some documentation that specifically defines the limit register to
> contain the upper limit of the WPR in units of 256 B with the WPR being
> inclusive of both the base and the limit. I'm not exactly sure why this
> has gone unnoticed, but I think nvgpu doesn't actually test for the WPR
> size when it loads the firmware. I only ran into this when implementing
> the WPR info register readout because Nouveau would refuse to load the
> firmware because it didn't fit into what it thought was the WPR.
>
> Anyway, I've tested this on all of gm20b, gp10b and gv11b and the above
> computations give me the same values that the (SoC) firmware claims that
> it configured the WPR with.
>
> Given the above, do you see any further use for acr_r352_wpr_is_set()?
> Should I follow up with a patch removing it?
You can leave it for now if you like, I'm reworking that entire
subsystem already anyway and can nuke it there.

Ben.
>
> Thierry
>
> >
> > > +
> > > +       /*
> > > +        * The upper address of the WPR seems to be computed
wrongly and is
> > > +        * actually SZ_128K - 1 bytes lower than it should be.
Adjust the
> > > +        * value accordingly.
> > > +        */
> > > +       limit += SZ_128K - 1;
> > > +
> > > +       sb->wpr_size = limit - base + 1;
> > > +       sb->wpr_addr = base;
> > > +
> > > +       nvkm_info(&sb->subdev, "WPR:
%016llx-%016llx\n", sb->wpr_addr,
> > > +                 sb->wpr_addr + sb->wpr_size - 1);
> > >
> > >         /* Check that WPR settings are valid */
> > >         if (sb->wpr_size == 0) {
> > > @@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct
gm200_secboot *gsb, u32 mc_base)
> > >                 return -EINVAL;
> > >         }
> > >
> > > -       if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {
> > > -               nvkm_error(&sb->subdev, "WPR region
not locked\n");
> > > -               return -EINVAL;
> > > -       }
> > > -
> > >         return 0;
> > >  }
> > >  #else
> > > @@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb)
> > >         struct gm200_secboot *gsb = gm200_secboot(sb);
> > >         int ret;
> > >
> > > -       ret = gm20b_secboot_tegra_read_wpr(gsb,
TEGRA210_MC_BASE);
> > > +       ret = gm20b_secboot_tegra_read_wpr(gsb);
> > >         if (ret)
> > >                 return ret;
> > >
> > > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> > > index 28ca29d0eeee..d84e85825995 100644
> > > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> > > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c
> > > @@ -23,15 +23,13 @@
> > >  #include "acr.h"
> > >  #include "gm200.h"
> > >
> > > -#define TEGRA186_MC_BASE                       0x02c10000
> > > -
> > >  static int
> > >  gp10b_secboot_oneinit(struct nvkm_secboot *sb)
> > >  {
> > >         struct gm200_secboot *gsb = gm200_secboot(sb);
> > >         int ret;
> > >
> > > -       ret = gm20b_secboot_tegra_read_wpr(gsb,
TEGRA186_MC_BASE);
> > > +       ret = gm20b_secboot_tegra_read_wpr(gsb);
> > >         if (ret)
> > >                 return ret;
> > >
> > > --
> > > 2.23.0
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel