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

[John-Mark Gurney]

John-Mark Gurney wrote this message on Tue, Sep 02, 2014 at 10:18 -0700:
> 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...

Also, if this change is made w/o adding a field to the tag, the busdma
man page needs to be updated w/ the fact that alignment also forces
segment sizes to be alignment sized...

cperciva forgot this when he did his commit...

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."