Re: [EXTERNAL] Re: [PATCH] hw/arm/sbsa-ref.c : Add a fake embedded controller

[Graeme Gregory]

On Mon, Aug 24, 2020 at 03:59:38PM +0100, Peter Maydell wrote:
> On Thu, 20 Aug 2020 at 14:32, Graeme Gregory <graeme@nuviainc.com> wrote:
> >
> > A difference between sbsa platform and the virt platform is PSCI is
> > handled by ARM-TF in the sbsa platform. This means that the PSCI code
> > there needs to communicate some of the platform power changes down
> > to the qemu code for things like shutdown/reset control.
> >
> > Space has been left to extend the EC if we find other use cases in
> > future where ARM-TF and qemu need to communicate.
> >
> > Signed-off-by: Graeme Gregory <graeme@nuviainc.com>
> > ---
> >  hw/arm/sbsa-ref.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >
> > diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
> > index f030a416fd..c8743fc1d0 100644
> > --- a/hw/arm/sbsa-ref.c
> > +++ b/hw/arm/sbsa-ref.c
> > @@ -41,6 +41,7 @@
> >  #include "hw/usb.h"
> >  #include "hw/char/pl011.h"
> >  #include "net/net.h"
> > +#include "migration/vmstate.h"
> >
> >  #define RAMLIMIT_GB 8192
> >  #define RAMLIMIT_BYTES (RAMLIMIT_GB * GiB)
> > @@ -62,6 +63,7 @@ enum {
> >      SBSA_CPUPERIPHS,
> >      SBSA_GIC_DIST,
> >      SBSA_GIC_REDIST,
> > +    SBSA_SECURE_EC,
> >      SBSA_SMMU,
> >      SBSA_UART,
> >      SBSA_RTC,
> > @@ -98,6 +100,14 @@ typedef struct {
> >  #define SBSA_MACHINE(obj) \
> >      OBJECT_CHECK(SBSAMachineState, (obj), TYPE_SBSA_MACHINE)
> >
> > +typedef struct {
> > +    SysBusDevice parent_obj;
> > +    MemoryRegion iomem;
> > +} SECUREECState;
> 
> Could you put the definition of the device in its own .c file,
> please (hw/misc/ seems like the right place). We generally
> prefer to keep the board and individual device models
> separate. (Some older code in the tree still has devices
> lurking in source files, but new stuff generally follows
> the rules.)
> 
Yes, certainly!

> > +enum sbsa_secure_ec_powerstates {
> > +    SBSA_SECURE_EC_CMD_NULL,
> > +    SBSA_SECURE_EC_CMD_POWEROFF,
> > +    SBSA_SECURE_EC_CMD_REBOOT,
> 
> The last two are clear enough, but what's a null power state for?
> 
My personal dislike of sending 0 to hardware as its harder to spot
when using scopes/logic analyzers. I can remove it if you wish and
explicitly number the states.

> > +static uint64_t secure_ec_read(void *opaque, hwaddr offset, unsigned size)
> > +{
> > +    /* No use for this currently */
> > +    return 0;
> > +}
> > +
> > +static void secure_ec_write(void *opaque, hwaddr offset,
> > +                     uint64_t value, unsigned size)
> > +{
> > +    if (offset == 0) { /* PSCI machine power command register */
> > +        switch (value) {
> > +        case SBSA_SECURE_EC_CMD_NULL:
> > +            break;
> > +        case SBSA_SECURE_EC_CMD_POWEROFF:
> > +            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> > +            break;
> > +        case SBSA_SECURE_EC_CMD_REBOOT:
> > +            qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
> > +            break;
> > +        default:
> > +            error_report("sbsa-ref: ERROR Unkown power command");
> 
> "unknown".
> 
> We prefer qemu_log_mask(LOG_GUEST_ERROR, ...) for logging this
> kind of "guest code did something wrong" situation.
> (you could also do that for attempting to read this w/o register
> if you liked).
> 
Excellent that is much better, Ill make that change.

> > +        }
> > +    } else {
> > +        error_report("sbsa-ref: unknown EC register");
> 
> similarly here
> 
> > +    }
> > +}
> > +
> > +static const MemoryRegionOps secure_ec_ops = {
> > +    .read = secure_ec_read,
> > +    .write = secure_ec_write,
> > +    .endianness = DEVICE_NATIVE_ENDIAN,
> 
> Consider explicitly specifying the permitted access size
> by setting .valid.min_access_size and .valid.max_access_size
> (restricting guests to only doing 32-bit writes will make
> life easier when you come to add registers later...)
> 
That was my original intent so i will do this.

> > +};
> > +
> > +static void secure_ec_init(Object *obj)
> > +{
> > +    SECUREECState *s = SECURE_EC(obj);
> > +    SysBusDevice *dev = SYS_BUS_DEVICE(obj);
> > +
> > +    memory_region_init_io(&s->iomem, obj, &secure_ec_ops, s, "secure-ec",
> > +                            0x1000);
> 
> Minor indent error here.
> 
Will fix, just FYI checkpatch does not pick this up currently.

> > +    sysbus_init_mmio(dev, &s->iomem);
> > +}
> > +
> > +static void create_secure_ec(MemoryRegion *mem)
> > +{
> > +    hwaddr base = sbsa_ref_memmap[SBSA_SECURE_EC].base;
> > +    DeviceState *dev = qdev_create(NULL, TYPE_SECURE_EC);
> > +    SysBusDevice *s = SYS_BUS_DEVICE(dev);
> > +
> > +    memory_region_add_subregion(mem, base,
> > +                                sysbus_mmio_get_region(s, 0));
> > +}
> > +
> >  static void sbsa_ref_init(MachineState *machine)
> >  {
> >      unsigned int smp_cpus = machine->smp.cpus;
> > @@ -708,6 +778,8 @@ static void sbsa_ref_init(MachineState *machine)
> >
> >      create_pcie(sms);
> >
> > +    create_secure_ec(secure_sysmem);
> > +
> >      sms->bootinfo.ram_size = machine->ram_size;
> >      sms->bootinfo.nb_cpus = smp_cpus;
> >      sms->bootinfo.board_id = -1;
> > @@ -798,8 +870,31 @@ static const TypeInfo sbsa_ref_info = {
> >      .instance_size = sizeof(SBSAMachineState),
> >  };
> >
> > +static const VMStateDescription vmstate_secure_ec_info = {
> > +    .name = "sbsa-secure-ec",
> > +    .version_id = 0,
> > +    .minimum_version_id = 0,
> > +};
> 
> If you don't have any internal state in a device (as in
> this case), then you don't need to set dc->vmsd at all.
> Just have a comment in the class init saying
>     /* No vmstate or reset required: device has no internal state */
> 
Thanks, that is cleaner, Ill do this.

Thanks for the review, Ill get right on v2.

Graeme