svn commit: r269814 - head/sys/dev/xen/blkfront

[Roger Pau Monné]

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.

Roger.
---
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);