From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH RFC] tools/ocaml/xb: Correct calculations of data/space the ring Date: Wed, 28 Oct 2015 16:43:54 +0000 Message-ID: <5630FB4A.6090403@citrix.com> References: <1446048336-19154-1-git-send-email-andrew.cooper3@citrix.com> <20151028161813.GU2924@var.bordeaux.inria.fr> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20151028161813.GU2924@var.bordeaux.inria.fr> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Samuel Thibault , Xen-devel , Ian Campbell , Ian Jackson , Wei Liu , David Scott List-Id: xen-devel@lists.xenproject.org On 28/10/15 16:18, Samuel Thibault wrote: > Andrew Cooper, le Wed 28 Oct 2015 16:05:36 +0000, a =E9crit : >> @@ -62,22 +62,32 @@ CAMLprim value ml_interface_read(value ml_interface, >> = >> xen_mb(); >> = >> - if ((prod - cons) > XENSTORE_RING_SIZE) >> + if (((prod - cons) > XENSTORE_RING_SIZE) || >> + ((cons - prod) < -XENSTORE_RING_SIZE)) > prod and cons are uint32_t, so the difference will still be unsigned. The RHS will also be promoted to unsigned, as int has insufficient range for the LHS. > IIRC the test is not bogus even when prod wraps around, (prod - cons) > will still correctly be the difference between both, modulo 2^32. (prod - cons) >=3D XENSTORE_RING_SIZE checks for the prod getting more than a ring's worth ahead of cons. (cons - prod) < -XENSTORE_RING_SIZE is supposed to check for cons getting ahead of prod (the naive (cons > prod) fails when the ring wraps), although I notice it still buggy in the case where cons =3D=3D prod. > > Indeed, the read side also needs the same two-memcpy fix. > >> + /* Check for any pending data at all. */ >> + total_data =3D prod - cons; >> + if (total_data =3D=3D 0) { > ... >> + else if (total_data > len) >> + /* Some data - make a partial read. */ >> + len =3D total_data; >> + >> + /* Check whether data crosses the end of the ring. */ >> + data =3D XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(cons); >> + if (len <=3D data) >> + /* Data within the remaining part of the ring. */ >> + memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), len); >> + else { >> + /* Data crosses the ring boundary. Read both halves. */ >> + memcpy(buffer, intf->req + MASK_XENSTORE_IDX(cons), data); >> + memcpy(buffer + data, intf->req, len - data); >> + } >> + > That looks right and nice-looking to me. > >> xen_mb(); >> intf->req_cons +=3D len; >> result =3D len; >> @@ -100,7 +110,7 @@ CAMLprim value ml_interface_write(value ml_interface, >> = >> struct xenstore_domain_interface *intf =3D interface->addr; >> XENSTORE_RING_IDX cons, prod; >> - int can_write; >> + int total_space, space; >> uint32_t connection; >> = >> cons =3D *(volatile uint32_t*)&intf->rsp_cons; >> @@ -111,17 +121,33 @@ CAMLprim value ml_interface_write(value ml_interfa= ce, >> caml_raise_constant(*caml_named_value("Xb.Reconnect")); >> = >> xen_mb(); >> - if ( (prod - cons) >=3D XENSTORE_RING_SIZE ) { >> + >> + if (((prod - cons) > XENSTORE_RING_SIZE) || >> + ((cons - prod) < -XENSTORE_RING_SIZE)) > Ditto. > >> + /* Check for space to write the full message. */ >> + total_space =3D prod - cons; >> + if (total_space =3D=3D 0) { > Shouldn't that be total_space =3D=3D XENSTORE_RING_SIZE? Yes - I think it should. (I should probably kill my pending tests - they are not going to get very far...) ~Andrew > >> + /* No space at all - exit having done nothing. */ >> result =3D 0; >> goto exit; >> } >> + else if (total_space < len) >> + /* Some space - make a partial write. */ >> + len =3D total_space; >> + >> + /* Check for space until the ring wraps. */ >> + space =3D XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod); >> + if (len <=3D space ) >> + /* Message fits inside the remaining part of the ring. */ >> + memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, len); >> + else { >> + /* Message wraps around the end of the ring. Write both halves. */ >> + memcpy(intf->rsp + MASK_XENSTORE_IDX(prod), buffer, space); >> + memcpy(intf->rsp, buffer + space, len - space); >> + } > That looks right and nicer-looking than my attempt :) > > Samuel