On Wed, Feb 12, 2020 at 11:48 PM Philippe Mathieu-Daudé <
address@hidden> wrote:
)-.On Wed, Feb 12, 2020 at 10:31 PM Niek Linnenbank
<address@hidden> wrote:
>
> Hi Corey,
>
> On Thu, Feb 6, 2020 at 10:09 PM Niek Linnenbank <address@hidden> wrote:
>>
>> Hi Corey,
>>
>> On Mon, Feb 3, 2020 at 2:10 PM Corey Minyard <address@hidden> 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 <address@hidden> 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 <address@hidden>
>>> > > > > + *
>>> > > > > + * 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;
>>> > > > > + }
>>> > > > > +}
> > > > > +}
[...]