From minios-devel-bounces@lists.xenproject.org Wed Nov 24 07:01:03 2021 Return-path: Envelope-to: archives@lists.xenproject.org Delivery-date: Wed, 24 Nov 2021 07:01:03 +0000 Received: from list by lists.xenproject.org with outflank-mailman.230078.397791 (Exim 4.92) (envelope-from ) id 1mpmH7-0005a1-LZ; Wed, 24 Nov 2021 07:01:01 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 230078.397791; Wed, 24 Nov 2021 07:01:01 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1mpmH7-0005Zp-I3; Wed, 24 Nov 2021 07:01:01 +0000 Received: by outflank-mailman (input) for mailman id 230078; Wed, 24 Nov 2021 07:01:00 +0000 Received: from se1-gles-flk1-in.inumbo.com ([94.247.172.50] helo=se1-gles-flk1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1mpmH6-0005YU-6m for minios-devel@lists.xenproject.org; Wed, 24 Nov 2021 07:01:00 +0000 Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by se1-gles-flk1.inumbo.com (Halon) with ESMTPS id 45f40cf3-4cf4-11ec-a9d2-d9f7a1cc8784; Wed, 24 Nov 2021 08:00:58 +0100 (CET) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id C9F842195D; Wed, 24 Nov 2021 07:00:56 +0000 (UTC) Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 953EF13240; Wed, 24 Nov 2021 07:00:56 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id QinaIijjnWHdCAAAMHmgww (envelope-from ); Wed, 24 Nov 2021 07:00:56 +0000 X-BeenThere: minios-devel@lists.xenproject.org List-Id: Mini-os development list List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: minios-devel-bounces@lists.xenproject.org Precedence: list Sender: "Minios-devel" X-Inumbo-ID: 45f40cf3-4cf4-11ec-a9d2-d9f7a1cc8784 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1637737256; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=CwiRND9sUmIgpLpyZ+X1paDgsGd1UAbnBN7Tro+bIjg=; b=adVOxqqoivBQ6u91xCkV/oqFHK21Lg3Qmyyy7ggvyQOW23BYT7RNib0l4zmVNZnQCt7aiY K6kTKJOuADUvQi0RT/ovQih1O4IcRc1rs83eoOAFST3pjjxjoMAcLUA3vnxagT6YWhXC1X QhD+CbdQ+3IjxzqTU8hMDLTIq1WhkkM= Subject: Re: [PATCH v4] xenbus: support large messages To: minios-devel@lists.xenproject.org, xen-devel@lists.xenproject.org Cc: samuel.thibault@ens-lyon.org, wl@xen.org References: <20211004094000.29868-1-jgross@suse.com> From: Juergen Gross Message-ID: <244e6bc7-9158-7c52-13ab-d25daafbc2c8@suse.com> Date: Wed, 24 Nov 2021 08:00:55 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <20211004094000.29868-1-jgross@suse.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Y181P6BFv7tgthqU2tAOS3T9XmO1q283n" This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --Y181P6BFv7tgthqU2tAOS3T9XmO1q283n Content-Type: multipart/mixed; boundary="dJ65ISaETfggy1VefxjzJAcoxxz0j18yK"; protected-headers="v1" From: Juergen Gross To: minios-devel@lists.xenproject.org, xen-devel@lists.xenproject.org Cc: samuel.thibault@ens-lyon.org, wl@xen.org Message-ID: <244e6bc7-9158-7c52-13ab-d25daafbc2c8@suse.com> Subject: Re: [PATCH v4] xenbus: support large messages References: <20211004094000.29868-1-jgross@suse.com> In-Reply-To: <20211004094000.29868-1-jgross@suse.com> --dJ65ISaETfggy1VefxjzJAcoxxz0j18yK Content-Type: multipart/mixed; boundary="------------6E2ADA8ABB96A4523042F315" Content-Language: en-US This is a multi-part message in MIME format. --------------6E2ADA8ABB96A4523042F315 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Ping? On 04.10.21 11:40, Juergen Gross wrote: > Today the implementation of the xenbus protocol in Mini-OS will only > allow to transfer the complete message to or from the ring page buffer.= > This is limiting the maximum message size to lower values as the xenbus= > protocol normally would allow. >=20 > Change that by allowing to transfer the xenbus message in chunks as > soon as they are available. >=20 > Avoid crashing Mini-OS in case of illegal data read from the ring > buffer. >=20 > Signed-off-by: Juergen Gross > --- > V2: > - drop redundant if (Samuel Thibault) > - move rmb() (Samuel Thibault) > V3: > - correct notification test (Samuel Thibault) > V4: > - more memory barriers (Samuel Thibault) > --- > xenbus/xenbus.c | 210 ++++++++++++++++++++++++++++-------------------= - > 1 file changed, 122 insertions(+), 88 deletions(-) >=20 > diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c > index 23de61e..b687678 100644 > --- a/xenbus/xenbus.c > +++ b/xenbus/xenbus.c > @@ -29,6 +29,7 @@ > #include > #include > #include > +#include > =20 > #define min(x,y) ({ \ > typeof(x) tmpx =3D (x); \ > @@ -46,6 +47,7 @@ > static struct xenstore_domain_interface *xenstore_buf; > static DECLARE_WAIT_QUEUE_HEAD(xb_waitq); > DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue); > +static __DECLARE_SEMAPHORE_GENERIC(xb_write_sem, 1); > =20 > xenbus_event_queue xenbus_events; > static struct watch { > @@ -231,75 +233,103 @@ char *xenbus_wait_for_state_change(const char* p= ath, XenbusState *state, xenbus_ > } > =20 > =20 > +static void xenbus_read_data(char *buf, unsigned int len) > +{ > + unsigned int off =3D 0; > + unsigned int prod, cons; > + unsigned int size; > + > + while (off !=3D len) > + { > + wait_event(xb_waitq, xenstore_buf->rsp_prod !=3D xenstore_buf-= >rsp_cons); > + > + prod =3D xenstore_buf->rsp_prod; > + cons =3D xenstore_buf->rsp_cons; > + DEBUG("Rsp_cons %d, rsp_prod %d.\n", cons, prod); > + size =3D min(len - off, prod - cons); > + > + rmb(); /* Make sure data read from ring is ordered with rsp_= prod. */ > + memcpy_from_ring(xenstore_buf->rsp, buf + off, > + MASK_XENSTORE_IDX(cons), size); > + off +=3D size; > + mb(); /* memcpy() and rsp_cons update must not be reordered= =2E */ > + xenstore_buf->rsp_cons +=3D size; > + mb(); /* rsp_cons must be visible before we look at rsp_pro= d. */ > + if (xenstore_buf->rsp_prod - cons >=3D XENSTORE_RING_SIZE) > + notify_remote_via_evtchn(xenbus_evtchn); > + } > +} > + > static void xenbus_thread_func(void *ign) > { > struct xsd_sockmsg msg; > - unsigned prod =3D xenstore_buf->rsp_prod; > + char *data; > =20 > for (;;) { > - wait_event(xb_waitq, prod !=3D xenstore_buf->rsp_prod); > - while (1) { > - prod =3D xenstore_buf->rsp_prod; > - DEBUG("Rsp_cons %d, rsp_prod %d.\n", xenstore_buf->rsp_con= s, > - xenstore_buf->rsp_prod); > - if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < size= of(msg)) > - break; > - rmb(); > - memcpy_from_ring(xenstore_buf->rsp, &msg, > - MASK_XENSTORE_IDX(xenstore_buf->rsp_cons)= , > - sizeof(msg)); > - DEBUG("Msg len %d, %d avail, id %d.\n", msg.len + sizeof(m= sg), > - xenstore_buf->rsp_prod - xenstore_buf->rsp_cons, msg= =2Ereq_id); > - > - if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < > - sizeof(msg) + msg.len) > - break; > - > - DEBUG("Message is good.\n"); > - > - if (msg.type =3D=3D XS_WATCH_EVENT) { > - struct xenbus_event *event =3D malloc(sizeof(*event) += msg.len); > - xenbus_event_queue *events =3D NULL; > - char *data =3D (char*)event + sizeof(*event); > - struct watch *watch; > - > - memcpy_from_ring(xenstore_buf->rsp, data, > - MASK_XENSTORE_IDX(xenstore_buf->rsp_cons + sizeof(= msg)), > - msg.len); > - > - event->path =3D data; > - event->token =3D event->path + strlen(event->path) + 1= ; > - > - mb(); > - xenstore_buf->rsp_cons +=3D msg.len + sizeof(msg); > - > - for (watch =3D watches; watch; watch =3D watch->next) > - if (!strcmp(watch->token, event->token)) { > - events =3D watch->events; > - break; > - } > - > - if (events) { > - event->next =3D *events; > - *events =3D event; > - wake_up(&xenbus_watch_queue); > - } else { > - printk("unexpected watch token %s\n", event->token= ); > - free(event); > + xenbus_read_data((char *)&msg, sizeof(msg)); > + DEBUG("Msg len %d, %d avail, id %d.\n", msg.len + sizeof(msg),= > + xenstore_buf->rsp_prod - xenstore_buf->rsp_cons, msg.req= _id); > + > + if (msg.len > XENSTORE_PAYLOAD_MAX) { > + printk("Xenstore violates protocol, message longer than al= lowed.\n"); > + return; > + } > + > + if (msg.type =3D=3D XS_WATCH_EVENT) { > + struct xenbus_event *event =3D malloc(sizeof(*event) + msg= =2Elen); > + xenbus_event_queue *events =3D NULL; > + struct watch *watch; > + char *c; > + int zeroes =3D 0; > + > + data =3D (char*)event + sizeof(*event); > + xenbus_read_data(data, msg.len); > + > + for (c =3D data; c < data + msg.len; c++) > + if (!*c) > + zeroes++; > + if (zeroes !=3D 2) { > + printk("Xenstore: illegal watch event data\n"); > + free(event); > + continue; > + } > + > + event->path =3D data; > + event->token =3D event->path + strlen(event->path) + 1; > + > + for (watch =3D watches; watch; watch =3D watch->next) > + if (!strcmp(watch->token, event->token)) { > + events =3D watch->events; > + break; > } > + > + if (events) { > + event->next =3D *events; > + *events =3D event; > + wake_up(&xenbus_watch_queue); > } else { > - req_info[msg.req_id].reply =3D malloc(sizeof(msg) + ms= g.len); > - memcpy_from_ring(xenstore_buf->rsp, req_info[msg.req_i= d].reply, > - MASK_XENSTORE_IDX(xenstore_buf->rsp_c= ons), > - msg.len + sizeof(msg)); > - mb(); > - xenstore_buf->rsp_cons +=3D msg.len + sizeof(msg); > - wake_up(&req_info[msg.req_id].waitq); > + printk("Xenstore: unexpected watch token %s\n", event-= >token); > + free(event); > } > =20 > - wmb(); > - notify_remote_via_evtchn(xenbus_evtchn); > + continue; > } > + > + data =3D malloc(sizeof(msg) + msg.len); > + memcpy(data, &msg, sizeof(msg)); > + xenbus_read_data(data + sizeof(msg), msg.len); > + > + if (msg.req_id >=3D NR_REQS || !req_info[msg.req_id].in_use) {= > + printk("Xenstore: illegal request id %d\n", msg.req_id); > + free(data); > + continue; > + } > + > + DEBUG("Message is good.\n"); > + > + req_info[msg.req_id].reply =3D data; > + > + wake_up(&req_info[msg.req_id].waitq); > } > } > =20 > @@ -451,36 +481,40 @@ static void xb_write(int type, int req_id, xenbus= _transaction_t trans_id, > =20 > cur_req =3D &header_req; > =20 > - BUG_ON(len > XENSTORE_RING_SIZE); > - /* Wait for the ring to drain to the point where we can send the > - message. */ > - prod =3D xenstore_buf->req_prod; > - if (prod + len - xenstore_buf->req_cons > XENSTORE_RING_SIZE) > - { > - /* Wait for there to be space on the ring */ > - DEBUG("prod %d, len %d, cons %d, size %d; waiting.\n", > - prod, len, xenstore_buf->req_cons, XENSTORE_RING_SIZE)= ; > - wait_event(xb_waitq, > - xenstore_buf->req_prod + len - xenstore_buf->req_cons = <=3D > - XENSTORE_RING_SIZE); > - DEBUG("Back from wait.\n"); > - prod =3D xenstore_buf->req_prod; > - } > + BUG_ON(len > XENSTORE_PAYLOAD_MAX); > + > + /* Make sure we are the only thread trying to write. */ > + down(&xb_write_sem); > =20 > - /* We're now guaranteed to be able to send the message without > - overflowing the ring. Do so. */ > + /* Send the message in chunks using free ring space when available= =2E */ > total_off =3D 0; > req_off =3D 0; > - while (total_off < len) > + while (total_off < len) > { > + prod =3D xenstore_buf->req_prod; > + if (prod - xenstore_buf->req_cons >=3D XENSTORE_RING_SIZE) > + { > + /* Send evtchn to notify remote */ > + notify_remote_via_evtchn(xenbus_evtchn); > + > + /* Wait for there to be space on the ring */ > + DEBUG("prod %d, len %d, cons %d, size %d; waiting.\n", pro= d, > + len - total_off, xenstore_buf->req_cons, XENSTORE_RI= NG_SIZE); > + wait_event(xb_waitq, > + prod - xenstore_buf->req_cons < XENSTORE_RING_S= IZE); > + DEBUG("Back from wait.\n"); > + } > + > this_chunk =3D min(cur_req->len - req_off, > - XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod)); > + XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod))= ; > + this_chunk =3D min(this_chunk, > + xenstore_buf->req_cons + XENSTORE_RING_SIZE -= prod); > memcpy((char *)xenstore_buf->req + MASK_XENSTORE_IDX(prod), > - (char *)cur_req->data + req_off, this_chunk); > + (char *)cur_req->data + req_off, this_chunk); > prod +=3D this_chunk; > req_off +=3D this_chunk; > total_off +=3D this_chunk; > - if (req_off =3D=3D cur_req->len) > + if (req_off =3D=3D cur_req->len) > { > req_off =3D 0; > if (cur_req =3D=3D &header_req) > @@ -488,20 +522,20 @@ static void xb_write(int type, int req_id, xenbus= _transaction_t trans_id, > else > cur_req++; > } > + > + /* Remote must see entire message before updating indexes */ > + wmb(); > + xenstore_buf->req_prod =3D prod; > } > =20 > + /* Send evtchn to notify remote */ > + notify_remote_via_evtchn(xenbus_evtchn); > + > DEBUG("Complete main loop of xb_write.\n"); > BUG_ON(req_off !=3D 0); > BUG_ON(total_off !=3D len); > - BUG_ON(prod > xenstore_buf->req_cons + XENSTORE_RING_SIZE); > =20 > - /* Remote must see entire message before updating indexes */ > - wmb(); > - > - xenstore_buf->req_prod +=3D len; > - > - /* Send evtchn to notify remote */ > - notify_remote_via_evtchn(xenbus_evtchn); > + up(&xb_write_sem); > } > =20 > /* Send a mesasge to xenbus, in the same fashion as xb_write, and >=20 --------------6E2ADA8ABB96A4523042F315 Content-Type: application/pgp-keys; name="OpenPGP_0xB0DE9DD628BF132F.asc" Content-Transfer-Encoding: quoted-printable Content-Description: OpenPGP public key Content-Disposition: attachment; filename="OpenPGP_0xB0DE9DD628BF132F.asc" -----BEGIN PGP PUBLIC KEY BLOCK----- xsBNBFOMcBYBCACgGjqjoGvbEouQZw/ToiBg9W98AlM2QHV+iNHsEs7kxWhKMjrioyspZKOBy= cWx w3ie3j9uvg9EOB3aN4xiTv4qbnGiTr3oJhkB1gsb6ToJQZ8uxGq2kaV2KL9650I1SJvedYm8O= f8Z d621lSmoKOwlNClALZNew72NjJLEzTalU1OdT7/i1TXkH09XSSI8mEQ/ouNcMvIJNwQpd369y= 9bf IhWUiVXEK7MlRgUG6MvIj6Y3Am/BBLUVbDa4+gmzDC9ezlZkTZG2t14zWPvxXP3FAp2pkW0xq= G7/ 377qptDmrk42GlSKN4z76ELnLxussxc7I2hx18NUcbP8+uty4bMxABEBAAHNHEp1ZXJnZW4gR= 3Jv c3MgPGpnQHBmdXBmLm5ldD7CwHkEEwECACMFAlOMcBYCGwMHCwkIBwMCAQYVCAIJCgsEFgIDA= QIe AQIXgAAKCRCw3p3WKL8TL0KdB/93FcIZ3GCNwFU0u3EjNbNjmXBKDY4FUGNQH2lvWAUy+dnyT= hpw dtF/jQ6j9RwE8VP0+NXcYpGJDWlNb9/JmYqLiX2Q3TyevpB0CA3dbBQp0OW0fgCetToGIQrg0= MbD 1C/sEOv8Mr4NAfbauXjZlvTj30H2jO0u+6WGM6nHwbh2l5O8ZiHkH32iaSTfN7Eu5RnNVUJbv= oPH Z8SlM4KWm8rG+lIkGurqqu5gu8q8ZMKdsdGC4bBxdQKDKHEFExLJK/nRPFmAuGlId1E3fe10v= 5QL +qHI3EIPtyfE7i9Hz6rVwi7lWKgh7pe0ZvatAudZ+JNIlBKptb64FaiIOAWDCx1SzR9KdWVyZ= 2Vu IEdyb3NzIDxqZ3Jvc3NAc3VzZS5jb20+wsB5BBMBAgAjBQJTjHCvAhsDBwsJCAcDAgEGFQgCC= QoL BBYCAwECHgECF4AACgkQsN6d1ii/Ey/HmQf/RtI7kv5A2PS4RF7HoZhPVPogNVbC4YA6lW7Dr= Wf0 teC0RR3MzXfy6pJ+7KLgkqMlrAbN/8Dvjoz78X+5vhH/rDLa9BuZQlhFmvcGtCF8eR0T1v0nC= /nu AFVGy+67q2DH8As3KPu0344TBDpAvr2uYM4tSqxK4DURx5INz4ZZ0WNFHcqsfvlGJALDeE0Lh= ITT d9jLzdDad1pQSToCnLl6SBJZjDOX9QQcyUigZFtCXFst4dlsvddrxyqT1f17+2cFSdu7+ynLm= XBK 7abQ3rwJY8SbRO2iRulogc5vr/RLMMlscDAiDkaFQWLoqHHOdfO9rURssHNN8WkMnQfvUewRz= 80h SnVlcmdlbiBHcm9zcyA8amdyb3NzQG5vdmVsbC5jb20+wsB5BBMBAgAjBQJTjHDXAhsDBwsJC= AcD AgEGFQgCCQoLBBYCAwECHgECF4AACgkQsN6d1ii/Ey8PUQf/ehmgCI9jB9hlgexLvgOtf7PJn= FOX gMLdBQgBlVPO3/D9R8LtF9DBAFPNhlrsfIG/SqICoRCqUcJ96Pn3P7UUinFG/I0ECGF4EvTE1= jnD kfJZr6jrbjgyoZHiw/4BNwSTL9rWASyLgqlA8u1mf+c2yUwcGhgkRAd1gOwungxcwzwqgljf0= N51 N5JfVRHRtyfwq/ge+YEkDGcTU6Y0sPOuj4Dyfm8fJzdfHNQsWq3PnczLVELStJNdapwPOoE+l= otu fe3AM2vAEYJ9rTz3Cki4JFUsgLkHFqGZarrPGi1eyQcXeluldO3m91NK/1xMI3/+8jbO0tsn1= tqS EUGIJi7ox80eSnVlcmdlbiBHcm9zcyA8amdyb3NzQHN1c2UuZGU+wsB5BBMBAgAjBQJTjHDrA= hsD BwsJCAcDAgEGFQgCCQoLBBYCAwECHgECF4AACgkQsN6d1ii/Ey+LhQf9GL45eU5vOowA2u5N3= g3O ZUEBmDHVVbqMtzwlmNC4k9Kx39r5s2vcFl4tXqW7g9/ViXYuiDXb0RfUpZiIUW89siKrkzmQ5= dM7 wRqzgJpJwK8Bn2MIxAKArekWpiCKvBOB/Cc+3EXE78XdlxLyOi/NrmSGRIov0karw2RzMNOu5= D+j LRZQd1Sv27AR+IP3I8U4aqnhLpwhK7MEy9oCILlgZ1QZe49kpcumcZKORmzBTNh30FVKK1Evm= V2x AKDoaEOgQB4iFQLhJCdP1I5aSgM5IVFdn7v5YgEYuJYx37IoN1EblHI//x/e2AaIHpzK5h88N= Eaw QsaNRpNSrcfbFmAg987ATQRTjHAWAQgAyzH6AOODMBjgfWE9VeCgsrwH3exNAU32gLq2xvjpW= nHI s98ndPUDpnoxWQugJ6MpMncr0xSwFmHEgnSEjK/PAjppgmyc57BwKII3sV4on+gDVFJR6Y8ZR= wgn BC5mVM6JjQ5xDk8WRXljExRfUX9pNhdE5eBOZJrDRoLUmmjDtKzWaDhIg/+1Hzz93X4fCQkNV= bVF LELU9bMaLPBG/x5q4iYZ2k2ex6d47YE1ZFdMm6YBYMOljGkZKwYde5ldM9mo45mmwe0icXKLk= pEd IXKTZeKDO+Hdv1aqFuAcccTg9RXDQjmwhC3yEmrmcfl0+rPghO0Iv3OOImwTEe4co3c1mwARA= QAB wsBfBBgBAgAJBQJTjHAWAhsMAAoJELDendYovxMvQ/gH/1ha96vm4P/L+bQpJwrZ/dneZcmEw= Tbe 8YFsw2V/Buv6Z4Mysln3nQK5ZadD534CF7TDVft7fC4tU4PONxF5D+/tvgkPfDAfF77zy2AH1= vJz Q1fOU8lYFpZXTXIHb+559UqvIB8AdgR3SAJGHHt4RKA0F7f5ipYBBrC6cyXJyyoprT10EMvU8= VGi wXvTyJz3fjoYsdFzpWPlJEBRMedCot60g5dmbdrZ5DWClAr0yau47zpWj3enf1tLWaqcsuylW= svi uGjKGw7KHQd3bxALOknAp4dN3QwBYCKuZ7AddY9yjynVaD5X7nF9nO5BjR/i1DG86lem3iBDX= zXs ZDn8R38=3D =3D2wuH -----END PGP PUBLIC KEY BLOCK----- --------------6E2ADA8ABB96A4523042F315-- --dJ65ISaETfggy1VefxjzJAcoxxz0j18yK-- --Y181P6BFv7tgthqU2tAOS3T9XmO1q283n Content-Type: application/pgp-signature; name="OpenPGP_signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="OpenPGP_signature" -----BEGIN PGP SIGNATURE----- wsB5BAABCAAjFiEEhRJncuj2BJSl0Jf3sN6d1ii/Ey8FAmGd4ycFAwAAAAAACgkQsN6d1ii/Ey8r /gf/d+UjYEoQh9p/TToGUFUQ3mxUMT+FCS89F5+2W0aHiHQ1E/8xi6N8caTBtw79TUSJdxcibWYb joLIAK9li+92dkgUunJ0cxu/v+x2BNEDAIq/65wPUotunS/8f+ketntkc7W+OS/DMW7p4jB2XHEe jwBMVTD69oT33eRpbLa18Vbc2M0s/jxk/iVbzuvp4GvKHqR2O4XeP0S5hSjwUxCn2+1sJAWfqxz5 j7166+x3oHaSVmP03gsZ8IM1Fw7+gbsw3HSci3b5O7BruJfcTxmT2ybRiVhtkdLuBr7Y1BclJGLw BLpugvf7IZ255XdD6pg91vvxcoi5pEEDRtvGL4fbxA== =8G1s -----END PGP SIGNATURE----- --Y181P6BFv7tgthqU2tAOS3T9XmO1q283n-- From minios-devel-bounces@lists.xenproject.org Sun Nov 28 19:36:21 2021 Return-path: Envelope-to: archives@lists.xenproject.org Delivery-date: Sun, 28 Nov 2021 19:36:21 +0000 Received: from list by lists.xenproject.org with outflank-mailman.234192.406467 (Exim 4.92) (envelope-from ) id 1mrPyD-0004xZ-4Q; Sun, 28 Nov 2021 19:36:17 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 234192.406467; Sun, 28 Nov 2021 19:36:17 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1mrPyD-0004xS-1I; Sun, 28 Nov 2021 19:36:17 +0000 Received: by outflank-mailman (input) for mailman id 234192; Sun, 28 Nov 2021 19:36:15 +0000 Received: from se1-gles-sth1-in.inumbo.com ([159.253.27.254] helo=se1-gles-sth1.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1mrPyB-0004xH-Ey for minios-devel@lists.xenproject.org; Sun, 28 Nov 2021 19:36:15 +0000 Received: from hera.aquilenet.fr (hera.aquilenet.fr [185.233.100.1]) by se1-gles-sth1.inumbo.com (Halon) with ESMTPS id 6fbc0b51-5082-11ec-976b-d102b41d0961; Sun, 28 Nov 2021 20:36:13 +0100 (CET) Received: from localhost (localhost [127.0.0.1]) by hera.aquilenet.fr (Postfix) with ESMTP id 86CF028D; Sun, 28 Nov 2021 20:36:08 +0100 (CET) Received: from hera.aquilenet.fr ([127.0.0.1]) by localhost (hera.aquilenet.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id EuzZJlBBoMwa; Sun, 28 Nov 2021 20:36:07 +0100 (CET) Received: from begin (unknown [IPv6:2a01:cb19:956:1b00:de41:a9ff:fe47:ec49]) by hera.aquilenet.fr (Postfix) with ESMTPSA id 036A1194; Sun, 28 Nov 2021 20:36:06 +0100 (CET) Received: from samy by begin with local (Exim 4.95) (envelope-from ) id 1mrPy1-006Dgr-EK; Sun, 28 Nov 2021 20:36:05 +0100 X-BeenThere: minios-devel@lists.xenproject.org List-Id: Mini-os development list List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: minios-devel-bounces@lists.xenproject.org Precedence: list Sender: "Minios-devel" X-Inumbo-ID: 6fbc0b51-5082-11ec-976b-d102b41d0961 X-Virus-Scanned: Debian amavisd-new at aquilenet.fr Date: Sun, 28 Nov 2021 20:36:05 +0100 From: Samuel Thibault To: Juergen Gross Cc: minios-devel@lists.xenproject.org, xen-devel@lists.xenproject.org, wl@xen.org Subject: Re: [PATCH v4] xenbus: support large messages Message-ID: <20211128193605.z4nr2uwqx2pvpgvo@begin> Mail-Followup-To: Samuel Thibault , Juergen Gross , minios-devel@lists.xenproject.org, xen-devel@lists.xenproject.org, wl@xen.org References: <20211004094000.29868-1-jgross@suse.com> <244e6bc7-9158-7c52-13ab-d25daafbc2c8@suse.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <244e6bc7-9158-7c52-13ab-d25daafbc2c8@suse.com> Organization: I am not organized User-Agent: NeoMutt/20170609 (1.8.3) X-Spamd-Bar: / Authentication-Results: hera.aquilenet.fr; none X-Rspamd-Server: hera X-Rspamd-Queue-Id: 86CF028D X-Spamd-Result: default: False [0.40 / 15.00]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; TO_DN_SOME(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; MIME_GOOD(-0.10)[text/plain]; HAS_ORG_HEADER(0.00)[]; RCVD_COUNT_THREE(0.00)[3]; FROM_EQ_ENVFROM(0.00)[]; MIME_TRACE(0.00)[0:+]; RCVD_TLS_LAST(0.00)[]; MID_RHS_NOT_FQDN(0.50)[] Hello, Sorry, it seems I missed that mail :/ Added my tag below. BTW, I didn't see the mb() fix between rsp_cons+= and reading rsp_prod on the Linux side? Samuel Juergen Gross, le mer. 24 nov. 2021 08:00:55 +0100, a ecrit: > Ping? > > On 04.10.21 11:40, Juergen Gross wrote: > > Today the implementation of the xenbus protocol in Mini-OS will only > > allow to transfer the complete message to or from the ring page buffer. > > This is limiting the maximum message size to lower values as the xenbus > > protocol normally would allow. > > > > Change that by allowing to transfer the xenbus message in chunks as > > soon as they are available. > > > > Avoid crashing Mini-OS in case of illegal data read from the ring > > buffer. > > > > Signed-off-by: Juergen Gross Reviewed-by: Samuel Thibault > > --- > > V2: > > - drop redundant if (Samuel Thibault) > > - move rmb() (Samuel Thibault) > > V3: > > - correct notification test (Samuel Thibault) > > V4: > > - more memory barriers (Samuel Thibault) > > --- > > xenbus/xenbus.c | 210 ++++++++++++++++++++++++++++-------------------- > > 1 file changed, 122 insertions(+), 88 deletions(-) > > > > diff --git a/xenbus/xenbus.c b/xenbus/xenbus.c > > index 23de61e..b687678 100644 > > --- a/xenbus/xenbus.c > > +++ b/xenbus/xenbus.c > > @@ -29,6 +29,7 @@ > > #include > > #include > > #include > > +#include > > #define min(x,y) ({ \ > > typeof(x) tmpx = (x); \ > > @@ -46,6 +47,7 @@ > > static struct xenstore_domain_interface *xenstore_buf; > > static DECLARE_WAIT_QUEUE_HEAD(xb_waitq); > > DECLARE_WAIT_QUEUE_HEAD(xenbus_watch_queue); > > +static __DECLARE_SEMAPHORE_GENERIC(xb_write_sem, 1); > > xenbus_event_queue xenbus_events; > > static struct watch { > > @@ -231,75 +233,103 @@ char *xenbus_wait_for_state_change(const char* path, XenbusState *state, xenbus_ > > } > > +static void xenbus_read_data(char *buf, unsigned int len) > > +{ > > + unsigned int off = 0; > > + unsigned int prod, cons; > > + unsigned int size; > > + > > + while (off != len) > > + { > > + wait_event(xb_waitq, xenstore_buf->rsp_prod != xenstore_buf->rsp_cons); > > + > > + prod = xenstore_buf->rsp_prod; > > + cons = xenstore_buf->rsp_cons; > > + DEBUG("Rsp_cons %d, rsp_prod %d.\n", cons, prod); > > + size = min(len - off, prod - cons); > > + > > + rmb(); /* Make sure data read from ring is ordered with rsp_prod. */ > > + memcpy_from_ring(xenstore_buf->rsp, buf + off, > > + MASK_XENSTORE_IDX(cons), size); > > + off += size; > > + mb(); /* memcpy() and rsp_cons update must not be reordered. */ > > + xenstore_buf->rsp_cons += size; > > + mb(); /* rsp_cons must be visible before we look at rsp_prod. */ > > + if (xenstore_buf->rsp_prod - cons >= XENSTORE_RING_SIZE) > > + notify_remote_via_evtchn(xenbus_evtchn); > > + } > > +} > > + > > static void xenbus_thread_func(void *ign) > > { > > struct xsd_sockmsg msg; > > - unsigned prod = xenstore_buf->rsp_prod; > > + char *data; > > for (;;) { > > - wait_event(xb_waitq, prod != xenstore_buf->rsp_prod); > > - while (1) { > > - prod = xenstore_buf->rsp_prod; > > - DEBUG("Rsp_cons %d, rsp_prod %d.\n", xenstore_buf->rsp_cons, > > - xenstore_buf->rsp_prod); > > - if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < sizeof(msg)) > > - break; > > - rmb(); > > - memcpy_from_ring(xenstore_buf->rsp, &msg, > > - MASK_XENSTORE_IDX(xenstore_buf->rsp_cons), > > - sizeof(msg)); > > - DEBUG("Msg len %d, %d avail, id %d.\n", msg.len + sizeof(msg), > > - xenstore_buf->rsp_prod - xenstore_buf->rsp_cons, msg.req_id); > > - > > - if (xenstore_buf->rsp_prod - xenstore_buf->rsp_cons < > > - sizeof(msg) + msg.len) > > - break; > > - > > - DEBUG("Message is good.\n"); > > - > > - if (msg.type == XS_WATCH_EVENT) { > > - struct xenbus_event *event = malloc(sizeof(*event) + msg.len); > > - xenbus_event_queue *events = NULL; > > - char *data = (char*)event + sizeof(*event); > > - struct watch *watch; > > - > > - memcpy_from_ring(xenstore_buf->rsp, data, > > - MASK_XENSTORE_IDX(xenstore_buf->rsp_cons + sizeof(msg)), > > - msg.len); > > - > > - event->path = data; > > - event->token = event->path + strlen(event->path) + 1; > > - > > - mb(); > > - xenstore_buf->rsp_cons += msg.len + sizeof(msg); > > - > > - for (watch = watches; watch; watch = watch->next) > > - if (!strcmp(watch->token, event->token)) { > > - events = watch->events; > > - break; > > - } > > - > > - if (events) { > > - event->next = *events; > > - *events = event; > > - wake_up(&xenbus_watch_queue); > > - } else { > > - printk("unexpected watch token %s\n", event->token); > > - free(event); > > + xenbus_read_data((char *)&msg, sizeof(msg)); > > + DEBUG("Msg len %d, %d avail, id %d.\n", msg.len + sizeof(msg), > > + xenstore_buf->rsp_prod - xenstore_buf->rsp_cons, msg.req_id); > > + > > + if (msg.len > XENSTORE_PAYLOAD_MAX) { > > + printk("Xenstore violates protocol, message longer than allowed.\n"); > > + return; > > + } > > + > > + if (msg.type == XS_WATCH_EVENT) { > > + struct xenbus_event *event = malloc(sizeof(*event) + msg.len); > > + xenbus_event_queue *events = NULL; > > + struct watch *watch; > > + char *c; > > + int zeroes = 0; > > + > > + data = (char*)event + sizeof(*event); > > + xenbus_read_data(data, msg.len); > > + > > + for (c = data; c < data + msg.len; c++) > > + if (!*c) > > + zeroes++; > > + if (zeroes != 2) { > > + printk("Xenstore: illegal watch event data\n"); > > + free(event); > > + continue; > > + } > > + > > + event->path = data; > > + event->token = event->path + strlen(event->path) + 1; > > + > > + for (watch = watches; watch; watch = watch->next) > > + if (!strcmp(watch->token, event->token)) { > > + events = watch->events; > > + break; > > } > > + > > + if (events) { > > + event->next = *events; > > + *events = event; > > + wake_up(&xenbus_watch_queue); > > } else { > > - req_info[msg.req_id].reply = malloc(sizeof(msg) + msg.len); > > - memcpy_from_ring(xenstore_buf->rsp, req_info[msg.req_id].reply, > > - MASK_XENSTORE_IDX(xenstore_buf->rsp_cons), > > - msg.len + sizeof(msg)); > > - mb(); > > - xenstore_buf->rsp_cons += msg.len + sizeof(msg); > > - wake_up(&req_info[msg.req_id].waitq); > > + printk("Xenstore: unexpected watch token %s\n", event->token); > > + free(event); > > } > > - wmb(); > > - notify_remote_via_evtchn(xenbus_evtchn); > > + continue; > > } > > + > > + data = malloc(sizeof(msg) + msg.len); > > + memcpy(data, &msg, sizeof(msg)); > > + xenbus_read_data(data + sizeof(msg), msg.len); > > + > > + if (msg.req_id >= NR_REQS || !req_info[msg.req_id].in_use) { > > + printk("Xenstore: illegal request id %d\n", msg.req_id); > > + free(data); > > + continue; > > + } > > + > > + DEBUG("Message is good.\n"); > > + > > + req_info[msg.req_id].reply = data; > > + > > + wake_up(&req_info[msg.req_id].waitq); > > } > > } > > @@ -451,36 +481,40 @@ static void xb_write(int type, int req_id, xenbus_transaction_t trans_id, > > cur_req = &header_req; > > - BUG_ON(len > XENSTORE_RING_SIZE); > > - /* Wait for the ring to drain to the point where we can send the > > - message. */ > > - prod = xenstore_buf->req_prod; > > - if (prod + len - xenstore_buf->req_cons > XENSTORE_RING_SIZE) > > - { > > - /* Wait for there to be space on the ring */ > > - DEBUG("prod %d, len %d, cons %d, size %d; waiting.\n", > > - prod, len, xenstore_buf->req_cons, XENSTORE_RING_SIZE); > > - wait_event(xb_waitq, > > - xenstore_buf->req_prod + len - xenstore_buf->req_cons <= > > - XENSTORE_RING_SIZE); > > - DEBUG("Back from wait.\n"); > > - prod = xenstore_buf->req_prod; > > - } > > + BUG_ON(len > XENSTORE_PAYLOAD_MAX); > > + > > + /* Make sure we are the only thread trying to write. */ > > + down(&xb_write_sem); > > - /* We're now guaranteed to be able to send the message without > > - overflowing the ring. Do so. */ > > + /* Send the message in chunks using free ring space when available. */ > > total_off = 0; > > req_off = 0; > > - while (total_off < len) > > + while (total_off < len) > > { > > + prod = xenstore_buf->req_prod; > > + if (prod - xenstore_buf->req_cons >= XENSTORE_RING_SIZE) > > + { > > + /* Send evtchn to notify remote */ > > + notify_remote_via_evtchn(xenbus_evtchn); > > + > > + /* Wait for there to be space on the ring */ > > + DEBUG("prod %d, len %d, cons %d, size %d; waiting.\n", prod, > > + len - total_off, xenstore_buf->req_cons, XENSTORE_RING_SIZE); > > + wait_event(xb_waitq, > > + prod - xenstore_buf->req_cons < XENSTORE_RING_SIZE); > > + DEBUG("Back from wait.\n"); > > + } > > + > > this_chunk = min(cur_req->len - req_off, > > - XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod)); > > + XENSTORE_RING_SIZE - MASK_XENSTORE_IDX(prod)); > > + this_chunk = min(this_chunk, > > + xenstore_buf->req_cons + XENSTORE_RING_SIZE - prod); > > memcpy((char *)xenstore_buf->req + MASK_XENSTORE_IDX(prod), > > - (char *)cur_req->data + req_off, this_chunk); > > + (char *)cur_req->data + req_off, this_chunk); > > prod += this_chunk; > > req_off += this_chunk; > > total_off += this_chunk; > > - if (req_off == cur_req->len) > > + if (req_off == cur_req->len) > > { > > req_off = 0; > > if (cur_req == &header_req) > > @@ -488,20 +522,20 @@ static void xb_write(int type, int req_id, xenbus_transaction_t trans_id, > > else > > cur_req++; > > } > > + > > + /* Remote must see entire message before updating indexes */ > > + wmb(); > > + xenstore_buf->req_prod = prod; > > } > > + /* Send evtchn to notify remote */ > > + notify_remote_via_evtchn(xenbus_evtchn); > > + > > DEBUG("Complete main loop of xb_write.\n"); > > BUG_ON(req_off != 0); > > BUG_ON(total_off != len); > > - BUG_ON(prod > xenstore_buf->req_cons + XENSTORE_RING_SIZE); > > - /* Remote must see entire message before updating indexes */ > > - wmb(); > > - > > - xenstore_buf->req_prod += len; > > - > > - /* Send evtchn to notify remote */ > > - notify_remote_via_evtchn(xenbus_evtchn); > > + up(&xb_write_sem); > > } > > /* Send a mesasge to xenbus, in the same fashion as xb_write, and > > >