Date: Thu, 25 Apr 2019 16:31:18 -0700 From: Scott Long <scottl@samsco.org> To: Ryan Stone <rysto32@gmail.com> Cc: John Baldwin <jhb@freebsd.org>, Tycho Nightingale <tychon@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: <1356DF49-8D68-45E7-9D29-7FE0097C7B1F@samsco.org> In-Reply-To: <CAFMmRNzouUCbU1n4xMNd4WbQP3=2wxQEHtbTG77J%2B4fKXX8EQA@mail.gmail.com> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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. Scott > 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1356DF49-8D68-45E7-9D29-7FE0097C7B1F>