Re: [Xen-devel] [PATCH 4/7] arm: smccc: handle SMCs according to SMCCC

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

Hi Jan,

On Thu, Aug 17, 2017 at 01:45:54AM -0600, Jan Beulich wrote:
> >>> On 16.08.17 at 23:41, <volodymyr_babchuk@xxxxxxxx> wrote:
> > Hello Jan,
> > 
> > On Wed, Aug 09, 2017 at 05:58:19AM -0600, Jan Beulich wrote:
> > 
> >> 
> >> > On 08/08/17 21:08, Volodymyr Babchuk wrote:
> >> >> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
> >> >> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
> >> >> +
> >> >> +typedef struct {
> >> >> +    uint32_t a[4];
> >> >> +} xen_arm_smccc_uid;
> >> 
> >> This is not the normal way of encoding a UID type.
> > I thought about this. According to RFC 4122, UUID should be defined like 
> > this:
> > struct xen_uuid_rfc_4122 {
> >     u32     time_low;                  /* low part of timestamp */
> >     u16     time_mid;                  /* mid part of timestamp */
> >     u16     time_hi_and_version;       /* high part of timestamp and 
> > version */
> >     u8      clock_seq_hi_and_reserved; /* clock seq hi and variant */
> >     u8      clock_seq_low;             /* clock seq low */
> >     u8      node[6];                   /* nodes */
> > };
> > 
> > This resembles structure of RFC, but it is highly inconvenient to use. The 
> > most
> > used operation for UUIDs is comparison, I think. Next popular operations are
> > serialization and deserialization. All those are very trivial, if you are 
> > using
> > array instead of separate fields. I just checked Linux kernel, it uses array
> > of 16 u8s. I used array of four u32s because this is how it is represented 
> > in
> > SMC convention.
> 
> In our tree, see e.g. struct xenpf_efi_guid and EFI_GUID plus the
> "conversion" function between them (cast_guid()).
There are two EFI_GUID definitions in xen tree. One is in tools/libxc/xc_efi.h,
where guid defined in linux style:
typedef struct {
        uint8_t b[16];
} efi_guid_t;

Another is in xen/include/efi/efidef.h, and it is defined in Microsoft style:
typedef struct {          
    UINT32  Data1;
    UINT16  Data2;
    UINT16  Data3;
    UINT8   Data4[8]; 
} EFI_GUID;  

I assume, that you meant second one.
Anyways, both of them are for EFI code and are not publicly exported.

> > Now I'm going to create separate public header for UUIDs.
> 
> I don't see the need for a separate header.
Look, I was asked to provide XEN SMCCC UUID in public headers. To do that,
I need some structure to hold UUID. There are two ways: I can either
introduce public struct xen_uuid, which later can be used by anyone.
In this case it should have some generic structure (RFC4122 compatible,
Linux compatible, Microsoft compatible, or some XEN way). In another words,
everyone should be happy with it.

Either it can be SMCCC-only structure. Then it can live in public SMCCC header,
it can have definition that is suitable for SMCCC use, and I'm not obligued
to make it compatible with any other UUID definition standard. According to
SMCCC, UUID is presented as four 32-bit fields. This is exactly what I did
there.

So, if you are saying, that there are no need for a separate header, then I
can use the second approach, right?

> >And I'm not sure
> > that RFC 4122 approach is the best. Serialization code for that structure
> > will require some fiddling with binary shifts. Personally I stick to the
> > Linux way (uint8_t data[16]).
> > So, I'm interested in maintainers opinion.
> > 
> >> >> +#define XEN_ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)     
> >> >>  \
> >> >> +    ((xen_arm_smccc_uid) {{(a), ((b) << 16 | (c) ),                    
> >> >>  \
> >> 
> >> This is not C89 compatible.
> > 
> > I'm sorry, but I not quite sure why this is not C89 compatible. According 
> > to [1]
> > C89 supports initializer lists.
> 
> It's the (<type>){<initializers>} style which C89 doesn't support
> afaik, and ...
I wrote this small test program:

#include <stdio.h>

struct test
{
        int a[2];
};

int main (int argc, char* argv[])
{
        printf("%d\n", ((struct test){{1,2}}).a[0]);
        return 0;
}

It is compiles with gcc --std=c89 without warnings.

> >> >> +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),             
> >> >>  \
> >> >> +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
> >> >> +
> > 
> > [1] http://port70.net/~nsz/c/c89/c89-draft.html#3.5.7 
> 
> ... this also doesn't have anything like that.
I'm sorry, I don't get what do you mean under "this".

> Jan
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel