On Fri, 21 Jun 2024 at 14:01, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> Hi Vincent,
>
> [...]
>
> > > > $ uuid -d 935FE837-FAC8-4394-C008-737D8852C60D
> > > > encode: STR: 935fe837-fac8-4394-c008-737d8852c60d
> > > > SIV: 195894493536133784175416063449172723213
> > > > decode: variant: reserved (Microsoft GUID)
> > > > version: 4 (random data based)
> > > > content: 93:5F:E8:37:FA:C8:03:94:00:08:73:7D:88:52:C6:0D
> > > > (no semantics: random data only)
> > > >
> > > > A reserved Microsoft GUID variant does not look right.
> > >
> > > This seems like an existing bug. RFC4122 defines the MS reserved GUIDs
> > > in the variant as
> > > 1 1 0 Reserved, Microsoft Corporation backward
> > > compatibility and the existing UUID_VARIANT_MASK is defined as 0xc0...
> >
> > I think the variant mask 0xc0 is correct:
> >
> > - The variant field is in the top three bits of the "clock seq high and
> > reserved" byte, but...
> > - The variant we want is 1 0 x (do not care for bit 5, a.k.a. "Msb2").
> >
> > With the mask 0xc0 we can clear the top two bits as we set the top most bit
> > just
> > after anyway.
> >
> > ...the mask needs to be used correctly, though; see below.
>
> Ah yes, the current code is using it in clrsetbits_8, which inverts it
> internally, so it's indeed correct.
>
> >
> > >
> > > The patch below should work for you (on top of Calebs')
> > >
> > > diff --git a/include/uuid.h b/include/uuid.h
> > > index b38b20d957ef..78ed5839d2d6 100644
> > > --- a/include/uuid.h
> > > +++ b/include/uuid.h
> > > @@ -81,7 +81,7 @@ struct uuid {
> > > #define UUID_VERSION_SHIFT 12
> > > #define UUID_VERSION 0x4
> > >
> > > -#define UUID_VARIANT_MASK 0xc0
> > > +#define UUID_VARIANT_MASK 0xb0
> > > #define UUID_VARIANT_SHIFT 7
> > > #define UUID_VARIANT 0x1
> > >
> > > diff --git a/lib/uuid.c b/lib/uuid.c
> > > index 89911b06ccc0..73251eaa397e 100644
> > > --- a/lib/uuid.c
> > > +++ b/lib/uuid.c
> > > @@ -411,9 +411,9 @@ void gen_uuid_v5(const struct uuid *namespace,
> > > struct uuid *uuid, ...)
> > > memcpy(uuid, hash, sizeof(*uuid));
> > >
> > > /* Configure variant/version bits */
> > > - tmp = be32_to_cpu(uuid->time_hi_and_version);
> > > + tmp = uuid->time_hi_and_version;
> >
> > Not caring for the endianness at all does not look right.
> > Indeed, while this does work on my side in little-endian, this does not
> > work on
> > on big-endian simulators.
>
> Yes, we need the conversions
>
> > Also, this is a 16b quantity, not 32b; we need to redefine tmp to uint16_t
> > and
> > use the be16 functions.
> >
>
> Yep I already pointed that out earlier.
>
> > > tmp = (tmp & ~UUID_VERSION_MASK) | (5 << UUID_VERSION_SHIFT);
> > > - uuid->time_hi_and_version = cpu_to_be32(tmp);
> > > + uuid->time_hi_and_version = tmp;
> > >
> > > uuid->clock_seq_hi_and_reserved &= UUID_VARIANT_MASK;
> >
> > We need to mask with ~UUID_VARIANT_MASK, I think.
> >
> > > uuid->clock_seq_hi_and_reserved |= UUID_VARIANT <<
> > > UUID_VARIANT_SHIFT;
> > >
> > > Can you give it a shot?
> >
> > This does indeed work on my little-endian machines, but not on big-endian
> > simulators.
> > For testing on big-endian, I suggest using only genguid as the sandbox will
> > not
> > help there:
> >
> > $ make sandbox_defconfig
> > $ make tools-only
> > $ ./tools/genguid 2a5aa852-b856-4d97-baa9-5c5f4421551f \
> > -c "qcom,qrb4210-rb2" \
> > QUALCOMM-UBOOT
> >
> > ...and feed the resulting UUID to `uuid -d'.
> > (The genguid command is the online help example.)
> >
> > > What I am afraid of is breaking existing use cases using a different
> > > variant mask....
> > > If that's the case we might need to keep the buggy existing
> > > UUID_VARIANT_MASK and use the new one only on v5 and newer code
> >
> > I tried to debug further and I suspect that:
> >
> > - Operations on 8b clock_seq_hi_and_reserved might need further casts.
> >
> > - My understanding is that we are generating the v5 UUID as big-endian in
> > memory; if this is indeed the case, genguid should not print it with the
> > GUID
> > byte order.
>
> RFC4122 says that
> "put name space ID in network byte order so it hashes the same no
> matter what endian machine we're on "
> the EFI spec says
> "It should also be noted that TimeLow, TimeMid, TimeHighAndVersion
> fields in the EFI are encoded as little endian. The following table
> defines the format of an EFI GUID (128 bits)."
>
> Which is lovely....
>
But this brings up something that Heinrich also mentioned. Since the
efi spec and RFC4122 interpret the endianess differently, how do you
expect uuid -d to work?
Thanks
/Ilias
> I'll send a patch with the changes
>
> Regards
> /Ilias
> >
> > For the moment I am unable to make the code work in all the following cases:
> >
> > - genguid on little-endian
> > - genguid on big-endian
> > - sandbox ESRT on little-endian
> >
> > I will let you and Caleb know if I make any progress.
> >
> > Best regards,
> > Vincent.
> >
> > >
> > > Thanks
> > > /Ilias
