Re: [Xen-devel] [PATCH v7 10/15] argo: implement the notify op
On Mon, Feb 4, 2019 at 7:12 AM Jan Beulich wrote:
>
> >>> On 31.01.19 at 05:28, wrote:
> > @@ -1802,6 +2157,21 @@ do_argo_op(unsigned int cmd,
> > XEN_GUEST_HANDLE_PARAM(void) arg1,
> > break;
> > }
> >
> > +case XEN_ARGO_OP_notify:
> > +{
> > +XEN_GUEST_HANDLE_PARAM(xen_argo_ring_data_t) ring_data_hnd =
> > + guest_handle_cast(arg1, xen_argo_ring_data_t);
> > +
> > +if ( unlikely((!guest_handle_is_null(arg2)) || arg3 || arg4) )
> > +{
> > +rc = -EINVAL;
> > +break;
> > +}
> > +
> > +rc = notify(currd, ring_data_hnd);
> > +break;
> > +}
> > +
> > default:
> > rc = -EOPNOTSUPP;
> > break;
> > @@ -1912,6 +2282,21 @@ compat_argo_op(unsigned int cmd,
> > XEN_GUEST_HANDLE_PARAM(void) arg1,
> > break;
> > }
> >
> > +case XEN_ARGO_OP_notify:
> > +{
> > +XEN_GUEST_HANDLE_PARAM(xen_argo_ring_data_t) ring_data_hnd =
> > + guest_handle_cast(arg1, xen_argo_ring_data_t);
> > +
> > +if ( unlikely((!guest_handle_is_null(arg2)) || arg3 || arg4) )
> > +{
> > +rc = -EINVAL;
> > +break;
> > +}
> > +
> > +rc = notify(currd, ring_data_hnd);
> > +break;
> > +}
>
> At the example of this (likely applies to earlier patches as much): Aren't
> you afraid of this recurring duplication? It's quite easy, especially when
> the functions here grow a little further, for someone to forget updating
> one (more likely the compat one obviously). Did you consider forwarding
> all operations not needing translation straight into do_argo_op(), and
> handling only the sendv one here?
Ack, ok.
Christopher
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 10/15] argo: implement the notify op
>>> On 31.01.19 at 05:28, wrote:
> @@ -1802,6 +2157,21 @@ do_argo_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg1,
> break;
> }
>
> +case XEN_ARGO_OP_notify:
> +{
> +XEN_GUEST_HANDLE_PARAM(xen_argo_ring_data_t) ring_data_hnd =
> + guest_handle_cast(arg1, xen_argo_ring_data_t);
> +
> +if ( unlikely((!guest_handle_is_null(arg2)) || arg3 || arg4) )
> +{
> +rc = -EINVAL;
> +break;
> +}
> +
> +rc = notify(currd, ring_data_hnd);
> +break;
> +}
> +
> default:
> rc = -EOPNOTSUPP;
> break;
> @@ -1912,6 +2282,21 @@ compat_argo_op(unsigned int cmd,
> XEN_GUEST_HANDLE_PARAM(void) arg1,
> break;
> }
>
> +case XEN_ARGO_OP_notify:
> +{
> +XEN_GUEST_HANDLE_PARAM(xen_argo_ring_data_t) ring_data_hnd =
> + guest_handle_cast(arg1, xen_argo_ring_data_t);
> +
> +if ( unlikely((!guest_handle_is_null(arg2)) || arg3 || arg4) )
> +{
> +rc = -EINVAL;
> +break;
> +}
> +
> +rc = notify(currd, ring_data_hnd);
> +break;
> +}
At the example of this (likely applies to earlier patches as much): Aren't
you afraid of this recurring duplication? It's quite easy, especially when
the functions here grow a little further, for someone to forget updating
one (more likely the compat one obviously). Did you consider forwarding
all operations not needing translation straight into do_argo_op(), and
handling only the sendv one here?
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 10/15] argo: implement the notify op
On Thu, Jan 31, 2019 at 8:45 AM Roger Pau Monné wrote:
>
> On Wed, Jan 30, 2019 at 08:28:15PM -0800, Christopher Clark wrote:
> > Queries for data about space availability in registered rings and
> > causes notification to be sent when space has become available.
> >
> > The hypercall op populates a supplied data structure with information about
> > ring state and if insufficient space is currently available in a given ring,
> > the hypervisor will record the domain's expressed interest and notify it
> > when it observes that space has become available.
> >
> > Checks for free space occur when this notify op is invoked, so it may be
> > intentionally invoked with no data structure to populate
> > (ie. a NULL argument) to trigger such a check and consequent notifications.
> >
> > Limit the maximum number of notify requests in a single operation to a
> > simple fixed limit of 256.
> >
> > Signed-off-by: Christopher Clark
> > Tested-by: Chris Patterson
>
> Reviewed-by: Roger Pau Monné
Thanks.
>
> Despite the usage of list_for_each_entry_safe instead of
> list_first_entry_or_null.
>
> > +static void
> > +pending_notify(struct list_head *to_notify)
> > +{
> > +struct pending_ent *ent, *next;
> > +
> > +ASSERT(LOCKING_Read_L1);
> > +
> > +/* Sending signals for all ents in this list, draining until it is
> > empty. */
> > +list_for_each_entry_safe(ent, next, to_notify, node)
>
> list_first_entry_or_null would be more suitable here.
ack, applied.
Christopher
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v7 10/15] argo: implement the notify op
On Wed, Jan 30, 2019 at 08:28:15PM -0800, Christopher Clark wrote:
> Queries for data about space availability in registered rings and
> causes notification to be sent when space has become available.
>
> The hypercall op populates a supplied data structure with information about
> ring state and if insufficient space is currently available in a given ring,
> the hypervisor will record the domain's expressed interest and notify it
> when it observes that space has become available.
>
> Checks for free space occur when this notify op is invoked, so it may be
> intentionally invoked with no data structure to populate
> (ie. a NULL argument) to trigger such a check and consequent notifications.
>
> Limit the maximum number of notify requests in a single operation to a
> simple fixed limit of 256.
>
> Signed-off-by: Christopher Clark
> Tested-by: Chris Patterson
Reviewed-by: Roger Pau Monné
Despite the usage of list_for_each_entry_safe instead of
list_first_entry_or_null.
> +static void
> +pending_notify(struct list_head *to_notify)
> +{
> +struct pending_ent *ent, *next;
> +
> +ASSERT(LOCKING_Read_L1);
> +
> +/* Sending signals for all ents in this list, draining until it is
> empty. */
> +list_for_each_entry_safe(ent, next, to_notify, node)
list_first_entry_or_null would be more suitable here.
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v7 10/15] argo: implement the notify op
Queries for data about space availability in registered rings and
causes notification to be sent when space has become available.
The hypercall op populates a supplied data structure with information about
ring state and if insufficient space is currently available in a given ring,
the hypervisor will record the domain's expressed interest and notify it
when it observes that space has become available.
Checks for free space occur when this notify op is invoked, so it may be
intentionally invoked with no data structure to populate
(ie. a NULL argument) to trigger such a check and consequent notifications.
Limit the maximum number of notify requests in a single operation to a
simple fixed limit of 256.
Signed-off-by: Christopher Clark
Tested-by: Chris Patterson
---
v6 #09 Jan: add compat ABI
v6 #04 Jan: xlat.lst: move argo struct entries to alphabetical position
v6: rewrap comment to fix line length
v6 #10 Roger: use list_for_each_entry{_safe} for looping
v5: add EBUSY ent flag when too many domains are already on pending list
v5: reorder notify flags: error flags last, fixed state first
v5: add compat validation macros to primary source file: common/argo.c
v5 : convert hypercall arg structs to struct form for compat checking
v5: dropped external file for compat macros: common/compat/argo.c
v4 #10 Roger: consolidate notify flags; infer pending notify if needed
v4 bugfix: take L3 before accessing ring_info in fill_ring_data
v4 #10 Roger: shorten notify flag names: drop _DATA_F
v4 #10 self/Roger: fill_ring_data: check pending_requeue error code
v4 : use standard data structures as per common code
v4 #10 Roger: lower indentation in fill_ring_data by using goto
v4 #10 Roger: reword the XEN_ARGO_RING_DATA_F_SUFFICIENT comment
v4 fix location of a FIXME that was incorrectly moved by this later commit
v3 #07 Jan: fix format string indention in printks
v3 (general) Jan: drop fixed width types for ringbuf_payload_space
v3 #07 Jan: rename ring_find_info_by_match to find_ring_info_by_match
v3 #07 Jan: fix numeric entries in printk format strings
v3: ringbuf_payload_space: simpler return 0 if get_sanitized_ring fails
v3 #10 Roger: simplify ringbuf_payload_space for empty rings
v3 #10 Roger: ringbuf_payload_space: add comment to explain how ret < INT32_MAX
v3 #10 Roger: drop out label, use return -EFAULT in fill_ring_data
v3 #10 Roger: add newline in signal_domid
v3 #10 Roger: move find functions to top of file and drop prototypes
v3 #04 Jan: meld the compat hypercall arg checking
v3 #04 Roger/Jan: make lock names clearer and assert their state
v3 #04 Jan: port -> aport with type; distinguish argo port from evtchn
v3 self: drop braces in foreach of notify_check_pending
v3 feedback Roger/Jan: ASSERT currd is current->domain or use 'd' variable name
v2 feedback Jan: drop cookie, implement teardown
v2 notify: add flag to indicate ring is shared
v2 argument name for fill_ring_data arg is now currd
v2 self: check ring size vs request and flag error rather than queue signal
v2 feedback Jan: drop 'message' from 'argo_message_op'
v2 self: simplify signal_domid, drop unnecessary label + goto
v2 self: skip the cookie check in pending_cancel
v2 self: implement npending limit on number of pending entries
v1 feedback #16 Jan: sanitize_ring in ringbuf_payload_space
v2 self: inline fill_ring_data_array
v2 self: avoid retesting dst_d for put_domain
v2 self/Jan: remove use of magic verification field and tidy up
v1 feedback #16 Jan: remove testing of magic in guest-supplied structure
v2 self: s/argo_pending_ent/pending_ent/g
v2 feedback v1#13 Roger: use OS-supplied roundup; drop from public header
v1,2 feedback Jan/Roger/Paul: drop errno returning guest access functions
v1 feedback Roger, Jan: drop argo prefix on static functions
v2 self: reduce indentation via goto out if arg NULL
v1 feedback #13 Jan: resolve checking of array handle and use of __copy
v1 #5 (#16) feedback Paul: notify op: use currd in do_argo_message_op
v1 #5 (#16) feedback Paul: notify op: use currd in argo_notify
v1 #5 (#16) feedback Paul: notify op: use currd in argo_notify_check_pending
v1 #5 (#16) feedback Paul: notify op: use currd in argo_fill_ring_data_array
v1 #13 (#16) feedback Paul: notify op: do/while: reindent only
v1 #13 (#16) feedback Paul: notify op: do/while: goto
v1 : add compat xlat.lst entries
v1: add definition for copy_field_from_guest_errno
v1 #13 feedback Jan: make 'ring data' comment comply with single-line style
v1 feedback #13 Jan: use __copy; so define and use __copy_field_to_guest_errno
v1: #13 feedback Jan: public namespace: prefix with xen
v1: #13 feedback Jan: add blank line after case in do_argo_message_op
v1: self: rename ent id to domain_id
v1: self: ent id-> domain_id
v1: self: drop signal if domain_cookie mismatches
v1. feedback #15 Jan: make loop i unsigned
v1. self: drop unnecessary mb() in argo_notify_check_pending
v1. self: add blank line
v1 #16 feedback Jan: const domain arg to +argo_fill_ring_data
v1. feedback #15 Jan: ch
