From owner-freebsd-sparc64@FreeBSD.ORG Sun Jan 13 17:14:00 2013 Return-Path: Delivered-To: freebsd-sparc64@FreeBSD.org Received: from mx1.freebsd.org (mx1.FreeBSD.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 89F7C585; Sun, 13 Jan 2013 17:14:00 +0000 (UTC) (envelope-from marius@alchemy.franken.de) Received: from alchemy.franken.de (alchemy.franken.de [194.94.249.214]) by mx1.freebsd.org (Postfix) with ESMTP id 0C76CFC0; Sun, 13 Jan 2013 17:13:59 +0000 (UTC) Received: from alchemy.franken.de (localhost [127.0.0.1]) by alchemy.franken.de (8.14.5/8.14.5/ALCHEMY.FRANKEN.DE) with ESMTP id r0DHDwuS013593; Sun, 13 Jan 2013 18:13:58 +0100 (CET) (envelope-from marius@alchemy.franken.de) Received: (from marius@localhost) by alchemy.franken.de (8.14.5/8.14.5/Submit) id r0DHDwf4013592; Sun, 13 Jan 2013 18:13:58 +0100 (CET) (envelope-from marius) Date: Sun, 13 Jan 2013 18:13:58 +0100 From: Marius Strobl To: Alexander Motin Subject: Re: smartmontools panics 9.1-RELEASE on sunfire 240 Message-ID: <20130113171358.GL26039@alchemy.franken.de> References: <20130104051914.GA22613@pix.net> <20130104235336.GB37999@alchemy.franken.de> <20130105013224.GA31361@pix.net> <20130105015242.GB26039@alchemy.franken.de> <20130106021923.GE1410@funkthat.com> <20130106031245.GC26039@alchemy.franken.de> <50EBE936.4000806@FreeBSD.org> <20130113155516.GK26039@alchemy.franken.de> <50F2E57C.1090305@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50F2E57C.1090305@FreeBSD.org> User-Agent: Mutt/1.5.21 (2010-09-15) Cc: Kurt Lidl , freebsd-sparc64@FreeBSD.org X-BeenThere: freebsd-sparc64@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Porting FreeBSD to the Sparc List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 13 Jan 2013 17:14:00 -0000 On Sun, Jan 13, 2013 at 06:49:00PM +0200, Alexander Motin wrote: > On 13.01.2013 17:55, Marius Strobl wrote: > > On Tue, Jan 08, 2013 at 11:39:02AM +0200, Alexander Motin wrote: > >> On 06.01.2013 05:12, Marius Strobl wrote: > >>> On Sat, Jan 05, 2013 at 06:19:23PM -0800, John-Mark Gurney wrote: > >>>> Marius Strobl wrote this message on Sat, Jan 05, 2013 at 02:52 +0100: > >>>>> On Fri, Jan 04, 2013 at 08:32:24PM -0500, Kurt Lidl wrote: > >>>>>> On Sat, Jan 05, 2013 at 12:53:36AM +0100, Marius Strobl wrote: > >>>>>>> Uhm, probably an userland buffer which isn't even 16-bit aligned. > >>>>>>> If that's the cause, the attached patch hopefully should at least > >>>>>>> prevent the panic. If it does, smartmontools still need to be fixed > >>>>>>> though. > >>>>>> > >>>>>> You patch prevents the panic from happening. > >>>>>> When I try to start smartd now, it reports: > >>>>>> > >>>>>> root@host-98: /usr/local/etc/rc.d/smartd onestart > >>>>>> Starting smartd. > >>>>>> smartd: cam_send_ccb: Invalid argument > >>>>>> /usr/local/etc/rc.d/smartd: WARNING: failed to start smartd > >>>>>> > >>>>>> I had updated the kernel on the machine to 9-STABLE, and > >>>>>> verified that without this patch, the crash still happened with > >>>>>> a 9-STABLE kernel, in addition to 9.1-RELEASE kernel. > >>>>>> > >>>>>> My kernel now identifies itself as: > >>>>>> FreeBSD 9.1-STABLE (GENERIC) #1 r245044:245048M: Fri Jan 4 20:19:50 EST 2013 > >>>>>> > >>>>>> -Kurt > >>>>>> > >>>>>>> Index: cam_periph.c > >>>>>>> =================================================================== > >>>>>>> --- cam_periph.c (revision 245046) > >>>>>>> +++ cam_periph.c (working copy) > >>>>>>> @@ -744,6 +744,9 @@ cam_periph_mapmem(union ccb *ccb, struct cam_perip > >>>>>>> if ((ccb->ccb_h.flags & CAM_DIR_MASK) == CAM_DIR_NONE) > >>>>>>> return(0); > >>>>>>> > >>>>>>> + if ((uintptr_t)ccb->ataio.data_ptr % sizeof(uint16_t) != 0) > >>>>>>> + return (EINVAL); > >>>>>>> + > >>>>>>> data_ptrs[0] = &ccb->ataio.data_ptr; > >>>>>>> lengths[0] = ccb->ataio.dxfer_len; > >>>>>>> dirs[0] = ccb->ccb_h.flags & CAM_DIR_MASK; > >>>>>> > >>>>> > >>>>> Alexander, are you okay with this approach or do you have a better > >>>>> idea how to handle this? In any case, it doesn't seem to make sense > >>>>> to teach the kernel how to cope with arbitrarily aligned buffers for > >>>>> ATA. > >>>> > >>>> Shouldn't we make it dependant on the __NO_STRICT_ALIGNMENT define so > >>>> that it won't immediately break other arches? > >>>> > >>> > >>> No, not doing so tremendously helps ensuring that the software is > >>> properly written (apart from compact flash, ATA devices really > >>> only support 16-bit and 32-bit accesses) and judging the history > >>> of the patches in the smartmontools port it apparently already > >>> has to care about proper alignment for SCSI anyway. It would also > >>> not be the first time the smartmontools port is blown out of the > >>> water :) > >> > >> That patch would do the trick, but I can't say that I like it. Yes, > >> there are many things tied to 16-bit in ATA world: both legacy ATA DMA > >> and AHCI require 16-bit aligned data, legacy ATA DMA also require even > >> transfer size. On the other side, Silicon Image siis(4) chips have no > >> such limitations, that makes it chip-specific, not system-specific > >> problem. > > > > My reading of 3.2.9 of the ATA-7 specification actually is different > > as it says that apart from CFA devices, both DMA and PIO transfers > > are 16-bit and generally there's no guarantee that there's anything > > like a DMA engine or busdma code that can handle unaligned transfers > > in between, especially for PIO. > > Yes, transfers are 16-bit, but odd-sized transfers are zero-padded by > transmitter. PIO may drop extra byte, but with DMA that extra received > byte of padding may overwrite something important. > > >> Also for other DMA cases alignment is handled by busdma code > >> via bounce buffers -- not free, but transparent for user. > > > > The main purpose of DMA bounce buffers is address range translation > > in case a device can't access the whole system memory, thus the > > sparc64 busdma code doesn't use bounce buffers as that job is done > > by the IOMMU. AFAICT, no MD bus_dmamap_load_uio() implementation > > currently handles unaligned transfers as this is not just about > > unaligned buffers in RAM but also the appropriate cache flushing > > necessary for these and just happens to work on x86. > > Hmm. Pity. > > >> For PIO > >> transfers data alignment is just outside of the ATA specification and > >> depends on implementation. > > > > I don't agree here as it is hard to imagine how an ATA controller > > should be able to handle unaligned PIO (assuming your statement > > regarding siis(4) driven chips refers to DMA only and unaligned PIO > > hasn't actually been implemented in some ATA controllers). > > siis(4) uses DMA for all data transfers, including PIO emulation. Ah, PIO emulation is something I had not thought of. > And it > has no address restrictions. Speaking about legacy ATA controller PIO, > what it the problem to read 16-bit value from aligned controller port > and then write it to RAM on odd address in two halves? With PIO > performance is out of question any way. I see no real technical problem in doing so, it's just unpleasant. > > >> I think better solution would be to implement support for misaligned PIO > >> in ata(4) driver. It would cost just about dozen or two lines and should > >> be quite trivial. Probably I should do it last time this alignment > >> problem appeared. > > > > In the end I don't really care as long as it works but from a sheer > > performance point of view it seems wrong to bounce PIO in the kernel > > if it is trivial for applications to care about the alignment required > > by the protocol they want to talk in the first place. > > I don't mind if application will manage some alignment, but so far it > doesn't. > That's hardly an excuse, it seems the majority of applications today aren't written with support for architectures with strict alignment requirements in mind and only tested on x86 :) AFAICT cdrtools in contrast are written with that in mind. Marius