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

[Alexander Motin]

On 03.09.2014 18:48, Roger Pau Monné wrote:
> 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, 

No, it is not. I've already posted other examples I can recall: SDHCI,
UHCI/OHCI and AHCI. Their limitations are different and less strict, but
still may need handling. For SDHCI, since it is quite slow and has many
other bugs, I practically implemented custom buffer bouncing. AHCI I
suppose works only because limitation is only for even addresses, and
odd ones happen extremely rarely (does not happen). For USB I am not
sure, but at least umass driver does not support unmapped I/O.

> it might be best to just fix blkfront to always 
> roundup segment size to 512, like the following:

I think some coffee is needed here. ;) Rounding addresses won't make
data properly aligned. Some copy is unavoidable in such cases. It would
be good if it was done properly by default buffer bouncer.

> 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"));


-- 
Alexander Motin