From owner-svn-src-all@FreeBSD.ORG Wed Sep 3 15:48:58 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id B00F0EBE; Wed, 3 Sep 2014 15:48:58 +0000 (UTC) Received: from mail-wi0-x232.google.com (mail-wi0-x232.google.com [IPv6:2a00:1450:400c:c05::232]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 558351C86; Wed, 3 Sep 2014 15:48:57 +0000 (UTC) Received: by mail-wi0-f178.google.com with SMTP id r20so10047422wiv.11 for ; Wed, 03 Sep 2014 08:48:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:message-id:date:from:user-agent:mime-version:to:cc:subject :references:in-reply-to:content-type:content-transfer-encoding; bh=XSA/dVEeGGg1FOnnS/kc4TprmChoekUeqRZnW7a9ybI=; b=t4kq5RxubVbsKMD1nakwnvIVFBKBdI1mD8NRR5eMfFZ+3AzSf6OTr1PZwzlhhb3Rfo Q7zelEbkFPvFM2cXXK7O4aB9bVrhTbtYq/5UYnSHbG7cdEzZyMhl4BSycBpvwkK6WKhc IGdgZOKFYrZ+/T6QX/5cLpTymxPlho1f6cx6lZoHqYPWg1984jKTj7Uj8sWXSvOt5A5p j9+2CTKoNGI0fB+PNeW8i4SxFPg1HhC5EkQsJgrKsVtaWy5P8BE3Y5CA+Xqnkyh6YeUJ HQyRigYLwkx4fzsEIs8BbGII1Fq6MNWQqq9Yk6xJMR13RgQGyY2pzVne0VhM/M1gazOL x5BA== X-Received: by 10.194.58.83 with SMTP id o19mr49103998wjq.20.1409759332963; Wed, 03 Sep 2014 08:48:52 -0700 (PDT) Received: from [172.16.1.30] (124.Red-83-33-238.dynamicIP.rima-tde.net. [83.33.238.124]) by mx.google.com with ESMTPSA id w10sm5403760wif.15.2014.09.03.08.48.51 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 03 Sep 2014 08:48:52 -0700 (PDT) Sender: =?UTF-8?Q?Roger_Pau_Monn=C3=A9?= Message-ID: <5407385B.1000005@FreeBSD.org> Date: Wed, 03 Sep 2014 17:48:43 +0200 From: =?windows-1252?Q?Roger_Pau_Monn=E9?= User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:31.0) Gecko/20100101 Thunderbird/31.0 MIME-Version: 1.0 To: John-Mark Gurney Subject: Re: svn commit: r269814 - head/sys/dev/xen/blkfront References: <53e8e31e.2179.30c1c657@svn.freebsd.org> <53FF7386.3050804@FreeBSD.org> <20140828184515.GV71691@funkthat.com> <53FF7BC4.6050801@FreeBSD.org> <5400BDC7.7020902@FreeBSD.org> <54058E1E.4050907@FreeBSD.org> <20140902171841.GX71691@funkthat.com> In-Reply-To: <20140902171841.GX71691@funkthat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: src-committers@FreeBSD.org, Alexander Motin , scottl@FreeBSD.org, cperciva@FreeBSD.org, svn-src-head@FreeBSD.org, gibbs@freebsd.org, svn-src-all@FreeBSD.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Sep 2014 15:48:58 -0000 El 02/09/14 a les 19.18, John-Mark Gurney ha escrit: > Roger Pau Monn wrote this message on Tue, Sep 02, 2014 at 11:30 +0200: >> El 29/08/14 a les 19.52, Roger Pau Monné ha escrit: >>> El 28/08/14 a les 20.58, Alexander Motin ha escrit: >>>> On 28.08.2014 21:45, John-Mark Gurney wrote: >>>>> Alexander Motin wrote this message on Thu, Aug 28, 2014 at 21:23 +0300: >>>>>> Hi, Roger. >>>>>> >>>>>> It looks to me like this commit does not work as it should. I got >>>>>> problem when I just tried `newfs /dev/ada0 ; mount /dev/ada0 /mnt`. >>>>>> Somehow newfs does not produce valid filesystem. Problem is reliably >>>>>> repeatable and reverting this commit fixes it. >>>>>> >>>>>> I found at least one possible cause there: If original data buffer is >>>>>> unmapped, misaligned and not physically contiguous, then present x86 >>>>>> bus_dmamap_load_bio() implementation will process each physically >>>>>> contiguous segment separately. Due to the misalignment first and last >>>>>> physical segments may have size not multiple to 512 bytes. Since each >>>>>> segment processed separately, they are not joined together, and >>>>>> xbd_queue_cb() is getting segments not multiple to 512 bytes. Attempt to >>>>>> convert them to exact number of sectors in the driver cause data corruption. >>>>> >>>>> Are you sure this isn't a problem w/ the tag not properly specifying >>>>> the correct alignement? >>>> >>>> I don't know how to specify it stronger then this: >>>> error = bus_dma_tag_create( >>>> bus_get_dma_tag(sc->xbd_dev), /* parent */ >>>> 512, PAGE_SIZE, /* algnmnt, boundary */ >>>> BUS_SPACE_MAXADDR, /* lowaddr */ >>>> BUS_SPACE_MAXADDR, /* highaddr */ >>>> NULL, NULL, /* filter, filterarg */ >>>> sc->xbd_max_request_size, >>>> sc->xbd_max_request_segments, >>>> PAGE_SIZE, /* maxsegsize */ >>>> BUS_DMA_ALLOCNOW, /* flags */ >>>> busdma_lock_mutex, /* lockfunc */ >>>> &sc->xbd_io_lock, /* lockarg */ >>>> &sc->xbd_io_dmat); >>>> >>>>> Also, I don't think there is a way for busdma >>>>> to say that you MUST have a segment be a multiple of 512, though you >>>>> could use a 512 boundary, but that would force all segments to only be >>>>> 512 bytes... >>>> >>>> As I understand, that is mandatory requirement for this "hardware". >>>> Alike 4K alignment requirement also exist at least for SDHCI, and IIRC >>>> UHCI/OHCI hardware. Even AHCI requires both segment addresses and >>>> lengths to be even. >>>> >>>> I may be wrong, but I think it is quite likely that hardware that >>>> requires segment address alignment quite likely will have the same >>>> requirements for segments length. >> >> Hello, >> >> I have the following fix, which makes sure the total length and the >> size of each segment is aligned. I'm not very knowledgeable of the >> busdma code, so someone has to review it. > > I feel that this alignment should only be enforced via a new option on > the tag... I don't see how alignment and segment size should be > conflated... I could totally see a device that requires an alignement > of 8 bytes, but has a segment size of 16, or vice versa, and requiring > them to be the same means we will bounce unnecesarily... > > cc'd scottl since he knows this code better than I... and cperciva as > he touched it for similar reasons.. > > Oh, I just found PR 152818, where cperciva did a similar fix to > bounce_bus_dmamap_load_buffer for the exact same reason... It was > committed in r216194... Since Xen blkfront seems to be the only driver to have such segment size requirements, it might be best to just fix blkfront to always roundup segment size to 512, like the following: diff --git a/sys/dev/xen/blkfront/blkfront.c b/sys/dev/xen/blkfront/blkfront.c index 26b8f09..2d284d9 100644 --- a/sys/dev/xen/blkfront/blkfront.c +++ b/sys/dev/xen/blkfront/blkfront.c @@ -209,7 +209,8 @@ xbd_queue_cb(void *arg, bus_dma_segment_t *segs, int nsegs, int error) buffer_ma = segs->ds_addr; fsect = (buffer_ma & PAGE_MASK) >> XBD_SECTOR_SHFT; - lsect = fsect + (segs->ds_len >> XBD_SECTOR_SHFT) - 1; + lsect = fsect + (roundup2(segs->ds_len, 512) + >> XBD_SECTOR_SHFT) - 1; KASSERT(lsect <= 7, ("XEN disk driver data cannot " "cross a page boundary")); >> --- >> diff --git a/sys/x86/x86/busdma_bounce.c b/sys/x86/x86/busdma_bounce.c >> index d1c75f8..688f559 100644 >> --- a/sys/x86/x86/busdma_bounce.c >> +++ b/sys/x86/x86/busdma_bounce.c >> @@ -620,6 +620,8 @@ bounce_bus_dmamap_load_phys(bus_dma_tag_t dmat, bus_dmamap_t map, >> segs = dmat->segments; >> >> if ((dmat->bounce_flags & BUS_DMA_COULD_BOUNCE) != 0) { >> + /* Make sure buflen is aligned */ >> + buflen = roundup2(buflen, dmat->common.alignment); >> _bus_dmamap_count_phys(dmat, map, buf, buflen, flags); >> if (map->pagesneeded != 0) { >> error = _bus_dmamap_reserve_pages(dmat, map, flags); >> @@ -634,6 +636,7 @@ bounce_bus_dmamap_load_phys(bus_dma_tag_t dmat, bus_dmamap_t map, >> if (((dmat->bounce_flags & BUS_DMA_COULD_BOUNCE) != 0) && >> map->pagesneeded != 0 && >> bus_dma_run_filter(&dmat->common, curaddr)) { >> + sgsize = roundup2(sgsize, dmat->common.alignment); >> sgsize = MIN(sgsize, PAGE_SIZE); >> curaddr = add_bounce_page(dmat, map, 0, curaddr, >> sgsize); >> > > Doesn't this same change need to be made to _bus_dmamap_count_phys so that > the cound will be correct? We already roundup the buflen to the alignement value before calling _bus_dmamap_count_phys, so every segment should have a size that's a multiple of alignement. > Also, make sure you review the other arch's bounce implementations, as > they were often copied from the x86 one... >