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

[Julien Grall <julien.grall@xxxxxxx>]

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?

Cheers,

--
Julien Grall

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