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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 17.08.17 at 14:35, <volodymyr_babchuk@xxxxxxxx> wrote:
> On Thu, Aug 17, 2017 at 01:45:54AM -0600, Jan Beulich wrote:
>> >>> On 16.08.17 at 23:41, <volodymyr_babchuk@xxxxxxxx> wrote:
>> > 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,

When I talk about the Xen tree, I normally mean the xen/ sub-tree
of xen.git.

> 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.

But struct xenpf_efi_guid is.

>> > 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.

Right, but that _still_ does not require you to add a new header. I'd
see such a relatively generic type go into xen.h.

> 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?

As per above - no, not really.

>> >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.

Sure, because afaik gcc keeps its extensions enabled in that mode.
Try a compiler that is _not_ gcc or C99 compatible.

>> >> >> +        ((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".

It means the document you referred to.

Jan

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