)-.On Wed, Feb 12, 2020 at 10:31 PM Niek Linnenbank <nieklinnenb...@gmail.com> wrote: > > Hi Corey, > > On Thu, Feb 6, 2020 at 10:09 PM Niek Linnenbank <nieklinnenb...@gmail.com> > wrote: >> >> Hi Corey, >> >> On Mon, Feb 3, 2020 at 2:10 PM Corey Minyard <cminy...@mvista.com> 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 <cminy...@mvista.com> 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
Suggestion while reading this, you might display a warning if the user provided UUID doesn't start with 8100c002. >> 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"); Similarly, display a warning "No UUID provided, using default one..." (or generate one 8100c002-XXX)? > + } > > 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. > [...] >>> > > > > --- /dev/null >>> > > > > +++ b/hw/misc/allwinner-sid.c >>> > > > > @@ -0,0 +1,170 @@ >>> > > > > +/* >>> > > > > + * Allwinner Security ID emulation >>> > > > > + * >>> > > > > + * Copyright (C) 2019 Niek Linnenbank <nieklinnenb...@gmail.com> >>> > > > > + * >>> > > > > + * 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); Maybe you want: s->rdkey = ldl_le_p(s->identifier.data); >>> > > > > + } >>> > > > > + } >>> > > > > + 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; >>> > > > > + } >>> > > > > +} > > > > > +} [...]