Re: [Xen-devel] [PATCH v8 for-4.12 10/17] argo: implement the notify op

[Rich Persaud <persaur@xxxxxxxxx>]

On Feb 7, 2019, at 04:04, Julien Grall <julien.grall@xxxxxxx> wrote:
> 
> Hi,
> 
>> On 2/7/19 6:32 AM, Christopher Clark wrote:
>>> On Wed, Feb 6, 2019 at 10:28 AM Julien Grall <julien.grall@xxxxxxx> wrote:
>>> 
>>> Hi,
>>> 
>>>> On 2/6/19 8:55 AM, Christopher Clark wrote:
>>>> +/*
>>>> + * XEN_ARGO_OP_notify
>>>> + *
>>>> + * Asks Xen for information about other rings in the system.
>>>> + *
>>>> + * ent->ring is the xen_argo_addr_t of the ring you want information on.
>>>> + * Uses the same ring matching rules as XEN_ARGO_OP_sendv.
>>>> + *
>>>> + * ent->space_required : if this field is not null then Xen will check
>>>> + * that there is space in the destination ring for this many bytes of 
>>>> payload.
>>>> + * If the ring is too small for the requested space_required, it will set 
>>>> the
>>>> + * XEN_ARGO_RING_EMSGSIZE flag on return.
>>>> + * If sufficient space is available, it will set XEN_ARGO_RING_SUFFICIENT
>>>> + * and CANCEL any pending notification for that ent->ring; otherwise it
>>>> + * will schedule a notification event and the flag will not be set.
>>>> + *
>>>> + * These flags are set by Xen when notify replies:
>>>> + * XEN_ARGO_RING_EXISTS     ring exists
>>>> + * XEN_ARGO_RING_SHARED     ring is registered for wildcard partner
>>>> + * XEN_ARGO_RING_EMPTY      ring is empty
>>>> + * XEN_ARGO_RING_SUFFICIENT sufficient space for space_required is there
>>>> + * XEN_ARGO_RING_EMSGSIZE   space_required is too large for the ring size
>>>> + * XEN_ARGO_RING_EBUSY      too many domains waiting for available space 
>>>> signals
>>>> + *
>>>> + * arg1: XEN_GUEST_HANDLE(xen_argo_ring_data_t) ring_data (may be NULL)
>>>> + * arg2: NULL
>>> 
>>> NIT: While looking at all the hypercall, I noticed you say NULL here. In
>>> most of the cases, NULL will be 0 but there are place where it might not
>>> be. So what is the exact value you expect here?
>> The implementation tests both arg1 and arg2 with: guest_handle_is_null
>> which on both x86 and ARM expands to: ((hnd).p == NULL)
> 
> My point here is someone reading the public headers may not read the 
> implementation. So he/she would not know whether NULL is equivalent to 0 or 
> another value.
> 
>> It uses that null test because both are XEN_GUEST_HANDLE_PARAM type in
>> the function signature:
>> long do_argo_op(
>>     unsigned int cmd,
>>     XEN_GUEST_HANDLE_PARAM(void) arg1,
>>     XEN_GUEST_HANDLE_PARAM(void) arg2,
>>     unsigned long arg3,
>>     unsigned long arg4);
>> and since it does that, the comment states NULL rather than zero.
>> It is Xen's definition for NULL that is used, so the expected value in
>> the register when the hypercall is invoked is: zero.
> 
> As above, I don't think it is clearly define in the headers that NULL means 
> 0. This is rather an implicit written rules in the hope that every OS are 
> going to follow that.
> 
> I know, I am pedantic here (hence the NIT) :). And I realize this is not 
> related to this series and a few places in the code assumes the same.
> 
>>>> + * arg3: 0 (ZERO)
>>>> + * arg4: 0 (ZERO)
>>> 
>>> NIT: I guess those to will be 0 in an unsigned long value?
>> Yes.
> 
> Can this be clarified in a follow-up patch?

Since this is a comment revision, could it be made by the committer during the 
imminent merge of this Argo series?

Rich
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel