On Wed, Feb 12, 2020 at 11:48 PM Philippe Mathieu-Daudé <phi...@redhat.com>
wrote:
> )-.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.
>
Yeah sure, I can add a warning for it.
>
> >> 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)?
>
If its allright with you, I would prefer not to display a warning for the
default behavior.
The other boards are also not displaying such warnings and it keeps the
qemu output clean.
>
> > + }
> >
> > 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);
>
Thanks Philippe, I'll use this function to set s->rdkey.
Regards,
Niek
>
> >>> > > > > + }
> >>> > > > > + }
> >>> > > > > + 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;
> >>> > > > > + }
> >>> > > > > +}
> > > > > > +}
> [...]
>
>
--
Niek Linnenbank
