From owner-svn-src-stable@freebsd.org Thu Jan 5 05:24:31 2017 Return-Path: Delivered-To: svn-src-stable@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id A4893C9F0DE; Thu, 5 Jan 2017 05:24:31 +0000 (UTC) (envelope-from sephe@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 6D20A1680; Thu, 5 Jan 2017 05:24:31 +0000 (UTC) (envelope-from sephe@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id v055OUYO098751; Thu, 5 Jan 2017 05:24:30 GMT (envelope-from sephe@FreeBSD.org) Received: (from sephe@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id v055OUTF098749; Thu, 5 Jan 2017 05:24:30 GMT (envelope-from sephe@FreeBSD.org) Message-Id: <201701050524.v055OUTF098749@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: sephe set sender to sephe@FreeBSD.org using -f From: Sepherosa Ziehau Date: Thu, 5 Jan 2017 05:24:30 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r311364 - in stable/11/sys/dev/hyperv: include vmbus X-SVN-Group: stable-11 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-stable@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for all the -stable branches of the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jan 2017 05:24:31 -0000 Author: sephe Date: Thu Jan 5 05:24:30 2017 New Revision: 311364 URL: https://svnweb.freebsd.org/changeset/base/311364 Log: MFC 309128,309129,309131-309136,309138-309140,309224,309225 309128 hyperv/vmbus: Commit the GPADL id only after the connection succeeds. Minor style change. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8563 309129 hyperv/vmbus: Minor style changes. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8564 309131 hyperv/vmbus: Fix sysctl tree leakage, if channel open fails. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8565 309132 hyperv/vmbus: Don't close unopened channels. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8566 309133 hyperv/vmbus: GPADL disconnect error on a revoked channel is benign. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8567 309134 hyperv/vmbus: No stranded bufring GPADL is allowed. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8568 309135 hyperv/vmbus: Return EISCONN if the bufring GPADL can't be disconnected. So that the callers of vmbus_chan_open_br() could handle the passed in bufring memory properly. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8569 309136 hyperv/vmbus: Don't free the bufring if its GPADL can't be disconnected. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8570 309138 hyperv/vmbus: Always try disconnect/free bufring memory upon channel close While I'm here, minor wording and style changes. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8598 309139 hyperv/vmbus: Propagate close error. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8599 309140 hyperv/vmbus: Add a simplified version of channel close. So that the caller can know the channel close error and react accordingly. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8600 309224 hyperv/vmbus: Zero out GPADL if error happens. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8601 309225 hyperv/vmbus: Add supportive transaction wait function. This function supports channel revocation properly. Sponsored by: Microsoft Differential Revision: https://reviews.freebsd.org/D8611 Modified: stable/11/sys/dev/hyperv/include/vmbus.h stable/11/sys/dev/hyperv/vmbus/vmbus_chan.c Directory Properties: stable/11/ (props changed) Modified: stable/11/sys/dev/hyperv/include/vmbus.h ============================================================================== --- stable/11/sys/dev/hyperv/include/vmbus.h Thu Jan 5 05:03:11 2017 (r311363) +++ stable/11/sys/dev/hyperv/include/vmbus.h Thu Jan 5 05:24:30 2017 (r311364) @@ -116,6 +116,7 @@ struct vmbus_chan_br { }; struct vmbus_channel; +struct vmbus_xact; struct vmbus_xact_ctx; struct hyperv_guid; struct task; @@ -129,6 +130,36 @@ vmbus_get_channel(device_t dev) return device_get_ivars(dev); } +/* + * vmbus_chan_open_br() + * + * Return values: + * 0 Succeeded. + * EISCONN Failed, and the memory passed through 'br' is still + * connected. Callers must _not_ free the the memory + * passed through 'br', if this error happens. + * other values Failed. The memory passed through 'br' is no longer + * connected. Callers are free to do anything with the + * memory passed through 'br'. + * + * + * + * vmbus_chan_close_direct() + * + * NOTE: + * Callers of this function _must_ make sure to close all sub-channels before + * closing the primary channel. + * + * Return values: + * 0 Succeeded. + * EISCONN Failed, and the memory associated with the bufring + * is still connected. Callers must _not_ free the the + * memory associated with the bufring, if this error + * happens. + * other values Failed. The memory associated with the bufring is + * no longer connected. Callers are free to do anything + * with the memory associated with the bufring. + */ int vmbus_chan_open(struct vmbus_channel *chan, int txbr_size, int rxbr_size, const void *udata, int udlen, vmbus_chan_callback_t cb, void *cbarg); @@ -136,12 +167,15 @@ int vmbus_chan_open_br(struct vmbus_cha const struct vmbus_chan_br *cbr, const void *udata, int udlen, vmbus_chan_callback_t cb, void *cbarg); void vmbus_chan_close(struct vmbus_channel *chan); +int vmbus_chan_close_direct(struct vmbus_channel *chan); void vmbus_chan_intr_drain(struct vmbus_channel *chan); void vmbus_chan_run_task(struct vmbus_channel *chan, struct task *task); void vmbus_chan_set_orphan(struct vmbus_channel *chan, struct vmbus_xact_ctx *); void vmbus_chan_unset_orphan(struct vmbus_channel *chan); +const void *vmbus_chan_xact_wait(const struct vmbus_channel *chan, + struct vmbus_xact *xact, size_t *resp_len, bool can_sleep); int vmbus_chan_gpadl_connect(struct vmbus_channel *chan, bus_addr_t paddr, int size, uint32_t *gpadl); Modified: stable/11/sys/dev/hyperv/vmbus/vmbus_chan.c ============================================================================== --- stable/11/sys/dev/hyperv/vmbus/vmbus_chan.c Thu Jan 5 05:03:11 2017 (r311363) +++ stable/11/sys/dev/hyperv/vmbus/vmbus_chan.c Thu Jan 5 05:24:30 2017 (r311364) @@ -53,7 +53,7 @@ __FBSDID("$FreeBSD$"); static void vmbus_chan_update_evtflagcnt( struct vmbus_softc *, const struct vmbus_channel *); -static void vmbus_chan_close_internal( +static int vmbus_chan_close_internal( struct vmbus_channel *); static int vmbus_chan_sysctl_mnf(SYSCTL_HANDLER_ARGS); static void vmbus_chan_sysctl_create( @@ -66,6 +66,8 @@ static int vmbus_chan_release(struct v static void vmbus_chan_set_chmap(struct vmbus_channel *); static void vmbus_chan_clear_chmap(struct vmbus_channel *); static void vmbus_chan_detach(struct vmbus_channel *); +static bool vmbus_chan_wait_revoke( + const struct vmbus_channel *); static void vmbus_chan_ins_prilist(struct vmbus_softc *, struct vmbus_channel *); @@ -322,7 +324,21 @@ vmbus_chan_open(struct vmbus_channel *ch error = vmbus_chan_open_br(chan, &cbr, udata, udlen, cb, cbarg); if (error) { - hyperv_dmamem_free(&chan->ch_bufring_dma, chan->ch_bufring); + if (error == EISCONN) { + /* + * XXX + * The bufring GPADL is still connected; abandon + * this bufring, instead of having mysterious + * crash or trashed data later on. + */ + vmbus_chan_printf(chan, "chan%u bufring GPADL " + "is still connected upon channel open error; " + "leak %d bytes memory\n", chan->ch_id, + txbr_size + rxbr_size); + } else { + hyperv_dmamem_free(&chan->ch_bufring_dma, + chan->ch_bufring); + } chan->ch_bufring = NULL; } return (error); @@ -345,7 +361,7 @@ vmbus_chan_open_br(struct vmbus_channel if (udlen > VMBUS_CHANMSG_CHOPEN_UDATA_SIZE) { vmbus_chan_printf(chan, "invalid udata len %d for chan%u\n", udlen, chan->ch_id); - return EINVAL; + return (EINVAL); } br = cbr->cbr; @@ -390,6 +406,8 @@ vmbus_chan_open_br(struct vmbus_channel /* * Connect the bufrings, both RX and TX, to this channel. */ + KASSERT(chan->ch_bufring_gpadl == 0, + ("bufring GPADL is still connected")); error = vmbus_chan_gpadl_connect(chan, cbr->cbr_paddr, txbr_size + rxbr_size, &chan->ch_bufring_gpadl); if (error) { @@ -442,23 +460,33 @@ vmbus_chan_open_br(struct vmbus_channel vmbus_msghc_put(sc, mh); if (status == 0) { - if (bootverbose) { + if (bootverbose) vmbus_chan_printf(chan, "chan%u opened\n", chan->ch_id); - } - return 0; + return (0); } vmbus_chan_printf(chan, "failed to open chan%u\n", chan->ch_id); error = ENXIO; failed: + sysctl_ctx_free(&chan->ch_sysctl_ctx); vmbus_chan_clear_chmap(chan); - if (chan->ch_bufring_gpadl) { - vmbus_chan_gpadl_disconnect(chan, chan->ch_bufring_gpadl); + if (chan->ch_bufring_gpadl != 0) { + int error1; + + error1 = vmbus_chan_gpadl_disconnect(chan, + chan->ch_bufring_gpadl); + if (error1) { + /* + * Give caller a hint that the bufring GPADL is still + * connected. + */ + error = EISCONN; + } chan->ch_bufring_gpadl = 0; } atomic_clear_int(&chan->ch_stflags, VMBUS_CHAN_ST_OPENED); - return error; + return (error); } int @@ -475,6 +503,12 @@ vmbus_chan_gpadl_connect(struct vmbus_ch uint64_t page_id; /* + * Reset GPADL, so that the result would consistent, if error + * happened later on. + */ + *gpadl0 = 0; + + /* * Preliminary checks. */ @@ -500,7 +534,6 @@ vmbus_chan_gpadl_connect(struct vmbus_ch * Allocate GPADL id. */ gpadl = vmbus_gpadl_alloc(sc); - *gpadl0 = gpadl; /* * Connect this GPADL to the target channel. @@ -579,15 +612,35 @@ vmbus_chan_gpadl_connect(struct vmbus_ch vmbus_chan_printf(chan, "gpadl_conn(chan%u) failed: %u\n", chan->ch_id, status); return EIO; - } else { - if (bootverbose) { - vmbus_chan_printf(chan, - "gpadl_conn(chan%u) succeeded\n", chan->ch_id); - } + } + + /* Done; commit the GPADL id. */ + *gpadl0 = gpadl; + if (bootverbose) { + vmbus_chan_printf(chan, "gpadl_conn(chan%u) succeeded\n", + chan->ch_id); } return 0; } +static bool +vmbus_chan_wait_revoke(const struct vmbus_channel *chan) +{ +#define WAIT_COUNT 200 /* 200ms */ + + int i; + + for (i = 0; i < WAIT_COUNT; ++i) { + if (vmbus_chan_is_revoked(chan)) + return (true); + /* Not sure about the context; use busy-wait. */ + DELAY(1000); + } + return (false); + +#undef WAIT_COUNT +} + /* * Disconnect the GPA from the target channel */ @@ -604,7 +657,7 @@ vmbus_chan_gpadl_disconnect(struct vmbus vmbus_chan_printf(chan, "can not get msg hypercall for gpadl_disconn(chan%u)\n", chan->ch_id); - return EBUSY; + return (EBUSY); } req = vmbus_msghc_dataptr(mh); @@ -614,18 +667,29 @@ vmbus_chan_gpadl_disconnect(struct vmbus error = vmbus_msghc_exec(sc, mh); if (error) { + vmbus_msghc_put(sc, mh); + + if (vmbus_chan_wait_revoke(chan)) { + /* + * Error is benign; this channel is revoked, + * so this GPADL will not be touched anymore. + */ + vmbus_chan_printf(chan, + "gpadl_disconn(revoked chan%u) msg hypercall " + "exec failed: %d\n", chan->ch_id, error); + return (0); + } vmbus_chan_printf(chan, "gpadl_disconn(chan%u) msg hypercall exec failed: %d\n", chan->ch_id, error); - vmbus_msghc_put(sc, mh); - return error; + return (error); } vmbus_msghc_wait_result(sc, mh); /* Discard result; no useful information */ vmbus_msghc_put(sc, mh); - return 0; + return (0); } static void @@ -681,16 +745,34 @@ vmbus_chan_set_chmap(struct vmbus_channe chan->ch_vmbus->vmbus_chmap[chan->ch_id] = chan; } -static void +static int vmbus_chan_close_internal(struct vmbus_channel *chan) { struct vmbus_softc *sc = chan->ch_vmbus; struct vmbus_msghc *mh; struct vmbus_chanmsg_chclose *req; + uint32_t old_stflags; int error; - /* TODO: stringent check */ - atomic_clear_int(&chan->ch_stflags, VMBUS_CHAN_ST_OPENED); + /* + * NOTE: + * Sub-channels are closed upon their primary channel closing, + * so they can be closed even before they are opened. + */ + for (;;) { + old_stflags = chan->ch_stflags; + if (atomic_cmpset_int(&chan->ch_stflags, old_stflags, + old_stflags & ~VMBUS_CHAN_ST_OPENED)) + break; + } + if ((old_stflags & VMBUS_CHAN_ST_OPENED) == 0) { + /* Not opened yet; done */ + if (bootverbose) { + vmbus_chan_printf(chan, "chan%u not opened\n", + chan->ch_id); + } + return (0); + } /* * Free this channel's sysctl tree attached to its device's @@ -716,7 +798,8 @@ vmbus_chan_close_internal(struct vmbus_c vmbus_chan_printf(chan, "can not get msg hypercall for chclose(chan%u)\n", chan->ch_id); - return; + error = ENXIO; + goto disconnect; } req = vmbus_msghc_dataptr(mh); @@ -730,16 +813,37 @@ vmbus_chan_close_internal(struct vmbus_c vmbus_chan_printf(chan, "chclose(chan%u) msg hypercall exec failed: %d\n", chan->ch_id, error); - return; - } else if (bootverbose) { - vmbus_chan_printf(chan, "close chan%u\n", chan->ch_id); + goto disconnect; } + if (bootverbose) + vmbus_chan_printf(chan, "chan%u closed\n", chan->ch_id); + +disconnect: /* * Disconnect the TX+RX bufrings from this channel. */ - if (chan->ch_bufring_gpadl) { - vmbus_chan_gpadl_disconnect(chan, chan->ch_bufring_gpadl); + if (chan->ch_bufring_gpadl != 0) { + int error1; + + error1 = vmbus_chan_gpadl_disconnect(chan, + chan->ch_bufring_gpadl); + if (error1) { + /* + * XXX + * The bufring GPADL is still connected; abandon + * this bufring, instead of having mysterious + * crash or trashed data later on. + */ + vmbus_chan_printf(chan, "chan%u bufring GPADL " + "is still connected after close\n", chan->ch_id); + chan->ch_bufring = NULL; + /* + * Give caller a hint that the bufring GPADL is + * still connected. + */ + error = EISCONN; + } chan->ch_bufring_gpadl = 0; } @@ -750,6 +854,42 @@ vmbus_chan_close_internal(struct vmbus_c hyperv_dmamem_free(&chan->ch_bufring_dma, chan->ch_bufring); chan->ch_bufring = NULL; } + return (error); +} + +int +vmbus_chan_close_direct(struct vmbus_channel *chan) +{ + int error; + +#ifdef INVARIANTS + if (VMBUS_CHAN_ISPRIMARY(chan)) { + struct vmbus_channel *subchan; + + /* + * All sub-channels _must_ have been closed, or are _not_ + * opened at all. + */ + mtx_lock(&chan->ch_subchan_lock); + TAILQ_FOREACH(subchan, &chan->ch_subchans, ch_sublink) { + KASSERT( + (subchan->ch_stflags & VMBUS_CHAN_ST_OPENED) == 0, + ("chan%u: subchan%u is still opened", + chan->ch_id, subchan->ch_subidx)); + } + mtx_unlock(&chan->ch_subchan_lock); + } +#endif + + error = vmbus_chan_close_internal(chan); + if (!VMBUS_CHAN_ISPRIMARY(chan)) { + /* + * This sub-channel is referenced, when it is linked to + * the primary channel; drop that reference now. + */ + vmbus_chan_detach(chan); + } + return (error); } /* @@ -1787,3 +1927,37 @@ vmbus_chan_unset_orphan(struct vmbus_cha chan->ch_orphan_xact = NULL; sx_xunlock(&chan->ch_orphan_lock); } + +const void * +vmbus_chan_xact_wait(const struct vmbus_channel *chan, + struct vmbus_xact *xact, size_t *resp_len, bool can_sleep) +{ + const void *ret; + + if (can_sleep) + ret = vmbus_xact_wait(xact, resp_len); + else + ret = vmbus_xact_busywait(xact, resp_len); + if (vmbus_chan_is_revoked(chan)) { + /* + * This xact probably is interrupted, and the + * interruption can race the reply reception, + * so we have to make sure that there are nothing + * left on the RX bufring, i.e. this xact will + * not be touched, once this function returns. + * + * Since the hypervisor will not put more data + * onto the RX bufring once the channel is revoked, + * the following loop will be terminated, once all + * data are drained by the driver's channel + * callback. + */ + while (!vmbus_chan_rx_empty(chan)) { + if (can_sleep) + pause("chxact", 1); + else + DELAY(1000); + } + } + return (ret); +}