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

[Yang Hongyang <yanghy@cn.fujitsu.com>]

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.

>
> My argument here would be that the xc_hypercall_buffer_t created here is
> a hidden part of the internal hcall buf infrastructure and should be
> transparent to the user's code. The user's code only interacts with it
> via the magic macros which take the "user pointer" name, not the mangled
> xc_hypercall_buffer_t's name.
>
> I'm wary of creating too many accessors macros, or of creating too many
> back doors around the attempts to steer code away from using regular
> memory to pass to hypercalls.
>
> Ian.
>
> .
>

-- 
Thanks,
Yang.