Re: [PATCH Remus v2 04/10] tools/libxc: introduce DECLARE_HYPERCALL_BUFFER_USER_POINTER

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

On Tue, 2015-05-12 at 17:24 +0800, Yang Hongyang wrote:
> 
> On 05/12/2015 04:19 PM, Ian Campbell wrote:
> > On Tue, 2015-05-12 at 15:18 +0800, Yang Hongyang wrote:
> >>
> >> On 05/11/2015 07:53 PM, Ian Campbell wrote:
> >>> On Fri, 2015-05-08 at 17:33 +0800, Yang Hongyang wrote:
> >>>> Define a user pointer that can access the hypercall buffer data
> >>>> Useful when you only need to access the hypercall buffer data
> >>>
> >>> Can you expand on this please, I think the context is a hypercall buffer
> >>> passed as an argument or as a member of a struct. Please describe why
> >>> DECLARE_HYPERCALL_BUFFER_ARGUMENT and/or DECLARE_HYPERCALL_BUFFER_SHADOW
> >>> are not usable here.
> >>
> >> There are cases that we only need to use the hypercall buffer data, and do
> >> not use the xc_hypercall_buffer_t struct.
> >>
> >> DECLARE_HYPERCALL_BUFFER_ARGUMENT will only define a xc_hypercall_buffer_t,
> >> while DECLARE_HYPERCALL_BUFFER_SHADOW do define a user pointer that can allow
> >> us to access the hypercall buffer data but it also define a
> >> xc_hypercall_buffer_t that we don't use, the compiler will report arg unused
> >> error.
> >>
> >> The cases for example:
> >> In send_all_pages(), we only need to use the haypercall buffer data which is a
> >> dirty bitmap, we set the dirty bitmap to all dirty and call send_dirty_pages,
> >> we will not use the xc_hypercall_buffer_t and hypercall to retrive the dirty
> >> bitmap.
> >> In send_some_pages(), we will also only need to use the dirty_bitmap. the
> >> retrive dirty bitmap hypercall are done by the caller.
> >
> > Thanks. Please include the bulk of this description in the commit
> > message.
> >
> > However, perhaps we should just mark the xc_hypercall_buffer_t as
> > potentially unused, by tagging it with  __attribute__((unused)), in some
> > or all of the DECLARE_HYPERCALL_* variants. Then I think you could use
> > DECLARE_HYPERCALL_BUFFER_SHADOW as is?
> 
> If __attribute__((unused)) won't cause other problems here, I also prefer
> to add this to DECLARE_HYPERCALL_BUFFER_SHADOW instead of create another
> macro, I think only DECLARE_HYPERCALL_BUFFER_SHADOW may need to set this
> attribute because this is a shadow, we might not use the xc_hypercall_buffer_t
> which already allocated.

We can certainly start from there and consider other such additions as
the need arises.

Ian.