Re: [PATCH v8 4/5] libxc: Provide set_xen_guest_handle_offset macro

[Ian Campbell <ian.campbell@citrix.com>]

On Fri, 2015-05-08 at 12:05 -0400, Boris Ostrovsky wrote:
> On 05/08/2015 11:56 AM, Ian Campbell wrote:
> > On Fri, 2015-05-08 at 11:41 -0400, Boris Ostrovsky wrote:
> >> On 05/08/2015 10:57 AM, Ian Campbell wrote:
> >>> On Wed, 2015-05-06 at 14:15 -0400, Boris Ostrovsky wrote:
> >>>> Add set_xen_guest_handle_offset() macro that can be used for setting
> >>>> xen_guest_handle to an offset into hypercall buffer.
> >>>>
> >>>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >>>> ---
> >>>>    tools/libxc/include/xenctrl.h |   11 ++++++++---
> >>>>    1 files changed, 8 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
> >>>> index 8c8eae6..4e6f62c 100644
> >>>> --- a/tools/libxc/include/xenctrl.h
> >>>> +++ b/tools/libxc/include/xenctrl.h
> >>>> @@ -322,16 +322,21 @@ typedef struct xc_hypercall_buffer xc_hypercall_buffer_t;
> >>>>     * Set a xen_guest_handle in a type safe manner, ensuring that the
> >>>>     * data pointer has been correctly allocated.
> >>>>     */
> >>>> -#undef set_xen_guest_handle
> >>>> -#define set_xen_guest_handle(_hnd, _val)                        \
> >>>> +#undef set_xen_guest_handle_offset
> >>> This isn't strictly speaking needed since you aren't trying to override
> >>> a version of this interface provided by the hypervisor headers.
> >>>
> >>>> +#define set_xen_guest_handle_offset(_hnd, _val, _off)           \
> >>>>        do {                                                        \
> >>>>            xc_hypercall_buffer_t _hcbuf_hnd1;                      \
> >>>>            typeof(XC__HYPERCALL_BUFFER_NAME(_val)) *_hcbuf_hnd2 =  \
> >>>>                    HYPERCALL_BUFFER(_val);                         \
> >>>>            (void) (&_hcbuf_hnd1 == _hcbuf_hnd2);                   \
> >>>> -        set_xen_guest_handle_raw(_hnd, (_hcbuf_hnd2)->hbuf);    \
> >>>> +        set_xen_guest_handle_raw(_hnd,                          \
> >>>> +                                 (_hcbuf_hnd2)->hbuf + (_off)); \
> >>> The hypervisor side guest_handle_add_offset equivalents operate in
> >>> multiples of the type. If we can arrange that here too then I think that
> >>> would be good.
> >>>
> >>> I think that means
> >>> +                                 (_hcbuf_hnd2)->hbuf + ((sizeof(*_val)*(_off))); \
> >>>
> >>> Then the caller becomes e.g.
> >>>
> >>> set_xen_guest_handle_offset(sysctl.u.pcitopoinfo.devs, devs, processed);
> >>>
> >>> which is much nicer IMHO.
> >>
> >> That's what I started with.
> >>
> >> This, however, breaks at least two things:
> >> --  Callers that pass HYPERCALL_BUFFER_NULL to set_xen_guest_handle().
> >> This I think can be fixed by declaring a void  *HYPERCALL_BUFFER_NULL
> >> (currenty HYPERCALL_BUFFER_NULL is just a syntactic building block)
> >>
> >> -- DECLARE_NAMED_HYPERCALL_BOUNCE. This makes _val a non-existing symbol
> >> (it creates a xc__hypercall_buffer_val). That I am not sure I know how
> >> to fix. Add type size filed to struct xc_hypercall_buffer?
> > How about refactoring such that this macro becomes an internal helper
> > used by both set_xen_guest_handle and set_xen_guest_handle_offset and
> > takes the (_hcbuf_hnd2)->hbuf or (_hcbuf_hnd2)->hbuf +
> > ((sizeof(*_val)*(_off)) as appropriate? (iow don't implement set as
> > set_offset(0))
> 
> But that will preclude us from using _offset() variant of the macro for 
> variables declared with DECLARE_NAMED_HYPERCALL_BOUNCE (and possibly 
> others, I haven't checked), won't it? We don't have any such uses now 
> but nevertheless...

Lets cross that bridge if/when we come to it.

Ian.