Date: Thu, 25 Apr 2019 21:09:40 -0400 From: Tycho Nightingale <tychon@freebsd.org> To: Scott Long <scottl@samsco.org> Cc: Ryan Stone <rysto32@gmail.com>, John Baldwin <jhb@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Ed Maste <emaste@freebsd.org>, scottl@freebsd.org Subject: Re: svn commit: r346386 - in head/sys: dev/bge dev/pci dev/twa x86/iommu Message-ID: <17472070-FDB0-4292-B9BF-C033D5B4F634@freebsd.org> In-Reply-To: <1356DF49-8D68-45E7-9D29-7FE0097C7B1F@samsco.org> References: <201904191343.x3JDhYVF010453@repo.freebsd.org> <CAFMmRNxaJ6gWaFpxMFdYx_T2%2BjTRGrvZpvptXFgDr=Z-Zj2=Eg@mail.gmail.com> <CAFMmRNywSPckXRi0oATi3AL%2BBX2MBUQd7t36TCncWiPf06%2BbkQ@mail.gmail.com> <5c43013c-1fc6-57c2-6dec-1fdfc5213fb3@FreeBSD.org> <CAFMmRNzouUCbU1n4xMNd4WbQP3=2wxQEHtbTG77J%2B4fKXX8EQA@mail.gmail.com> <1356DF49-8D68-45E7-9D29-7FE0097C7B1F@samsco.org>
next in thread | previous in thread | raw e-mail | index | archive | help
As Ryan suggests r232260 should be recommitted to get acc(4) fixed. However, given the age of the devices involved and the lack of support = by the standard, I=E2=80=99d say the threshold isn=E2=80=99t met to = reinstate the boundary globally preemptively. To get the insurance, which may not even be necessary, you start to = contort working drivers. For example the insurance can cause legitimate = mappings to fail to load. Since bus_dma(9) has no interface to return = the =E2=80=9Cactive=E2=80=9D boundary and only returns the EFBIG when = boundary constraint causes a mapping to fail to load this causes a = rather cryptic failure when a driver. E.g. with the insurance a a tiny = 8 byte region will fail to load into a single segment on ioat(4) if it = happens to cross an invisible 4GB boundary instituted by the parent. = You need create a tag which allows multiple segments. How many = segments? Well it depends on how many artificial boundaries; it starts = to get ugly. Seems better to support the handful of those devices which = need hand-holding at the tag level for those devices. Tycho > On Apr 25, 2019, at 7:31 PM, Scott Long <scottl@samsco.org> wrote: >=20 > Yeah, it might be turning into an old wives tale at this point. I = clearly remember > it being discussed at the PCI-SIG in late 2003 when PCIe 1.0 was in = its final > draft stages. However, that was a long time ago, and it=E2=80=99s = possible that even > if it=E2=80=99s a limitation in some version or another of the spec, = that most hardware > and firmware just silently account for it. At the time (and even now) = it didn=E2=80=99t > seem like an onerous limitation to place on drivers, especially with = it being > quite easy to implement in FreeBSD. So I=E2=80=99m on the fence; it = might be a bit of > trivia that=E2=80=99s not relevant, and maybe wasn=E2=80=99t ever = relevant, but it=E2=80=99s also cheap > insurance. >=20 > Scott >=20 >=20 >> On Apr 25, 2019, at 4:24 PM, Ryan Stone <rysto32@gmail.com> wrote: >>=20 >> +scottl@, who I believe explained this to us in the first place. >>=20 >> As I recall, it had something to do with 64-bit DMA being expressed = as >> segment base + 32-bit offset. DMA engines that blindly try to cross = a >> 32-bit boundary end up back at the start of the segment and = read/write >> the wrong memory location. >>=20 >> On Thu, Apr 25, 2019 at 4:37 PM John Baldwin <jhb@freebsd.org> wrote: >>>=20 >>> I had looked for the aac change, but wasn't able to find it, perhaps = because I >>> looked at tags created in aac.c rather than aac_pci.c. I agree aac = will need to >>> be re-patched. I'm not really certain how many other devices are = actually broken. >>> They would all be due to a firmware bug, nothing inherent in PCI. >>>=20 >>> I believe twa(4) and bge(4) issues predated aac(4) FWIW. >>>=20 >>> Unfortunately, the main bit of discussion about moving the limit = into the PCI bus >>> itself seems to be an IRC discussion on 2/28/12 that resulted in = revision r232267 >>> as a quick MFC'able fix, but I don't have a log of that = conversation. :( I >>> couldn't find anything in e-mail either that was definitive for why = this might have >>> been inherent in PCI-e vs a few firmware writers having similar = bugs. >>>=20 >>> On 4/25/19 12:20 PM, Ryan Stone wrote: >>>> Following up, this is what will have to be re-instated in the aac = driver: >>>>=20 >>>> http://svn.freebsd.org/changeset/base/232260 >>>>=20 >>>> However, my biggest concern is that we have no idea how many new >>>> devices with the broken behaviour might have been introduced since = we >>>> fixed the problem in general. How does Linux handle the issue? >>>>=20 >>>> On Thu, Apr 25, 2019 at 3:17 PM Ryan Stone <rysto32@gmail.com> = wrote: >>>>>=20 >>>>> This change makes me *very* uncomfortable. It was originally = brought >>>>> in due to issues with Adaptec RAID cards using the aac(9) driver. = The >>>>> symptoms of the bug included silent corruption of data as it was >>>>> written to disk. Are we sure that this change is a good idea, = given >>>>> how catastrophic it is when a device gets this wrong? >>>>>=20 >>>>> On Fri, Apr 19, 2019 at 9:43 AM Tycho Nightingale = <tychon@freebsd.org> wrote: >>>>>>=20 >>>>>> Author: tychon >>>>>> Date: Fri Apr 19 13:43:33 2019 >>>>>> New Revision: 346386 >>>>>> URL: https://svnweb.freebsd.org/changeset/base/346386 >>>>>>=20 >>>>>> Log: >>>>>> remove the 4GB boundary requirement on PCI DMA segments >>>>>>=20 >>>>>> Reviewed by: kib >>>>>> Discussed with: jhb >>>>>> Sponsored by: Dell EMC Isilon >>>>>> Differential Revision: https://reviews.freebsd.org/D19867 >>>>>>=20 >>>>>> Modified: >>>>>> head/sys/dev/bge/if_bgereg.h >>>>>> head/sys/dev/pci/pci.c >>>>>> head/sys/dev/pci/pcivar.h >>>>>> head/sys/dev/twa/tw_osl.h >>>>>> head/sys/dev/twa/tw_osl_freebsd.c >>>>>> head/sys/x86/iommu/intel_ctx.c >>>>>>=20 >>>>>> Modified: head/sys/dev/bge/if_bgereg.h >>>>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>>>> --- head/sys/dev/bge/if_bgereg.h Fri Apr 19 13:23:41 2019 = (r346385) >>>>>> +++ head/sys/dev/bge/if_bgereg.h Fri Apr 19 13:43:33 2019 = (r346386) >>>>>> @@ -3067,3 +3067,11 @@ struct bge_softc { >>>>>> #define BGE_LOCK_ASSERT(_sc) = mtx_assert(&(_sc)->bge_mtx, MA_OWNED) >>>>>> #define BGE_UNLOCK(_sc) = mtx_unlock(&(_sc)->bge_mtx) >>>>>> #define BGE_LOCK_DESTROY(_sc) = mtx_destroy(&(_sc)->bge_mtx) >>>>>> + >>>>>> +#ifdef BUS_SPACE_MAXADDR >>>>>> +#if (BUS_SPACE_MAXADDR > 0xFFFFFFFF) >>>>>> +#define BGE_DMA_BOUNDARY (0x100000000) >>>>>> +#else >>>>>> +#define BGE_DMA_BOUNDARY 0 >>>>>> +#endif >>>>>> +#endif >>>>>>=20 >>>>>> Modified: head/sys/dev/pci/pci.c >>>>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>>>> --- head/sys/dev/pci/pci.c Fri Apr 19 13:23:41 2019 = (r346385) >>>>>> +++ head/sys/dev/pci/pci.c Fri Apr 19 13:43:33 2019 = (r346386) >>>>>> @@ -4343,9 +4343,6 @@ pci_attach_common(device_t dev) >>>>>> { >>>>>> struct pci_softc *sc; >>>>>> int busno, domain; >>>>>> -#ifdef PCI_DMA_BOUNDARY >>>>>> - int error, tag_valid; >>>>>> -#endif >>>>>> #ifdef PCI_RES_BUS >>>>>> int rid; >>>>>> #endif >>>>>> @@ -4365,23 +4362,7 @@ pci_attach_common(device_t dev) >>>>>> if (bootverbose) >>>>>> device_printf(dev, "domain=3D%d, physical = bus=3D%d\n", >>>>>> domain, busno); >>>>>> -#ifdef PCI_DMA_BOUNDARY >>>>>> - tag_valid =3D 0; >>>>>> - if = (device_get_devclass(device_get_parent(device_get_parent(dev))) !=3D >>>>>> - devclass_find("pci")) { >>>>>> - error =3D = bus_dma_tag_create(bus_get_dma_tag(dev), 1, >>>>>> - PCI_DMA_BOUNDARY, BUS_SPACE_MAXADDR, = BUS_SPACE_MAXADDR, >>>>>> - NULL, NULL, BUS_SPACE_MAXSIZE, = BUS_SPACE_UNRESTRICTED, >>>>>> - BUS_SPACE_MAXSIZE, 0, NULL, NULL, = &sc->sc_dma_tag); >>>>>> - if (error) >>>>>> - device_printf(dev, "Failed to create DMA = tag: %d\n", >>>>>> - error); >>>>>> - else >>>>>> - tag_valid =3D 1; >>>>>> - } >>>>>> - if (!tag_valid) >>>>>> -#endif >>>>>> - sc->sc_dma_tag =3D bus_get_dma_tag(dev); >>>>>> + sc->sc_dma_tag =3D bus_get_dma_tag(dev); >>>>>> return (0); >>>>>> } >>>>>>=20 >>>>>>=20 >>>>>> Modified: head/sys/dev/pci/pcivar.h >>>>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>>>> --- head/sys/dev/pci/pcivar.h Fri Apr 19 13:23:41 2019 = (r346385) >>>>>> +++ head/sys/dev/pci/pcivar.h Fri Apr 19 13:43:33 2019 = (r346386) >>>>>> @@ -693,14 +693,6 @@ int pcie_link_reset(device_t port, = int pcie_location); >>>>>>=20 >>>>>> void pci_print_faulted_dev(void); >>>>>>=20 >>>>>> -#ifdef BUS_SPACE_MAXADDR >>>>>> -#if (BUS_SPACE_MAXADDR > 0xFFFFFFFF) >>>>>> -#define PCI_DMA_BOUNDARY 0x100000000 >>>>>> -#else >>>>>> -#define PCI_DMA_BOUNDARY 0 >>>>>> -#endif >>>>>> -#endif >>>>>> - >>>>>> #endif /* _SYS_BUS_H_ */ >>>>>>=20 >>>>>> /* >>>>>>=20 >>>>>> Modified: head/sys/dev/twa/tw_osl.h >>>>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>>>> --- head/sys/dev/twa/tw_osl.h Fri Apr 19 13:23:41 2019 = (r346385) >>>>>> +++ head/sys/dev/twa/tw_osl.h Fri Apr 19 13:43:33 2019 = (r346386) >>>>>> @@ -57,6 +57,12 @@ >>>>>> #define TW_OSLI_MAX_NUM_IOS (TW_OSLI_MAX_NUM_REQUESTS = - 2) >>>>>> #define TW_OSLI_MAX_NUM_AENS 0x100 >>>>>>=20 >>>>>> +#ifdef PAE >>>>>> +#define TW_OSLI_DMA_BOUNDARY (1u << 31) >>>>>> +#else >>>>>> +#define TW_OSLI_DMA_BOUNDARY = ((bus_size_t)((uint64_t)1 << 32)) >>>>>> +#endif >>>>>> + >>>>>> /* Possible values of req->state. */ >>>>>> #define TW_OSLI_REQ_STATE_INIT 0x0 /* being = initialized */ >>>>>> #define TW_OSLI_REQ_STATE_BUSY 0x1 /* submitted to CL = */ >>>>>>=20 >>>>>> Modified: head/sys/dev/twa/tw_osl_freebsd.c >>>>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>>>> --- head/sys/dev/twa/tw_osl_freebsd.c Fri Apr 19 13:23:41 2019 = (r346385) >>>>>> +++ head/sys/dev/twa/tw_osl_freebsd.c Fri Apr 19 13:43:33 2019 = (r346386) >>>>>> @@ -551,7 +551,7 @@ tw_osli_alloc_mem(struct twa_softc *sc) >>>>>> /* Create the parent dma tag. */ >>>>>> if (bus_dma_tag_create(bus_get_dma_tag(sc->bus_dev), /* = parent */ >>>>>> sc->alignment, /* = alignment */ >>>>>> - 0, /* = boundary */ >>>>>> + TW_OSLI_DMA_BOUNDARY, /* = boundary */ >>>>>> BUS_SPACE_MAXADDR, /* lowaddr = */ >>>>>> BUS_SPACE_MAXADDR, /* highaddr = */ >>>>>> NULL, NULL, /* filter, = filterarg */ >>>>>>=20 >>>>>> Modified: head/sys/x86/iommu/intel_ctx.c >>>>>> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >>>>>> --- head/sys/x86/iommu/intel_ctx.c Fri Apr 19 13:23:41 2019 = (r346385) >>>>>> +++ head/sys/x86/iommu/intel_ctx.c Fri Apr 19 13:43:33 2019 = (r346386) >>>>>> @@ -130,7 +130,7 @@ ctx_tag_init(struct dmar_ctx *ctx, device_t = dev) >>>>>> maxaddr =3D MIN(ctx->domain->end, BUS_SPACE_MAXADDR); >>>>>> ctx->ctx_tag.common.ref_count =3D 1; /* Prevent free */ >>>>>> ctx->ctx_tag.common.impl =3D &bus_dma_dmar_impl; >>>>>> - ctx->ctx_tag.common.boundary =3D PCI_DMA_BOUNDARY; >>>>>> + ctx->ctx_tag.common.boundary =3D 0; >>>>>> ctx->ctx_tag.common.lowaddr =3D maxaddr; >>>>>> ctx->ctx_tag.common.highaddr =3D maxaddr; >>>>>> ctx->ctx_tag.common.maxsize =3D maxaddr; >>>>>>=20 >>>>=20 >>>=20 >>>=20 >>> -- >>> John Baldwin >=20 >=20
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?17472070-FDB0-4292-B9BF-C033D5B4F634>