From owner-freebsd-sparc64@FreeBSD.ORG Sun Jan 13 16:49:10 2013 Return-Path: Delivered-To: freebsd-sparc64@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by hub.freebsd.org (Postfix) with ESMTP id 7695AAE0 for ; Sun, 13 Jan 2013 16:49:10 +0000 (UTC) (envelope-from mavbsd@gmail.com) Received: from mail-bk0-f52.google.com (mail-bk0-f52.google.com [209.85.214.52]) by mx1.freebsd.org (Postfix) with ESMTP id 0872AEA9 for ; Sun, 13 Jan 2013 16:49:09 +0000 (UTC) Received: by mail-bk0-f52.google.com with SMTP id w5so1557381bku.25 for ; Sun, 13 Jan 2013 08:49:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=x-received:sender:message-id:date:from:user-agent:mime-version:to :cc:subject:references:in-reply-to:content-type :content-transfer-encoding; bh=MWR4j8Mv32M4DiNzvSiVgrcMFvioBxAHtBctGu6HBog=; b=PLAjahaabCWY4KuFZBf15vDrH+11vR6lzXS+b+SZRHkM6mytkvN/bEuYZ9BtEte86e dYovt9VjfgKsrOFYNmGa1ZZedD/CvYTpTS9b6dWYyoYJQjXIk+Eq2RRMgfQyi219zP2B 7peClBFBL1T1OoGImIm+uNv4fr8Nf1gy9yWMYlU/PDWKjQqPr9IK3pB1z96xMn5pP3ZA XzC823wpXYNJqEg9MEUU3JE72aOnpF9pnzeH08MAJ6Xe0Y0tAoh5iokuCWfuAT8FT5cc hUc3eu7tB6ul4cYKVMnyVsBUEZ97S9zLZuCiLbMrANb5aSC2zC+olyRAsKa1/KLoWLM0 pEZw== X-Received: by 10.204.157.26 with SMTP id z26mr38551383bkw.101.1358095743510; Sun, 13 Jan 2013 08:49:03 -0800 (PST) Received: from mavbook.mavhome.dp.ua (mavhome.mavhome.dp.ua. [213.227.240.37]) by mx.google.com with ESMTPS id r16sm7808185bkv.3.2013.01.13.08.49.01 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Sun, 13 Jan 2013 08:49:02 -0800 (PST) Sender: Alexander Motin Message-ID: <50F2E57C.1090305@FreeBSD.org> Date: Sun, 13 Jan 2013 18:49:00 +0200 From: Alexander Motin User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Marius Strobl Subject: Re: smartmontools panics 9.1-RELEASE on sunfire 240 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> In-Reply-To: <20130113155516.GK26039@alchemy.franken.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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 16:49:10 -0000 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. 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 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. -- Alexander Motin