From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39624) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoaHH-0004io-Sn for qemu-devel@nongnu.org; Wed, 12 Nov 2014 10:56:20 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XoaHD-0002Cs-0V for qemu-devel@nongnu.org; Wed, 12 Nov 2014 10:56:15 -0500 Received: from smtp.citrix.com ([66.165.176.89]:45008) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoaHC-0002Ae-PY for qemu-devel@nongnu.org; Wed, 12 Nov 2014 10:56:10 -0500 Message-ID: <54638307.1080500@eu.citrix.com> Date: Wed, 12 Nov 2014 15:55:51 +0000 From: George Dunlap MIME-Version: 1.0 References: <1415807116-8375-1-git-send-email-roger.pau@citrix.com> In-Reply-To: <1415807116-8375-1-git-send-email-roger.pau@citrix.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] xen_disk: fix unmapping of persistent grants List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roger Pau Monne , qemu-devel@nongnu.org, xen-devel@lists.xenproject.org Cc: Kevin Wolf , Konrad Rzeszutek Wilk , Stefan Hajnoczi , Stefano Stabellini On 11/12/2014 03:45 PM, Roger Pau Monne wrote: > This patch fixes two issues with persistent grants and the disk PV backend > (Qdisk): > > - Don't use batch mappings when using persistent grants, doing so prevents > unmapping single grants (the whole area has to be unmapped at once). > - Unmap persistent grants before switching to the closed state, so the > frontend can also free them. > > Signed-off-by: Roger Pau Monné > Reported-and-Tested-by: George Dunlap > Cc: Stefano Stabellini > Cc: Kevin Wolf > Cc: Stefan Hajnoczi > Cc: George Dunlap CC'ing Konrad and Stefano: This fixes a critical bug that should be a blocker for the Xen 4.5 release. Without this, any backend using qdisk for a PV guest with pygrub (including qcow2 and vhd) will crash dom0. -George > --- > hw/block/xen_disk.c | 35 ++++++++++++++++++++++++----------- > 1 file changed, 24 insertions(+), 11 deletions(-) > > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c > index 231e9a7..1300c0a 100644 > --- a/hw/block/xen_disk.c > +++ b/hw/block/xen_disk.c > @@ -43,8 +43,6 @@ > > /* ------------------------------------------------------------- */ > > -static int batch_maps = 0; > - > static int max_requests = 32; > > /* ------------------------------------------------------------- */ > @@ -105,6 +103,7 @@ struct XenBlkDev { > blkif_back_rings_t rings; > int more_work; > int cnt_map; > + bool batch_maps; > > /* request lists */ > QLIST_HEAD(inflight_head, ioreq) inflight; > @@ -309,7 +308,7 @@ static void ioreq_unmap(struct ioreq *ioreq) > if (ioreq->num_unmap == 0 || ioreq->mapped == 0) { > return; > } > - if (batch_maps) { > + if (ioreq->blkdev->batch_maps) { > if (!ioreq->pages) { > return; > } > @@ -386,7 +385,7 @@ static int ioreq_map(struct ioreq *ioreq) > new_maps = ioreq->v.niov; > } > > - if (batch_maps && new_maps) { > + if (ioreq->blkdev->batch_maps && new_maps) { > ioreq->pages = xc_gnttab_map_grant_refs > (gnt, new_maps, domids, refs, ioreq->prot); > if (ioreq->pages == NULL) { > @@ -433,7 +432,7 @@ static int ioreq_map(struct ioreq *ioreq) > */ > grant = g_malloc0(sizeof(*grant)); > new_maps--; > - if (batch_maps) { > + if (ioreq->blkdev->batch_maps) { > grant->page = ioreq->pages + (new_maps) * XC_PAGE_SIZE; > } else { > grant->page = ioreq->page[new_maps]; > @@ -718,7 +717,9 @@ static void blk_alloc(struct XenDevice *xendev) > QLIST_INIT(&blkdev->freelist); > blkdev->bh = qemu_bh_new(blk_bh, blkdev); > if (xen_mode != XEN_EMULATE) { > - batch_maps = 1; > + blkdev->batch_maps = TRUE; > + } else { > + blkdev->batch_maps = FALSE; > } > if (xc_gnttab_set_max_grants(xendev->gnttabdev, > MAX_GRANTS(max_requests, BLKIF_MAX_SEGMENTS_PER_REQUEST)) < 0) { > @@ -923,6 +924,13 @@ static int blk_connect(struct XenDevice *xendev) > } else { > blkdev->feature_persistent = !!pers; > } > + if (blkdev->feature_persistent) { > + /* > + * Disable batch maps, since that would prevent unmapping > + * single persistent grants. > + */ > + blkdev->batch_maps = FALSE; > + } > > blkdev->protocol = BLKIF_PROTOCOL_NATIVE; > if (blkdev->xendev.protocol) { > @@ -1000,6 +1008,16 @@ static void blk_disconnect(struct XenDevice *xendev) > blkdev->cnt_map--; > blkdev->sring = NULL; > } > + > + /* > + * Unmap persistent grants before switching to the closed state > + * so the frontend can free them. > + */ > + if (blkdev->feature_persistent) { > + g_tree_destroy(blkdev->persistent_gnts); > + assert(blkdev->persistent_gnt_count == 0); > + blkdev->feature_persistent = FALSE; > + } > } > > static int blk_free(struct XenDevice *xendev) > @@ -1011,11 +1029,6 @@ static int blk_free(struct XenDevice *xendev) > blk_disconnect(xendev); > } > > - /* Free persistent grants */ > - if (blkdev->feature_persistent) { > - g_tree_destroy(blkdev->persistent_gnts); > - } > - > while (!QLIST_EMPTY(&blkdev->freelist)) { > ioreq = QLIST_FIRST(&blkdev->freelist); > QLIST_REMOVE(ioreq, list);