Re: [Xen-devel] [PATCH v2 5/8] tmem: Move TMEM_CONTROL subop of tmem hypercall to sysctl.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> >>> +struct xen_sysctl_tmem_op {
> >>> +    uint32_t cmd;       /* IN: XEN_SYSCTL_TMEM_OP_* . */
> >>> +    int32_t pool_id;    /* IN: 0 by default unless _SAVE_*, RESTORE_* .*/
> >>> +    uint32_t cli_id;    /* IN: client id, 0 for 
> >>> XEN_SYSCTL_TMEM_QUERY_FREEABLE_MB
> >>> +                           for all others can be the domain id or
> >>> +                           XEN_SYSCTL_TMEM_OP_ALL_CLIENTS for all. */
> >>> +    uint32_t arg1;      /* IN: If not applicable to command use 0. */
> >>> +    uint32_t arg2;      /* IN: If not applicable to command use 0. */
> >> Please can this interface be fixed as part of the move, even if it is in
> >> subsequent patches for clarity.
> > I will gladly fix this interface in further patches. By all means!
> 
> What I wish to avoid is 4.6 releasing with the API in this state, as
> adjusting valgrind and strace to compensate will be miserable.
> 
> The best solution would be to have this patch and the fixups adjacent in
> the series, at which point the valgrind/strace adjustments can start
> with the clean API for 4.6

See attached two patches. The first one makes the 'oid[3]' be a nice structure.

Then I decided to see if I can expand that to also be part of the
'tmem_op', which looked legit (as it is the same size and offset and
pahole wise it looks right).

But sadly the compat layer is not happy with me:


In file included from /home/konrad/ssd/konrad/xen/xen/include/xen/compat.h:12:0,
                 from /home/konrad/ssd/konrad/xen/xen/include/compat/xen.h:3,
                 from /home/konrad/ssd/konrad/xen/xen/include/xen/shared.h:6,
                 from /home/konrad/ssd/konrad/xen/xen/include/xen/sched.h:7,
                 from setup.c:5:
/home/konrad/ssd/konrad/xen/xen/include/xen/tmem_xen.h: In function 
âtmem_get_tmemop_from_clientâ:
/home/konrad/ssd/konrad/xen/xen/include/compat/xlat.h:935:26: error: 
incompatible types when assigning to type âxen_tmem_oid_tâ from type 
âcompat_tmem_oid_tâ
         (_d_)->u.gen.oid = (_s_)->u.gen.oid; \
                          ^
/home/konrad/ssd/konrad/xen/xen/include/xen/tmem_xen.h:306:9: note: in 
expansion of macro âXLAT_tmem_opâ
         XLAT_tmem_op(op, &cop);
         ^
make[3]: *** [setup.o] Error 1
make[3]: *** Waiting for unfinished jobs....
In file included from /home/konrad/ssd/konrad/xen/xen/include/xen/compat.h:12:0,
                 from /home/konrad/ssd/konrad/xen/xen/include/compat/xen.h:3,
                 from /home/konrad/ssd/konrad/xen/xen/include/xen/shared.h:6,
                 from /home/konrad/ssd/konrad/xen/xen/include/xen/sched.h:7,
                 from memory.c:15:
/home/konrad/ssd/konrad/xen/xen/include/xen/tmem_xen.h: In function 
âtmem_get_tmemop_from_clientâ:
/home/konrad/ssd/konrad/xen/xen/include/compat/xlat.h:935:26: error: 
incompatible types when assigning to type âxen_tmem_oid_tâ from type 
âcompat_tmem_oid_tâ
         (_d_)->u.gen.oid = (_s_)->u.gen.oid; \
                          ^
/home/konrad/ssd/konrad/xen/xen/include/xen/tmem_xen.h:306:9: note: in 
expansion of macro âXLAT_tmem_opâ
         XLAT_tmem_op(op, &cop);
         ^
make[3]: *** [memory.o] Error 1
make[3]: *** Waiting for unfinished jobs....
In file included from /home/konrad/ssd/konrad/xen/xen/include/xen/compat.h:12:0,
                 from /home/konrad/ssd/konrad/xen/xen/include/compat/xen.h:3,
                 from /home/konrad/ssd/konrad/xen/xen/include/xen/shared.h:6,
                 from /home/konrad/ssd/konrad/xen/xen/include/xen/sched.h:7,
                 from page_alloc.c:27:
/home/konrad/ssd/konrad/xen/xen/include/xen/tmem_xen.h: In function 
âtmem_get_tmemop_from_clientâ:
/home/konrad/ssd/konrad/xen/xen/include/compat/xlat.h:935:26: error: 
incompatible types when assigning to type âxen_tmem_oid_tâ from type 
âcompat_tmem_oid_tâ
         (_d_)->u.gen.oid = (_s_)->u.gen.oid; \
                          ^
/home/konrad/ssd/konrad/xen/xen/include/xen/tmem_xen.h:306:9: note: in 
expansion of macro âXLAT_tmem_opâ
         XLAT_tmem_op(op, &cop);
         ^
make[3]: *** [page_alloc.o] Error 1
make[2]: *** [/home/konrad/ssd/konrad/xen/xen/common/built_in.o] Error 2
make[2]: *** Waiting for unfinished jobs....
make[2]: *** [/home/konrad/ssd/konrad/xen/xen/arch/x86/built_in.o] Error 2
make[1]: *** [/home/konrad/ssd/konrad/xen/xen/xen] Error 2
make: *** [build] Error 2

Which is right:


struct compat_tmem_oid {
    uint64_t oid[3];
};
typedef struct compat_tmem_oid compat_tmem_oid_t;

typedef COMPAT_HANDLE(char) tmem_cli_va_compat_t;
struct compat_tmem_op {
    uint32_t cmd;
    int32_t pool_id;
    union {
        struct {
            uint64_t uuid[2];
            uint32_t flags;
            uint32_t arg1;
        } creat;
        struct {

            compat_tmem_oid_t oid;   <====== We have 'struct tmem_oid' in the 
tmem_op.

            uint32_t index;
            uint32_t tmem_offset;
            uint32_t pfn_offset;
            uint32_t len;
            compat_pfn_t cmfn;
        } gen;
    } u;
};
typedef struct compat_tmem_op tmem_op_compat_t;
DEFINE_COMPAT_HANDLE(tmem_op_compat_t);
#define XLAT_tmem_op(_d_, _s_) do { \
    (_d_)->cmd = (_s_)->cmd; \
    (_d_)->pool_id = (_s_)->pool_id; \
    switch (u) { \
    case XLAT_tmem_op_u_creat: \
        { \
            unsigned int i0; \
            for (i0 = 0; i0 <  2; ++i0) { \
                (_d_)->u.creat.uuid[i0] = (_s_)->u.creat.uuid[i0]; \
            } \
        } \
        (_d_)->u.creat.flags = (_s_)->u.creat.flags; \
        (_d_)->u.creat.arg1 = (_s_)->u.creat.arg1; \
        break; \
    case XLAT_tmem_op_u_gen: \
        (_d_)->u.gen.oid = (_s_)->u.gen.oid; \
        (_d_)->u.gen.index = (_s_)->u.gen.index; \
        (_d_)->u.gen.tmem_offset = (_s_)->u.gen.tmem_offset; \
        (_d_)->u.gen.pfn_offset = (_s_)->u.gen.pfn_offset; \
        (_d_)->u.gen.len = (_s_)->u.gen.len; \
        (_d_)->u.gen.cmfn = (_s_)->u.gen.cmfn; \
        break; \
    } \
} while (0)


static inline int tmem_get_tmemop_from_client(tmem_op_t *op, tmem_cli_op_t uops)
{
#ifdef CONFIG_COMPAT
    if ( has_hvm_container_vcpu(current) ?
         hvm_guest_x86_mode(current) != 8 :
         is_pv_32bit_vcpu(current) )
    {
        int rc;
        enum XLAT_tmem_op_u u;
        tmem_op_compat_t cop;

        rc = copy_from_guest(&cop, guest_handle_cast(uops, void), 1);
        if ( rc )
            return rc;
        switch ( cop.cmd )
        {
        case TMEM_NEW_POOL:   u = XLAT_tmem_op_u_creat; break;
        case TMEM_AUTH:       u = XLAT_tmem_op_u_creat; break;
        case TMEM_RESTORE_NEW:u = XLAT_tmem_op_u_creat; break;
        default:              u = XLAT_tmem_op_u_gen ;  break;
        }
#define XLAT_tmem_op_HNDL_u_ctrl_buf(_d_, _s_) \
        guest_from_compat_handle((_d_)->u.ctrl.buf, (_s_)->u.ctrl.buf)
        XLAT_tmem_op(op, &cop);
#undef XLAT_tmem_op_HNDL_u_ctrl_buf
        return 0;
    }
#endif
    return copy_from_guest(op, uops, 1);
}

Attachment: 0001-tmem-Make-the-uint64_t-oid-3-a-proper-structure-xen_.patch
Description: Text document

Attachment: 0002-tmem-Use-struct-xen_tmem_oid-for-all-every-user.patch
Description: Text document

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