linux-kernel - Re: [PATCH v2] platform/mellanox: Add bootctl driver for Mellanox BlueField Soc

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 31 Jan 2019 19:02:18 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Liming Sun <lsun@...lanox.com>
Cc:     Andy Shevchenko <andy@...radead.org>,
        Darren Hart <dvhart@...radead.org>,
        Vadim Pasternak <vadimp@...lanox.com>,
        David Woods <dwoods@...lanox.com>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] platform/mellanox: Add bootctl driver for Mellanox
 BlueField Soc

On Thu, Jan 31, 2019 at 6:53 PM Liming Sun <lsun@...lanox.com> wrote:
>
> This commit adds the bootctl platform driver for Mellanox BlueField
> Soc, which controls the eMMC boot partition swapping and sends SMC
> calls to ATF running at exception level EL3 to program some system
> register. This register is only accessible in secure code and is
> used to enable the watchdog after reboot.
>
> Below are the sequences of a typical use case.
>
>   1. User-space tool upgrades one eMMC boot partition and requests
>      the boot partition swapping;
>
>   2. The bootctl driver handles such request and sends SMC call
>      to ATF. ATF programs register BREADCRUMB0 which has value
>      preserved during software reset. It also programs eMMC to
>      swap the boot partition;
>
>   3. After software reset (rebooting), ATF BL1 (BootRom) checks
>      register BREADCRUMB0 to enable watchdog if configured;
>
>   4. If booting fails, the watchdog timer will trigger rebooting.
>      In such case, ATF BootRom will switch the boot partition
>      to the previous one.

Thanks for an update. My comments below.


> Reviewed-by: David Woods <dwoods@...lanox.com>

I'm not sure I see this guy to review v2. Of course if you consider
all changes minor, you may leave this tag.

> Signed-off-by: Liming Sun <lsun@...lanox.com>
> ---

Here should be a changelog what had been done in new version.
> +/* UUID used to probe ATF service. */

> +static const char * const mlxbf_bootctl_svc_uuid_str =
> +       "89c036b4-e7d7-11e6-8797-001aca00bfc4";

static const char *xxx = ...;

> +/*
> + * UUID structure used to match the returned value from ATF in
> + * four 32-bit words. No need to do endian conversion here.
> + */
> +union mlxbf_bootctl_uuid {
> +       guid_t guid;
> +       u32 words[4];
> +};

I'm not sure it's the best you can do. instead of using union, better
to use conversion from and to corresponding uuid type.

The rest I will comment on v3.

-- 
With Best Regards,
Andy Shevchenko