Re: [PATCH 3/3] aspeed: sbc: Allow per-machine settings

[Peter Delevoryas]

On Fri, Jul 01, 2022 at 07:23:58AM +0200, Cédric Le Goater wrote:
> On 7/1/22 03:36, Peter Delevoryas wrote:
> > On Tue, Jun 28, 2022 at 05:47:40PM +0200, Cédric Le Goater wrote:
> > > From: Joel Stanley <joel@jms.id.au>
> > > 
> > > In order to correctly report secure boot running firmware the values
> > > of certain registers must be set.
> > > 
> > > We don't yet have documentation from ASPEED on what they mean. The
> > > meaning is inferred from u-boot's use of them.
> > > 
> > > Introduce properties so the settings can be configured per-machine.
> > > 
> > > Signed-off-by: Joel Stanley <joel@jms.id.au>
> > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > ---
> > >   include/hw/misc/aspeed_sbc.h | 13 ++++++++++++
> > >   hw/misc/aspeed_sbc.c         | 41 ++++++++++++++++++++++++++++++++++--
> > >   2 files changed, 52 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/include/hw/misc/aspeed_sbc.h b/include/hw/misc/aspeed_sbc.h
> > > index 67e43b53ecc3..405e6782b97a 100644
> > > --- a/include/hw/misc/aspeed_sbc.h
> > > +++ b/include/hw/misc/aspeed_sbc.h
> > > @@ -17,9 +17,22 @@ OBJECT_DECLARE_TYPE(AspeedSBCState, AspeedSBCClass, 
> > > ASPEED_SBC)
> > >   #define ASPEED_SBC_NR_REGS (0x93c >> 2)
> > > +#define QSR_AES                     BIT(27)
> > > +#define QSR_RSA1024                 (0x0 << 12)
> > > +#define QSR_RSA2048                 (0x1 << 12)
> > > +#define QSR_RSA3072                 (0x2 << 12)
> > > +#define QSR_RSA4096                 (0x3 << 12)
> > > +#define QSR_SHA224                  (0x0 << 10)
> > > +#define QSR_SHA256                  (0x1 << 10)
> > > +#define QSR_SHA384                  (0x2 << 10)
> > > +#define QSR_SHA512                  (0x3 << 10)
> > > +
> > >   struct AspeedSBCState {
> > >       SysBusDevice parent;
> > > +    bool emmc_abr;
> > > +    uint32_t signing_settings;
> > > +
> > >       MemoryRegion iomem;
> > >       uint32_t regs[ASPEED_SBC_NR_REGS];
> > > diff --git a/hw/misc/aspeed_sbc.c b/hw/misc/aspeed_sbc.c
> > > index bfa8b81d01c7..3946e6179bdd 100644
> > > --- a/hw/misc/aspeed_sbc.c
> > > +++ b/hw/misc/aspeed_sbc.c
> > > @@ -11,6 +11,7 @@
> > >   #include "qemu/osdep.h"
> > >   #include "qemu/log.h"
> > >   #include "qemu/error-report.h"
> > > +#include "hw/qdev-properties.h"
> > >   #include "hw/misc/aspeed_sbc.h"
> > >   #include "qapi/error.h"
> > >   #include "migration/vmstate.h"
> > > @@ -19,6 +20,27 @@
> > >   #define R_STATUS        (0x014 / 4)
> > >   #define R_QSR           (0x040 / 4)
> > > +/* R_STATUS */
> > > +#define ABR_EN                  BIT(14) /* Mirrors SCU510[11] */
> > > +#define ABR_IMAGE_SOURCE        BIT(13)
> > > +#define SPI_ABR_IMAGE_SOURCE    BIT(12)
> > > +#define SB_CRYPTO_KEY_EXP_DONE  BIT(11)
> > > +#define SB_CRYPTO_BUSY          BIT(10)
> > > +#define OTP_WP_EN               BIT(9)
> > > +#define OTP_ADDR_WP_EN          BIT(8)
> > > +#define LOW_SEC_KEY_EN          BIT(7)
> > > +#define SECURE_BOOT_EN          BIT(6)
> > > +#define UART_BOOT_EN            BIT(5)
> > > +/* bit 4 reserved*/
> > > +#define OTP_CHARGE_PUMP_READY   BIT(3)
> > > +#define OTP_IDLE                BIT(2)
> > > +#define OTP_MEM_IDLE            BIT(1)
> > > +#define OTP_COMPARE_STATUS      BIT(0)
> > > +
> > > +/* QSR */
> > > +#define QSR_RSA_MASK           (0x3 << 12)
> > > +#define QSR_HASH_MASK          (0x3 << 10)
> > > +
> > >   static uint64_t aspeed_sbc_read(void *opaque, hwaddr addr, unsigned int 
> > > size)
> > >   {
> > >       AspeedSBCState *s = ASPEED_SBC(opaque);
> > > @@ -80,8 +102,17 @@ static void aspeed_sbc_reset(DeviceState *dev)
> > >       memset(s->regs, 0, sizeof(s->regs));
> > >       /* Set secure boot enabled with RSA4096_SHA256 and enable eMMC ABR 
> > > */
> > > -    s->regs[R_STATUS] = 0x000044C6;
> > > -    s->regs[R_QSR] = 0x07C07C89;
> > > +    s->regs[R_STATUS] = OTP_IDLE | OTP_MEM_IDLE;
> > > +
> > > +    if (s->emmc_abr) {
> > > +        s->regs[R_STATUS] &= ABR_EN;
> > > +    }
> > > +
> > > +    if (s->signing_settings) {
> > > +        s->regs[R_STATUS] &= SECURE_BOOT_EN;
> > > +    }
> > > +
> > > +    s->regs[R_QSR] = s->signing_settings;
> > >   }
> > >   static void aspeed_sbc_realize(DeviceState *dev, Error **errp)
> > > @@ -105,6 +136,11 @@ static const VMStateDescription vmstate_aspeed_sbc = 
> > > {
> > >       }
> > >   };
> > > +static Property aspeed_sbc_properties[] = {
> > > +    DEFINE_PROP_BOOL("emmc-abr", AspeedSBCState, emmc_abr, 0),
> > > +    DEFINE_PROP_UINT32("signing-settings", AspeedSBCState, 
> > > signing_settings, 0),
> > > +};
> > 
> > This needs a DEFINE_PROP_END_OF_LIST(), I bisected to this commit in 
> > Cedric's
> > aspeed-7.1 branch.
> 
> Ah you did also ! Sorry I should have told. The problem only showed
> on f35 using clang, and I didn't notice until I pushed the branch
> on gitlab yersterday.

Oh glad you noticed too, it's no problem.

> 
> > Reviewed-by: Peter Delevoryas <pdel@fb.com>
> > Tested-by: Peter Delevoryas <pdel@fb.com>
> 
> I will include the patch in the next PR.

That's great, thanks!

> 
> Thanks,
> 
> C.
> 
> 
> > > +
> > >   static void aspeed_sbc_class_init(ObjectClass *klass, void *data)
> > >   {
> > >       DeviceClass *dc = DEVICE_CLASS(klass);
> > > @@ -112,6 +148,7 @@ static void aspeed_sbc_class_init(ObjectClass *klass, 
> > > void *data)
> > >       dc->realize = aspeed_sbc_realize;
> > >       dc->reset = aspeed_sbc_reset;
> > >       dc->vmsd = &vmstate_aspeed_sbc;
> > > +    device_class_set_props(dc, aspeed_sbc_properties);
> > >   }
> > >   static const TypeInfo aspeed_sbc_info = {
> > > -- 
> > > 2.35.3
> > > 
> > > 
>