[PATCH 0/3] aspeed: small cleanups

[PATCH 0/3] aspeed: small cleanups

[Cédric Le Goater]

Hello,

I was preparing a PR and I realized that these small patches had never
seen the light. Here they are.

Thanks,

C.

Cédric Le Goater (2):
  aspeed/scu: Add trace events for read ops
  aspeed/i2c: Change trace event for NORMAL_STOP states

Joel Stanley (1):
  aspeed: sbc: Allow per-machine settings

 include/hw/misc/aspeed_sbc.h | 13 ++++++++++++
 hw/i2c/aspeed_i2c.c          |  2 +-
 hw/misc/aspeed_sbc.c         | 41 ++++++++++++++++++++++++++++++++++--
 hw/misc/aspeed_scu.c         |  2 ++
 hw/misc/trace-events         |  1 +
 5 files changed, 56 insertions(+), 3 deletions(-)

-- 
2.35.3

[PATCH 1/3] aspeed/scu: Add trace events for read ops

[Cédric Le Goater]

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/misc/aspeed_scu.c | 2 ++
 hw/misc/trace-events | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 19b03471fc4e..83353649064a 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -270,6 +270,7 @@ static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
         break;
     }
 
+    trace_aspeed_scu_read(offset, size, s->regs[reg]);
     return s->regs[reg];
 }
 
@@ -637,6 +638,7 @@ static uint64_t aspeed_ast2600_scu_read(void *opaque, hwaddr offset,
         break;
     }
 
+    trace_aspeed_scu_read(offset, size, s->regs[reg]);
     return s->regs[reg];
 }
 
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index c5e37b01547f..f776f24fb5d1 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -69,6 +69,7 @@ slavio_led_mem_readw(uint32_t ret) "Read diagnostic LED 0x%04x"
 
 # aspeed_scu.c
 aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
+aspeed_scu_read(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
 
 # mps2-scc.c
 mps2_scc_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
-- 
2.35.3

Re: [PATCH 1/3] aspeed/scu: Add trace events for read ops

[Peter Delevoryas]

> On Jun 28, 2022, at 8:47 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Also a nice change.

Reviewed-by: Peter Delevoryas <pdel@fb.com>

> ---
> hw/misc/aspeed_scu.c | 2 ++
> hw/misc/trace-events | 1 +
> 2 files changed, 3 insertions(+)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 19b03471fc4e..83353649064a 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -270,6 +270,7 @@ static uint64_t aspeed_scu_read(void *opaque, hwaddr offset, unsigned size)
>         break;
>     }
> 
> +    trace_aspeed_scu_read(offset, size, s->regs[reg]);
>     return s->regs[reg];
> }
> 
> @@ -637,6 +638,7 @@ static uint64_t aspeed_ast2600_scu_read(void *opaque, hwaddr offset,
>         break;
>     }
> 
> +    trace_aspeed_scu_read(offset, size, s->regs[reg]);
>     return s->regs[reg];
> }
> 
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index c5e37b01547f..f776f24fb5d1 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -69,6 +69,7 @@ slavio_led_mem_readw(uint32_t ret) "Read diagnostic LED 0x%04x"
> 
> # aspeed_scu.c
> aspeed_scu_write(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
> +aspeed_scu_read(uint64_t offset, unsigned size, uint32_t data) "To 0x%" PRIx64 " of size %u: 0x%" PRIx32
> 
> # mps2-scc.c
> mps2_scc_read(uint64_t offset, uint64_t data, unsigned size) "MPS2 SCC read: offset 0x%" PRIx64 " data 0x%" PRIx64 " size %u"
> -- 
> 2.35.3
> 
> 

[PATCH 2/3] aspeed/i2c: Change trace event for NORMAL_STOP states

[Cédric Le Goater]

Using a 'stop' string seems more appropriate than 'normal'.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 hw/i2c/aspeed_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
index 37ae1f2e04bd..9b41bc38964f 100644
--- a/hw/i2c/aspeed_i2c.c
+++ b/hw/i2c/aspeed_i2c.c
@@ -58,7 +58,7 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
                ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH) ?
                                                "slave-match|" : "",
                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP) ?
-                                               "normal|" : "",
+                                               "stop|" : "",
                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ?
                                                "abnormal"  : "");
 
-- 
2.35.3

Re: [PATCH 2/3] aspeed/i2c: Change trace event for NORMAL_STOP states

[Peter Delevoryas]

> On Jun 28, 2022, at 8:47 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> Using a 'stop' string seems more appropriate than 'normal'.

Ha yes! This is an understatement, the change is much appreciated.

I've seen that trace and never realized it was referring to ‘stop’.

Reviewed-by: Peter Delevoryas <pdel@fb.com>

> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
> hw/i2c/aspeed_i2c.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/aspeed_i2c.c b/hw/i2c/aspeed_i2c.c
> index 37ae1f2e04bd..9b41bc38964f 100644
> --- a/hw/i2c/aspeed_i2c.c
> +++ b/hw/i2c/aspeed_i2c.c
> @@ -58,7 +58,7 @@ static inline void aspeed_i2c_bus_raise_interrupt(AspeedI2CBus *bus)
>                ARRAY_FIELD_EX32(bus->regs, I2CD_INTR_STS, SLAVE_ADDR_RX_MATCH) ?
>                                                "slave-match|" : "",
>                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, NORMAL_STOP) ?
> -                                               "normal|" : "",
> +                                               "stop|" : "",
>                SHARED_ARRAY_FIELD_EX32(bus->regs, reg_intr_sts, ABNORMAL) ?
>                                                "abnormal"  : "");
> 
> -- 
> 2.35.3
> 
> 

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

[Cédric Le Goater]

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),
+};
+
 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

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

[Peter Delevoryas]

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.

Reviewed-by: Peter Delevoryas <pdel@fb.com>
Tested-by: Peter Delevoryas <pdel@fb.com>

> +
>  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
> 
> 

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

[Cédric Le Goater]

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.

> Reviewed-by: Peter Delevoryas <pdel@fb.com>
> Tested-by: Peter Delevoryas <pdel@fb.com>

I will include the patch in the next PR.

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
>>
>>

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
> > > 
> > > 
>