Hi Corey, On Thu, Feb 6, 2020 at 10:09 PM Niek Linnenbank wrote: > Hi Corey, > > On Mon, Feb 3, 2020 at 2:10 PM Corey Minyard wrote: > >> On Sun, Feb 02, 2020 at 10:27:49PM +0100, Niek Linnenbank wrote: >> > Hi Corey, >> > >> > Thanks for reviewing! >> > >> > On Mon, Jan 20, 2020 at 6:59 PM Corey Minyard >> wrote: >> > >> > > On Sat, Jan 18, 2020 at 04:25:08PM +0100, Philippe Mathieu-Daudé >> wrote: >> > > > Cc'ing Corey/David for good advices about using UUID. >> > > >> > > Is there any reason you didn't use the built-in qemu UUID for this? >> It >> > > would simplify things in general. >> > > >> > >> > Currently the Allwinner SID device is using the QemuUUID type from >> > include/qemu/uuid.h. >> > Is that the build-in UUID you are referring to or should I use something >> > else? >> >> You are using the QemuUUID type, which is of course what you should do, >> but you aren't using the UUID generated by qemu (at least that I can >> find). >> That in include/sysemu/sysemu.h and is named qemu_uuid. Whether you >> should use that or not depends on your needs. If you just need some >> uuid, then that's what you should probably use. If you need something >> the user can individually control for this device, for instance, then >> that probably won't do. >> > > Ah I did not know about the qemu_uuid variable, thanks for pointing that > out. > Basically the SID identifier is a number that is unique for each board > that comes > out of the factory. It is currently used by U-Boot to as input to generate > a MAC address. > > If I understand correctly, qemu_uuid is optional and by default zero. > However the SID value should not be zero, as otherwise U-Boot can't pick a > MAC address > resulting in a non-working ethernet device. > > Currently the hw/arm/orangepi.c machine specifies a fixed SID to be used > for the emulated board, > also containing a prefix (8100c002) that indicates the H3 chipset. One > thing that I am strugling with is that > I'm not able to override the property using '-global', if > hw/arm/orangepi.c initializes the property with qdev_prop_set_string: > > $ qemu-system-arm -M orangepi-pc -kernel u-boot -nographic -nic user \ > -global allwinner-sid.identifier=8100c002-0001-0002-0003-000044556688 > > If I don't set the property in hw/arm/orangepi.c, I can set it with > '-global'. Do you perhaps have a > recommendation how to improve that? Basically what is needed is that the > machine sets the default > property including the chip prefix, and that the user can override it. > Although it is not required for a > working emulated board, it would be a nice-to-have that the user can set > it. > FYI and possibly others who have a similar usecase, I figured out how to do this. In the machine init function, after creating the new SoC object, simply check if the identifier has a value: + if (qemu_uuid_is_null(&s->h3->sid.identifier)) { + qdev_prop_set_string(DEVICE(s->h3), "identifier", + "8100c002-0001-0002-0003-000044556677"); + } That way, if the user passed -global to override it, the machine will not overrule the user's value and by default the machine sets an identifier containing the H3 specific chip prefix. Regards, Niek > >> > >> > >> > > Also, in case no one else say, you have tabs in your code that you >> need >> > > to get rid of. >> > > >> > > >> > If there are any tabs in the code, it was not intended. I re-checked >> this >> > patch and others >> > again but found no tabs in the code. >> > Could you please point out where you found the extra tabs? >> >> My apologies, I saw 1-character misalignments, and that usually means >> that there's a tab. But it looks like it has something to do with the >> way it was forwarded. I didn't get the original email. >> >> Ah yes that is possible indeed. I'll add you to the CC list for the next > version of this patch series. > > Regards, > Niek > > >> -corey >> >> > >> > Regards, >> > Niek >> > >> > >> > > -corey >> > > >> > > > >> > > > On 1/8/20 9:00 PM, Niek Linnenbank wrote: >> > > > > The Security Identifier device found in various Allwinner System >> on >> > > Chip >> > > > > designs gives applications a per-board unique identifier. This >> commit >> > > > > adds support for the Allwinner Security Identifier using a 128-bit >> > > > > UUID value as input. >> > > > > >> > > > > Signed-off-by: Niek Linnenbank >> > > > > --- >> > > > > include/hw/arm/allwinner-h3.h | 3 + >> > > > > include/hw/misc/allwinner-sid.h | 61 ++++++++++++ >> > > > > hw/arm/allwinner-h3.c | 11 ++- >> > > > > hw/arm/orangepi.c | 4 + >> > > > > hw/misc/allwinner-sid.c | 170 >> > > ++++++++++++++++++++++++++++++++ >> > > > > hw/misc/Makefile.objs | 1 + >> > > > > hw/misc/trace-events | 4 + >> > > > > 7 files changed, 253 insertions(+), 1 deletion(-) >> > > > > create mode 100644 include/hw/misc/allwinner-sid.h >> > > > > create mode 100644 hw/misc/allwinner-sid.c >> > > > > >> > > > > diff --git a/include/hw/arm/allwinner-h3.h >> > > b/include/hw/arm/allwinner-h3.h >> > > > > index 5a25a92eae..9ed365507c 100644 >> > > > > --- a/include/hw/arm/allwinner-h3.h >> > > > > +++ b/include/hw/arm/allwinner-h3.h >> > > > > @@ -46,6 +46,7 @@ >> > > > > #include "hw/misc/allwinner-h3-ccu.h" >> > > > > #include "hw/misc/allwinner-cpucfg.h" >> > > > > #include "hw/misc/allwinner-h3-sysctrl.h" >> > > > > +#include "hw/misc/allwinner-sid.h" >> > > > > #include "target/arm/cpu.h" >> > > > > /** >> > > > > @@ -63,6 +64,7 @@ enum { >> > > > > AW_H3_SRAM_A2, >> > > > > AW_H3_SRAM_C, >> > > > > AW_H3_SYSCTRL, >> > > > > + AW_H3_SID, >> > > > > AW_H3_EHCI0, >> > > > > AW_H3_OHCI0, >> > > > > AW_H3_EHCI1, >> > > > > @@ -115,6 +117,7 @@ typedef struct AwH3State { >> > > > > AwH3ClockCtlState ccu; >> > > > > AwCpuCfgState cpucfg; >> > > > > AwH3SysCtrlState sysctrl; >> > > > > + AwSidState sid; >> > > > > GICState gic; >> > > > > MemoryRegion sram_a1; >> > > > > MemoryRegion sram_a2; >> > > > > diff --git a/include/hw/misc/allwinner-sid.h >> > > b/include/hw/misc/allwinner-sid.h >> > > > > new file mode 100644 >> > > > > index 0000000000..41189967e2 >> > > > > --- /dev/null >> > > > > +++ b/include/hw/misc/allwinner-sid.h >> > > > > @@ -0,0 +1,61 @@ >> > > > > +/* >> > > > > + * Allwinner Security ID emulation >> > > > > + * >> > > > > + * Copyright (C) 2019 Niek Linnenbank >> > > > > + * >> > > > > + * This program is free software: you can redistribute it and/or >> > > modify >> > > > > + * it under the terms of the GNU General Public License as >> published >> > > by >> > > > > + * the Free Software Foundation, either version 2 of the >> License, or >> > > > > + * (at your option) any later version. >> > > > > + * >> > > > > + * This program is distributed in the hope that it will be >> useful, >> > > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > > > > + * GNU General Public License for more details. >> > > > > + * >> > > > > + * You should have received a copy of the GNU General Public >> License >> > > > > + * along with this program. If not, see < >> > > http://www.gnu.org/licenses/>. >> > > > > + */ >> > > > > + >> > > > > +#ifndef HW_MISC_ALLWINNER_SID_H >> > > > > +#define HW_MISC_ALLWINNER_SID_H >> > > > > + >> > > > > +#include "qemu/osdep.h" >> > > > > +#include "qom/object.h" >> > > > > +#include "hw/sysbus.h" >> > > > > +#include "qemu/uuid.h" >> > > > > + >> > > > > +/** >> > > > > + * Object model >> > > > > + * @{ >> > > > > + */ >> > > > > + >> > > > > +#define TYPE_AW_SID "allwinner-sid" >> > > > > +#define AW_SID(obj) \ >> > > > > + OBJECT_CHECK(AwSidState, (obj), TYPE_AW_SID) >> > > > > + >> > > > > +/** @} */ >> > > > > + >> > > > > +/** >> > > > > + * Allwinner Security ID object instance state >> > > > > + */ >> > > > > +typedef struct AwSidState { >> > > > > + /*< private >*/ >> > > > > + SysBusDevice parent_obj; >> > > > > + /*< public >*/ >> > > > > + >> > > > > + /** Maps I/O registers in physical memory */ >> > > > > + MemoryRegion iomem; >> > > > > + >> > > > > + /** Control register defines how and what to read */ >> > > > > + uint32_t control; >> > > > > + >> > > > > + /** RdKey register contains the data retrieved by the device >> */ >> > > > > + uint32_t rdkey; >> > > > > + >> > > > > + /** Stores the emulated device identifier */ >> > > > > + QemuUUID identifier; >> > > > > + >> > > > > +} AwSidState; >> > > > > + >> > > > > +#endif /* HW_MISC_ALLWINNER_SID_H */ >> > > > > diff --git a/hw/arm/allwinner-h3.c b/hw/arm/allwinner-h3.c >> > > > > index e9ad6d23df..af7317e86a 100644 >> > > > > --- a/hw/arm/allwinner-h3.c >> > > > > +++ b/hw/arm/allwinner-h3.c >> > > > > @@ -36,6 +36,7 @@ const hwaddr allwinner_h3_memmap[] = { >> > > > > [AW_H3_SRAM_A2] = 0x00044000, >> > > > > [AW_H3_SRAM_C] = 0x00010000, >> > > > > [AW_H3_SYSCTRL] = 0x01c00000, >> > > > > + [AW_H3_SID] = 0x01c14000, >> > > > > [AW_H3_EHCI0] = 0x01c1a000, >> > > > > [AW_H3_OHCI0] = 0x01c1a400, >> > > > > [AW_H3_EHCI1] = 0x01c1b000, >> > > > > @@ -76,7 +77,6 @@ struct AwH3Unimplemented { >> > > > > { "mmc0", 0x01c0f000, 4 * KiB }, >> > > > > { "mmc1", 0x01c10000, 4 * KiB }, >> > > > > { "mmc2", 0x01c11000, 4 * KiB }, >> > > > > - { "sid", 0x01c14000, 1 * KiB }, >> > > > > { "crypto", 0x01c15000, 4 * KiB }, >> > > > > { "msgbox", 0x01c17000, 4 * KiB }, >> > > > > { "spinlock", 0x01c18000, 4 * KiB }, >> > > > > @@ -196,6 +196,11 @@ static void allwinner_h3_init(Object *obj) >> > > > > sysbus_init_child_obj(obj, "cpucfg", &s->cpucfg, >> > > sizeof(s->cpucfg), >> > > > > TYPE_AW_CPUCFG); >> > > > > + >> > > > > + sysbus_init_child_obj(obj, "sid", &s->sid, sizeof(s->sid), >> > > > > + TYPE_AW_SID); >> > > > > + object_property_add_alias(obj, "identifier", OBJECT(&s->sid), >> > > > > + "identifier", &error_abort); >> > > > > } >> > > > > static void allwinner_h3_realize(DeviceState *dev, Error **errp) >> > > > > @@ -316,6 +321,10 @@ static void allwinner_h3_realize(DeviceState >> > > *dev, Error **errp) >> > > > > qdev_init_nofail(DEVICE(&s->cpucfg)); >> > > > > sysbus_mmio_map(SYS_BUS_DEVICE(&s->cpucfg), 0, >> > > s->memmap[AW_H3_CPUCFG]); >> > > > > + /* Security Identifier */ >> > > > > + qdev_init_nofail(DEVICE(&s->sid)); >> > > > > + sysbus_mmio_map(SYS_BUS_DEVICE(&s->sid), 0, >> s->memmap[AW_H3_SID]); >> > > > > + >> > > > > /* Universal Serial Bus */ >> > > > > sysbus_create_simple(TYPE_AW_H3_EHCI, >> s->memmap[AW_H3_EHCI0], >> > > > > qdev_get_gpio_in(DEVICE(&s->gic), >> > > > > diff --git a/hw/arm/orangepi.c b/hw/arm/orangepi.c >> > > > > index 051184f14f..a7f870c88b 100644 >> > > > > --- a/hw/arm/orangepi.c >> > > > > +++ b/hw/arm/orangepi.c >> > > > > @@ -54,6 +54,10 @@ static void orangepi_init(MachineState >> *machine) >> > > > > object_property_set_int(OBJECT(s->h3), 24000000, >> "clk1-freq", >> > > > > &error_abort); >> > > > > + /* Setup SID properties */ >> > > > > + qdev_prop_set_string(DEVICE(s->h3), "identifier", >> > > > > + "8100c002-0001-0002-0003-000044556677"); >> > > > > + >> > > > > /* Mark H3 object realized */ >> > > > > object_property_set_bool(OBJECT(s->h3), true, "realized", >> > > &error_abort); >> > > > > diff --git a/hw/misc/allwinner-sid.c b/hw/misc/allwinner-sid.c >> > > > > new file mode 100644 >> > > > > index 0000000000..954de935bc >> > > > > --- /dev/null >> > > > > +++ b/hw/misc/allwinner-sid.c >> > > > > @@ -0,0 +1,170 @@ >> > > > > +/* >> > > > > + * Allwinner Security ID emulation >> > > > > + * >> > > > > + * Copyright (C) 2019 Niek Linnenbank >> > > > > + * >> > > > > + * This program is free software: you can redistribute it and/or >> > > modify >> > > > > + * it under the terms of the GNU General Public License as >> published >> > > by >> > > > > + * the Free Software Foundation, either version 2 of the >> License, or >> > > > > + * (at your option) any later version. >> > > > > + * >> > > > > + * This program is distributed in the hope that it will be >> useful, >> > > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> > > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> > > > > + * GNU General Public License for more details. >> > > > > + * >> > > > > + * You should have received a copy of the GNU General Public >> License >> > > > > + * along with this program. If not, see < >> > > http://www.gnu.org/licenses/>. >> > > > > + */ >> > > > > + >> > > > > +#include "qemu/osdep.h" >> > > > > +#include "qemu/units.h" >> > > > > +#include "hw/sysbus.h" >> > > > > +#include "migration/vmstate.h" >> > > > > +#include "qemu/log.h" >> > > > > +#include "qemu/module.h" >> > > > > +#include "qemu/guest-random.h" >> > > > > +#include "qapi/error.h" >> > > > > +#include "hw/qdev-properties.h" >> > > > > +#include "hw/misc/allwinner-sid.h" >> > > > > +#include "trace.h" >> > > > > + >> > > > > +/* SID register offsets */ >> > > > > +enum { >> > > > > + REG_PRCTL = 0x40, /* Control */ >> > > > > + REG_RDKEY = 0x60, /* Read Key */ >> > > > > +}; >> > > > > + >> > > > > +/* SID register flags */ >> > > > > +enum { >> > > > > + REG_PRCTL_WRITE = 0x0002, /* Unknown write flag */ >> > > > > + REG_PRCTL_OP_LOCK = 0xAC00, /* Lock operation */ >> > > > > +}; >> > > > > + >> > > > > +static uint64_t allwinner_sid_read(void *opaque, hwaddr offset, >> > > > > + unsigned size) >> > > > > +{ >> > > > > + const AwSidState *s = AW_SID(opaque); >> > > > > + uint64_t val = 0; >> > > > > + >> > > > > + switch (offset) { >> > > > > + case REG_PRCTL: /* Control */ >> > > > > + val = s->control; >> > > > > + break; >> > > > > + case REG_RDKEY: /* Read Key */ >> > > > > + val = s->rdkey; >> > > > > + break; >> > > > > + default: >> > > > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds offset >> > > 0x%04x\n", >> > > > > + __func__, (uint32_t)offset); >> > > > > + return 0; >> > > > > + } >> > > > > + >> > > > > + trace_allwinner_sid_read(offset, val, size); >> > > > > + >> > > > > + return val; >> > > > > +} >> > > > > + >> > > > > +static void allwinner_sid_write(void *opaque, hwaddr offset, >> > > > > + uint64_t val, unsigned size) >> > > > > +{ >> > > > > + AwSidState *s = AW_SID(opaque); >> > > > > + >> > > > > + trace_allwinner_sid_write(offset, val, size); >> > > > > + >> > > > > + switch (offset) { >> > > > > + case REG_PRCTL: /* Control */ >> > > > > + s->control = val; >> > > > > + >> > > > > + if ((s->control & REG_PRCTL_OP_LOCK) && >> > > > > + (s->control & REG_PRCTL_WRITE)) { >> > > > > + uint32_t id = s->control >> 16; >> > > > > + >> > > > > + if (id < sizeof(QemuUUID)) { >> > > > > + s->rdkey = (s->identifier.data[id]) | >> > > > > + (s->identifier.data[id + 1] << 8) | >> > > > > + (s->identifier.data[id + 2] << 16) | >> > > > > + (s->identifier.data[id + 3] << 24); >> > > > > + } >> > > > > + } >> > > > > + s->control &= ~REG_PRCTL_WRITE; >> > > > > + break; >> > > > > + case REG_RDKEY: /* Read Key */ >> > > > > + break; >> > > > > + default: >> > > > > + qemu_log_mask(LOG_GUEST_ERROR, "%s: out-of-bounds offset >> > > 0x%04x\n", >> > > > > + __func__, (uint32_t)offset); >> > > > > + break; >> > > > > + } >> > > > > +} >> > > > > + >> > > > > +static const MemoryRegionOps allwinner_sid_ops = { >> > > > > + .read = allwinner_sid_read, >> > > > > + .write = allwinner_sid_write, >> > > > > + .endianness = DEVICE_NATIVE_ENDIAN, >> > > > > + .valid = { >> > > > > + .min_access_size = 4, >> > > > > + .max_access_size = 4, >> > > > > + }, >> > > > > + .impl.min_access_size = 4, >> > > > > +}; >> > > > > + >> > > > > +static void allwinner_sid_reset(DeviceState *dev) >> > > > > +{ >> > > > > + AwSidState *s = AW_SID(dev); >> > > > > + >> > > > > + /* Set default values for registers */ >> > > > > + s->control = 0; >> > > > > + s->rdkey = 0; >> > > > > +} >> > > > > + >> > > > > +static void allwinner_sid_init(Object *obj) >> > > > > +{ >> > > > > + SysBusDevice *sbd = SYS_BUS_DEVICE(obj); >> > > > > + AwSidState *s = AW_SID(obj); >> > > > > + >> > > > > + /* Memory mapping */ >> > > > > + memory_region_init_io(&s->iomem, OBJECT(s), >> &allwinner_sid_ops, s, >> > > > > + TYPE_AW_SID, 1 * KiB); >> > > > > + sysbus_init_mmio(sbd, &s->iomem); >> > > > > +} >> > > > > + >> > > > > +static Property allwinner_sid_properties[] = { >> > > > > + DEFINE_PROP_UUID_NODEFAULT("identifier", AwSidState, >> identifier), >> > > > > + DEFINE_PROP_END_OF_LIST() >> > > > > +}; >> > > > > + >> > > > > +static const VMStateDescription allwinner_sid_vmstate = { >> > > > > + .name = "allwinner-sid", >> > > > > + .version_id = 1, >> > > > > + .minimum_version_id = 1, >> > > > > + .fields = (VMStateField[]) { >> > > > > + VMSTATE_UINT32(control, AwSidState), >> > > > > + VMSTATE_UINT32(rdkey, AwSidState), >> > > > > + VMSTATE_END_OF_LIST() >> > > > > + } >> > > > > +}; >> > > > > + >> > > > > +static void allwinner_sid_class_init(ObjectClass *klass, void >> *data) >> > > > > +{ >> > > > > + DeviceClass *dc = DEVICE_CLASS(klass); >> > > > > + >> > > > > + dc->reset = allwinner_sid_reset; >> > > > > + dc->vmsd = &allwinner_sid_vmstate; >> > > > > + dc->props = allwinner_sid_properties; >> > > > > +} >> > > > > + >> > > > > +static const TypeInfo allwinner_sid_info = { >> > > > > + .name = TYPE_AW_SID, >> > > > > + .parent = TYPE_SYS_BUS_DEVICE, >> > > > > + .instance_init = allwinner_sid_init, >> > > > > + .instance_size = sizeof(AwSidState), >> > > > > + .class_init = allwinner_sid_class_init, >> > > > > +}; >> > > > > + >> > > > > +static void allwinner_sid_register(void) >> > > > > +{ >> > > > > + type_register_static(&allwinner_sid_info); >> > > > > +} >> > > > > + >> > > > > +type_init(allwinner_sid_register) >> > > > > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs >> > > > > index 12c2c306b5..59500d5681 100644 >> > > > > --- a/hw/misc/Makefile.objs >> > > > > +++ b/hw/misc/Makefile.objs >> > > > > @@ -31,6 +31,7 @@ common-obj-$(CONFIG_IVSHMEM_DEVICE) += ivshmem.o >> > > > > common-obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-ccu.o >> > > > > obj-$(CONFIG_ALLWINNER_H3) += allwinner-cpucfg.o >> > > > > common-obj-$(CONFIG_ALLWINNER_H3) += allwinner-h3-sysctrl.o >> > > > > +common-obj-$(CONFIG_ALLWINNER_H3) += allwinner-sid.o >> > > > > common-obj-$(CONFIG_REALVIEW) += arm_sysctl.o >> > > > > common-obj-$(CONFIG_NSERIES) += cbus.o >> > > > > common-obj-$(CONFIG_ECCMEMCTL) += eccmemctl.o >> > > > > diff --git a/hw/misc/trace-events b/hw/misc/trace-events >> > > > > index d3e0952429..67d8bf493c 100644 >> > > > > --- a/hw/misc/trace-events >> > > > > +++ b/hw/misc/trace-events >> > > > > @@ -5,6 +5,10 @@ allwinner_cpucfg_cpu_reset(uint8_t cpu_id, >> uint32_t >> > > reset_addr) "id %u, reset_ad >> > > > > allwinner_cpucfg_read(uint64_t offset, uint64_t data, unsigned >> size) >> > > "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %" PRIu32 >> > > > > allwinner_cpucfg_write(uint64_t offset, uint64_t data, unsigned >> > > size) "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %" PRIu32 >> > > > > +# allwinner-sid.c >> > > > > +allwinner_sid_read(uint64_t offset, uint64_t data, unsigned size) >> > > "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %" PRIu32 >> > > > > +allwinner_sid_write(uint64_t offset, uint64_t data, unsigned >> size) >> > > "offset 0x%" PRIx64 " data 0x%" PRIx64 " size %" PRIu32 >> > > > > + >> > > > > # eccmemctl.c >> > > > > ecc_mem_writel_mer(uint32_t val) "Write memory enable 0x%08x" >> > > > > ecc_mem_writel_mdr(uint32_t val) "Write memory delay 0x%08x" >> > > > > >> > > > >> > > >> > >> > >> > -- >> > Niek Linnenbank >> > > > -- > Niek Linnenbank > > -- Niek Linnenbank