[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.