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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

On 09.08.17 14:58, Jan Beulich wrote:
> > > > On 09.08.17 at 12:10, <julien.grall@xxxxxxx> wrote:
> > CC "THE REST" maintainers to get an opinion on the public headers.
> Please be more specific as to what you expect - the only public
> header affected here is ARM-specific.
>
> > 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.
Just to be clear: you are proposing to store UID in such struct
struct uuid_t {
    unsigned32  time_low;
    unsigned16  time_mid;
    unsigned16  time_hi_and_version;
    unsigned8   clock_seq_hi_and_reserved;
    unsigned8   clock_seq_low;
    byte        node[6];
};
right?


> > > +#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.
Oops, sorry. Didn't knew that XEN should be C89 compatible.
Is there any guide for novices? I didn't found anything useful in docs/ 
(not even coding style document). On wiki I have found only 
"Submitting_Xen_Project_Patches" page, which is very helpful, but it 
does not cover topics like which C standard to use.
> > > +        ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0),              \
> > > +        ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
> > > +
> > > +/*
> > > + * Hypervisor Service version.
> > > + *
> > > + * We can't use XEN version here, because of SMCCC requirements:
> > > + * Major revision should change every time SMC/HVC function is removed.
> > > + * Minor revision should change every time SMC/HVC function is added.
> > > + * So, it is SMCCC protocol revision code, not XEN version.
> I don't understand this explanation - how is the situation here
> different from some arbitrary, non-toolstack-only hypercall?
Because this is generic interface that should be supported by all 
hypervisors, including XEN. Think about this as a way for a guest to 
determine under which hypervisor it operates, and which functions it 
provides.
> > > + * Those values are subjected to change, when interface will be extended.
> > > + * They should not be stored in public/asm-arm/smc.h because they should
> > > + * be queried by guest using SMC/HVC interface.
> > > + */
> > > +#define XEN_SMCCC_MAJOR_REVISION 0
> > > +#define XEN_SMCCC_MINOR_REVISION 1
> The comment says the values shouldn't be put here, but then
> they're still being put here?
Yeah, sorry. Missed this.

> > > +/* Hypervisor Service UID. Randomly generated with 3rd party tool.  */
> > > +#define XEN_SMCCC_UID XEN_ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
> > > +                                        0x9a, 0xcf, 0x79, 0xd1, \
> > > +                                        0x8d, 0xde, 0xe6, 0x67)
> Why is the right side using _ARM_ in its name, but the macro being
> defined isn't?
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel