Nouveau - Jan 2016 - [PATCH v2 2/5] core: add support for secure boot

Hi Alexandre,

On 18 January 2016 at 06:10, Alexandre Courbot <acourbot at nvidia.com>
wrote:

[snip]
> +static const char *
> +managed_falcons_names[] = {
> +       [NVKM_SECBOOT_FALCON_PMU] = "PMU",
> +       [NVKM_SECBOOT_FALCON_RESERVED] = "<invalid>",
"<reserved>" perhaps ? we already have one invalid below.
> +       [NVKM_SECBOOT_FALCON_FECS] = "FECS",
> +       [NVKM_SECBOOT_FALCON_GPCCS] = "GPCCS",
> +       [NVKM_SECBOOT_FALCON_END] = "<invalid>",
> +};
> +
[snip]
> +int
> +nvkm_secboot_ctor(const struct nvkm_secboot_func *func,
> +                 struct nvkm_device *device, int index,
> +                 struct nvkm_secboot *sb)
> +{
> +       unsigned long fid;
> +
> +       nvkm_subdev_ctor(&nvkm_secboot, device, index, 0,
&sb->subdev);
> +       sb->func = func;
> +
Move these two after the switch statement ?
> +       /* setup the performing falcon's base address and masks */
> +       switch (func->boot_falcon) {
> +       case NVKM_SECBOOT_FALCON_PMU:
> +               sb->base = 0x10a000;
> +               sb->irq_mask = 0x1000000;
> +               sb->enable_mask = 0x2000;
> +               break;
> +       default:
> +               nvdev_error(device, "invalid secure boot
falcon\n");
> +               return -EINVAL;
> +       };
> +
> +       nvkm_info(&sb->subdev, "securely managed
falcons:\n");
> +       for_each_set_bit(fid, &sb->func->managed_falcons,
> +                        NVKM_SECBOOT_FALCON_END)
> +               nvkm_info(&sb->subdev, "- %s\n",
managed_falcons_names[fid]);
> +
> +       return 0;
> +}
Cheers,
Emil

On 01/21/2016 10:09 PM, Emil Velikov wrote:
> Hi Alexandre,
> 
> On 18 January 2016 at 06:10, Alexandre Courbot <acourbot at
nvidia.com> wrote:
> 
> [snip]
>> +static const char *
>> +managed_falcons_names[] = {
>> +       [NVKM_SECBOOT_FALCON_PMU] = "PMU",
>> +       [NVKM_SECBOOT_FALCON_RESERVED] = "<invalid>",
> "<reserved>" perhaps ? we already have one invalid below.
Does <reserved> really mean: "we don't want to tell you?"
here? :)
> 
>> +       [NVKM_SECBOOT_FALCON_FECS] = "FECS",
>> +       [NVKM_SECBOOT_FALCON_GPCCS] = "GPCCS",
>> +       [NVKM_SECBOOT_FALCON_END] = "<invalid>",
>> +};
>> +
> 
> [snip]
>> +int
>> +nvkm_secboot_ctor(const struct nvkm_secboot_func *func,
>> +                 struct nvkm_device *device, int index,
>> +                 struct nvkm_secboot *sb)
>> +{
>> +       unsigned long fid;
>> +
>> +       nvkm_subdev_ctor(&nvkm_secboot, device, index, 0,
&sb->subdev);
>> +       sb->func = func;
>> +
> Move these two after the switch statement ?
They need to be done here to make the failure path cleanup stuff work
correctly, so it's correct as-is.
> 
>> +       /* setup the performing falcon's base address and masks */
>> +       switch (func->boot_falcon) {
>> +       case NVKM_SECBOOT_FALCON_PMU:
>> +               sb->base = 0x10a000;
>> +               sb->irq_mask = 0x1000000;
>> +               sb->enable_mask = 0x2000;
>> +               break;
>> +       default:
>> +               nvdev_error(device, "invalid secure boot
falcon\n");
>> +               return -EINVAL;
>> +       };
>> +
>> +       nvkm_info(&sb->subdev, "securely managed
falcons:\n");
>> +       for_each_set_bit(fid, &sb->func->managed_falcons,
>> +                        NVKM_SECBOOT_FALCON_END)
>> +               nvkm_info(&sb->subdev, "- %s\n",
managed_falcons_names[fid]);
>> +
>> +       return 0;
>> +}
> 
> Cheers,
> Emil
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20160121/ea3cc3e2/attachment.sig>

On 21 January 2016 at 12:13, Ben Skeggs <skeggsb at gmail.com>
wrote:
> On 01/21/2016 10:09 PM, Emil Velikov wrote:
>> Hi Alexandre,
>>
>> On 18 January 2016 at 06:10, Alexandre Courbot <acourbot at
nvidia.com> wrote:
>>
>> [snip]
>>> +static const char *
>>> +managed_falcons_names[] = {
>>> +       [NVKM_SECBOOT_FALCON_PMU] = "PMU",
>>> +       [NVKM_SECBOOT_FALCON_RESERVED] =
"<invalid>",
>> "<reserved>" perhaps ? we already have one invalid
below.
> Does <reserved> really mean: "we don't want to tell
you?" here? :)
>
That or we have some secret WIP that we're haven't decided if it'll
work out :-)
>>
>>> +       [NVKM_SECBOOT_FALCON_FECS] = "FECS",
>>> +       [NVKM_SECBOOT_FALCON_GPCCS] = "GPCCS",
>>> +       [NVKM_SECBOOT_FALCON_END] = "<invalid>",
>>> +};
>>> +
>>
>> [snip]
>>> +int
>>> +nvkm_secboot_ctor(const struct nvkm_secboot_func *func,
>>> +                 struct nvkm_device *device, int index,
>>> +                 struct nvkm_secboot *sb)
>>> +{
>>> +       unsigned long fid;
>>> +
>>> +       nvkm_subdev_ctor(&nvkm_secboot, device, index, 0,
&sb->subdev);
>>> +       sb->func = func;
>>> +
>> Move these two after the switch statement ?
> They need to be done here to make the failure path cleanup stuff work
> correctly, so it's correct as-is.
>
| always get confused which ones needed to setup their own dtors and
which ones didn't. Thanks !

-Emil