Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
> Date: Thu, 8 Oct 2020 00:34:28 -0500
> From: Jordan Hargrave
>
> Ok updated the new changes.
I think this is good enough for further cleanup in the tree now.
Builds all thr amd64 kernels, doesn't break i386 and arm64 GENERIC.MP.
However, I think acpidmar(4) shouldn't be enabled yet until have done
a bit more testing. Theo do we want to compile in the driver such
that people can easily flip it on in UKC?
I also noticed that re(4) does a bad DMA transfer as soon as the
interface is brought up. With your diff the kernel doesn't panic if
that happensl it just spits out some debugging information. We may
want to change that in the future.
ok kettenis@ once the question about enabling acpidmar(4) is resolved.
>
> On Mon, Oct 05, 2020 at 09:54:02PM +0200, Mark Kettenis wrote:
> > > Date: Thu, 17 Sep 2020 20:54:51 -0500
> > > From: Jordan Hargrave
> > > Cc: ma...@peereboom.org, kette...@openbsd.org, tech@openbsd.org,
> > > d...@openbsd.org, j...@openbsd.org
> > > Content-Type: text/plain; charset=us-ascii
> > > Content-Disposition: inline
> > >
> > > Ok made more changes
> > >
> > > >
> > > > Should be handled by that activate function as well.
> > > >
> >
> > So there are quite a few style issues. I can point them out to you,
> > or I could fix them after this is committed, which is probably more
> > efficient.
> >
> > Also, there seems to be lot of debug code left in here that should be
> > removed or at least hidden before this gets committed.
> >
> > >
> > > diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC
> > > index 1d6397391..a69c72c26 100644
> > > --- a/sys/arch/amd64/conf/GENERIC
> > > +++ b/sys/arch/amd64/conf/GENERIC
> > > @@ -45,6 +45,7 @@ acpibtn*at acpi?
> > > acpicpu* at acpi?
> > > acpicmos*at acpi?
> > > acpidock*at acpi?
> > > +acpidmar0at acpi?
> > > acpiec* at acpi?
> > > acpipci* at acpi?
> > > acpiprt* at acpi?
> > > diff --git a/sys/arch/amd64/include/pci_machdep.h
> > > b/sys/arch/amd64/include/pci_machdep.h
> > > index bc295cc22..ea09f1abc 100644
> > > --- a/sys/arch/amd64/include/pci_machdep.h
> > > +++ b/sys/arch/amd64/include/pci_machdep.h
> > > @@ -91,7 +91,12 @@ void
> > > *pci_intr_establish_cpu(pci_chipset_tag_t, pci_intr_handle_t,
> > > int, struct cpu_info *,
> > > int (*)(void *), void *, const char *);
> > > void pci_intr_disestablish(pci_chipset_tag_t, void *);
> > > +#if NACPIDMAR > 0
> > > +int pci_probe_device_hook(pci_chipset_tag_t,
> > > + struct pci_attach_args *);
> > > +#else
> > > #define pci_probe_device_hook(c, a) (0)
> > > +#endif
> >
> > This is probably a bad idea. You don't include "acpidmar.h" in this
> > file, and doing so is probaly undesireable. But that means the
> > definition of the hook function depends on whether the file that
> > includes this does that or not.
> >
> > Better just unconditionally provide the prototype and use a #if
> > NACPIDMAR > 0 in the implementation.
> >
>
> Ok changed to that method
>
> > > +#include "acpidmar.h"
> > > +#include "amd_iommu.h"
> > > +
> > > +//#define IOMMU_DEBUG
> >
> > No C++-style comments please. Make this an #undef or use /* */.
> >
> > > +
> > > +#ifdef IOMMU_DEBUG
> > > +#define dprintf(x...) printf(x)
> > > +#else
> > > +#define dprintf(x...)
> > > +#endif
> > > +
> > > +#ifdef DDB
> > > +int acpidmar_ddb = 0;
> > > +#endif
> > > +
> > > +int intel_iommu_gfx_mapped = 0;
> >
> > Unused variable.
> >
> > > +int force_cm = 1;
> >
> > Rename to "acpidmar_force_cm"?
> >
> > > +
> > > +void showahci(void *);
> >
> > Unused prototype.
> >
> > > +
> > > +/* Page Table Entry per domain */
> > > +struct iommu_softc;
> > > +
> > > +static inline int
> > > +mksid(int b, int d, int f)
> > > +{
> > > + return (b << 8) + (d << 3) + f;
> > > +}
> > > +
> > > +static inline int
> > > +sid_devfn(int sid)
> > > +{
> > > + return sid & 0xff;
> > > +}
> > > +
> > > +static inline int
> > > +sid_bus(int sid)
> > > +{
> > > + return (sid >> 8) & 0xff;
> > > +}
> > > +
> > > +static inline int
> > > +sid_dev(int sid)
> > > +{
> > > + return (sid >> 3) & 0x1f;
> > > +}
> > > +
> > > +static inline int
> > > +sid_fun(int sid)
> > > +{
> > > + return (sid >> 0) & 0x7;
> > > +}
> > > +
> > > +/* Alias mapping */
> > > +#define SID_INVALID 0x8000L
> > > +static uint32_t sid_flag[65536];
> > > +
> > > +struct domain_dev {
> > > + int sid;
> > > + int sec;
> > > + int sub;
> > > + TAILQ_ENTRY(domain_dev) link;
> > > +};
> > > +
> > > +struct domain {
> > > + struct iommu_softc *iommu;
> > > + int did;
> > > + int gaw;
> > > + struct pte_entry*pte;
> > > + paddr_t ptep;
> > > + struct bus_dma_tag dmat;
> > > + int flag;
> > > +
> > > + struct mutexexlck;
> > >
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
Ok updated the new changes.
On Mon, Oct 05, 2020 at 09:54:02PM +0200, Mark Kettenis wrote:
> > Date: Thu, 17 Sep 2020 20:54:51 -0500
> > From: Jordan Hargrave
> > Cc: ma...@peereboom.org, kette...@openbsd.org, tech@openbsd.org,
> > d...@openbsd.org, j...@openbsd.org
> > Content-Type: text/plain; charset=us-ascii
> > Content-Disposition: inline
> >
> > Ok made more changes
> >
> > >
> > > Should be handled by that activate function as well.
> > >
>
> So there are quite a few style issues. I can point them out to you,
> or I could fix them after this is committed, which is probably more
> efficient.
>
> Also, there seems to be lot of debug code left in here that should be
> removed or at least hidden before this gets committed.
>
> >
> > diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC
> > index 1d6397391..a69c72c26 100644
> > --- a/sys/arch/amd64/conf/GENERIC
> > +++ b/sys/arch/amd64/conf/GENERIC
> > @@ -45,6 +45,7 @@ acpibtn* at acpi?
> > acpicpu* at acpi?
> > acpicmos* at acpi?
> > acpidock* at acpi?
> > +acpidmar0 at acpi?
> > acpiec*at acpi?
> > acpipci* at acpi?
> > acpiprt* at acpi?
> > diff --git a/sys/arch/amd64/include/pci_machdep.h
> > b/sys/arch/amd64/include/pci_machdep.h
> > index bc295cc22..ea09f1abc 100644
> > --- a/sys/arch/amd64/include/pci_machdep.h
> > +++ b/sys/arch/amd64/include/pci_machdep.h
> > @@ -91,7 +91,12 @@ void
> > *pci_intr_establish_cpu(pci_chipset_tag_t, pci_intr_handle_t,
> > int, struct cpu_info *,
> > int (*)(void *), void *, const char *);
> > void pci_intr_disestablish(pci_chipset_tag_t, void *);
> > +#if NACPIDMAR > 0
> > +intpci_probe_device_hook(pci_chipset_tag_t,
> > + struct pci_attach_args *);
> > +#else
> > #definepci_probe_device_hook(c, a) (0)
> > +#endif
>
> This is probably a bad idea. You don't include "acpidmar.h" in this
> file, and doing so is probaly undesireable. But that means the
> definition of the hook function depends on whether the file that
> includes this does that or not.
>
> Better just unconditionally provide the prototype and use a #if
> NACPIDMAR > 0 in the implementation.
>
Ok changed to that method
> > +#include "acpidmar.h"
> > +#include "amd_iommu.h"
> > +
> > +//#define IOMMU_DEBUG
>
> No C++-style comments please. Make this an #undef or use /* */.
>
> > +
> > +#ifdef IOMMU_DEBUG
> > +#define dprintf(x...) printf(x)
> > +#else
> > +#define dprintf(x...)
> > +#endif
> > +
> > +#ifdef DDB
> > +intacpidmar_ddb = 0;
> > +#endif
> > +
> > +intintel_iommu_gfx_mapped = 0;
>
> Unused variable.
>
> > +intforce_cm = 1;
>
> Rename to "acpidmar_force_cm"?
>
> > +
> > +void showahci(void *);
>
> Unused prototype.
>
> > +
> > +/* Page Table Entry per domain */
> > +struct iommu_softc;
> > +
> > +static inline int
> > +mksid(int b, int d, int f)
> > +{
> > + return (b << 8) + (d << 3) + f;
> > +}
> > +
> > +static inline int
> > +sid_devfn(int sid)
> > +{
> > + return sid & 0xff;
> > +}
> > +
> > +static inline int
> > +sid_bus(int sid)
> > +{
> > + return (sid >> 8) & 0xff;
> > +}
> > +
> > +static inline int
> > +sid_dev(int sid)
> > +{
> > + return (sid >> 3) & 0x1f;
> > +}
> > +
> > +static inline int
> > +sid_fun(int sid)
> > +{
> > + return (sid >> 0) & 0x7;
> > +}
> > +
> > +/* Alias mapping */
> > +#define SID_INVALID 0x8000L
> > +static uint32_t sid_flag[65536];
> > +
> > +struct domain_dev {
> > + int sid;
> > + int sec;
> > + int sub;
> > + TAILQ_ENTRY(domain_dev) link;
> > +};
> > +
> > +struct domain {
> > + struct iommu_softc *iommu;
> > + int did;
> > + int gaw;
> > + struct pte_entry*pte;
> > + paddr_t ptep;
> > + struct bus_dma_tag dmat;
> > + int flag;
> > +
> > + struct mutexexlck;
> > + charexname[32];
> > + struct extent *iovamap;
> > + TAILQ_HEAD(,domain_dev) devices;
> > + TAILQ_ENTRY(domain) link;
> > +};
> > +
> > +#define DOM_DEBUG 0x1
> > +#define DOM_NOMAP 0x2
> > +
> > +struct dmar_devlist {
> > + int type;
> > + int bus;
> > + int ndp;
> > + struct acpidmar_devpath *dp;
> > + TAILQ_ENTRY(dmar_devlist) link;
> > +};
> > +
> > +TAILQ_HEAD(devlist_head, dmar_devlist);
> > +
> > +struct ivhd_devlist {
> > + int start_id;
> > + int end_id;
> > + int cfg;
> > + TAILQ_ENTRY(ivhd_devlist) link;
> > +};
> > +
> > +struct rmrr_softc {
> > + TAILQ_ENTRY(rmrr_softc) link;
> > + struct devlist_head devices;
> > + int segment;
> > + uint64_t
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
> Date: Thu, 17 Sep 2020 20:54:51 -0500
> From: Jordan Hargrave
> Cc: ma...@peereboom.org, kette...@openbsd.org, tech@openbsd.org,
> d...@openbsd.org, j...@openbsd.org
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
>
> Ok made more changes
>
> On Mon, Sep 14, 2020 at 08:19:18PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 8 Sep 2020 21:43:39 -0500
> > > From: Jordan Hargrave
> > >
> > > Made changes for the iommu_readq -> iommu_read_8 and also now
> > > dynamically allocate the hwdte for AMD IOMMU.
> >
> > Some more bits...
> >
> > > On Fri, Sep 04, 2020 at 09:17:18PM +0200, Mark Kettenis wrote:
> > > > > Date: Fri, 4 Sep 2020 00:50:44 -0500
> > > > > From: Jordan Hargrave
> > > >
> > > > A few hints below...
> > > >
> > > > > > > +
> > > > > > > +/* Page Table Entry per domain */
> > > > > > > +static struct ivhd_dte hwdte[65536] __aligned(PAGE_SIZE);
> > > > > > > +
> > > > > > > +/* Alias mapping */
> > > > > > > +#define SID_INVALID 0x8000L
> > > > > > > +static uint32_t sid_flag[65536];
> > > > > >
> > > > > > Can we avoid having these large arrays, or at least allocate them
> > > > > > dynamically? That would also avoid the explicit alignment which is
> > > > > > somewhat nasty since it affects the entire kernel.
> > > > >
> > > > > OK. But the hwdte does need the 2M area to be all contiguous but it
> > > > > is not
> > > > > needed for DMAR/Intel. You *can* have up to 8 different device table
> > > > > entries
> > > > > though to split up the area.
> > > >
> > > > The appropriate interface to use in this context is
> > > > bus_dmamem_alloc(9). You can specify alignment, and if you set nsegs
> > > > to 1, you will get memory that is physicaly contiguous.
> > > >
> > > > To map the memory into kernel address space you'll need create a map
> > > > using bus_dmamap_create(9) and map it using bus_dmamem_map(9). Then
> > > > instead of using pmap_extract(9) you use bus_dmamap_load_raw(9) which
> > > > then populates the physical addresses.
> > > >
> > > > Many of the drivers written by dlg@ define convenience functions to do
> > > > all these steps, although interestingly enough he tends to use
> > > > bus_dmamap_load(9) instead of bus_dmamap_load_raw(9) which is
> > > > sub-optimal.
> > > >
> > > > > > > +
> > > > > > > +struct domain_dev {
> > > > > > > + int sid;
> > > > > > > + int sec;
> > > > > > > + int sub;
> > > > > > > + TAILQ_ENTRY(domain_dev) link;
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct domain {
> > > > > > > + struct iommu_softc *iommu;
> > > > > > > + int did;
> > > > > > > + int gaw;
> > > > > > > + struct pte_entry*pte;
> > > > > > > + paddr_t ptep;
> > > > > > > + struct bus_dma_tag dmat;
> > > > > > > + int flag;
> > > > > > > +
> > > > > > > + struct mutexexlck;
> > > > > > > + charexname[32];
> > > > > > > + struct extent *iovamap;
> > > > > > > + TAILQ_HEAD(,domain_dev) devices;
> > > > > > > + TAILQ_ENTRY(domain) link;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define DOM_DEBUG 0x1
> > > > > > > +#define DOM_NOMAP 0x2
> > > > > > > +
> > > > > > > +struct dmar_devlist {
> > > > > > > + int type;
> > > > > > > + int bus;
> > > > > > > + int ndp;
> > > > > > > + struct acpidmar_devpath *dp;
> > > > > > > + TAILQ_ENTRY(dmar_devlist) link;
> > > > > > > +};
> > > > > > > +
> > > > > > > +TAILQ_HEAD(devlist_head, dmar_devlist);
> > > > > > > +
> > > > > > > +struct ivhd_devlist {
> > > > > > > + int start_id;
> > > > > > > + int end_id;
> > > > > > > + int cfg;
> > > > > > > + TAILQ_ENTRY(ivhd_devlist) link;
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct rmrr_softc {
> > > > > > > + TAILQ_ENTRY(rmrr_softc) link;
> > > > > > > + struct devlist_head devices;
> > > > > > > + int segment;
> > > > > > > + uint64_tstart;
> > > > > > > + uint64_tend;
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct atsr_softc {
> > > > > > > + TAILQ_ENTRY(atsr_softc) link;
> > > > > > > + struct devlist_head devices;
> > > > > > > + int segment;
> > > > > > > + int flags;
> > > > > > > +};
> > > > > > > +
> > > > > > > +struct iommu_pic {
> > > > > > > + struct pic pic;
> > > > > > > + struct iommu_softc *iommu;
> > > > > > > +};
> > > > > > > +
> > > > > > > +#define IOMMU_FLAGS_CATCHALL 0x1
> > > > > > > +#define IOMMU_FLAGS_BAD 0x2
> > > > > > > +#define IOMMU_FLAGS_SUSPEND 0x4
> > > > > > > +
> > > > > > > +struct iommu_softc {
> > > > > > > + TAILQ_ENTRY(iommu_softc
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
Ping... still need more eyes on this
This is the IOMMU code for VT-d and AMD Vi implementation
It overrides the DMA Tag for each device and assigns it to a
protected domain
On Thu, Sep 17, 2020 at 08:54:51PM -0500, Jordan Hargrave wrote:
> Ok made more changes
>
> On Mon, Sep 14, 2020 at 08:19:18PM +0200, Mark Kettenis wrote:
> > > Date: Tue, 8 Sep 2020 21:43:39 -0500
> > > From: Jordan Hargrave
> > >
> > > Made changes for the iommu_readq -> iommu_read_8 and also now
> > > dynamically allocate the hwdte for AMD IOMMU.
> >
> > Some more bits...
> >
> > > On Fri, Sep 04, 2020 at 09:17:18PM +0200, Mark Kettenis wrote:
> > > > > Date: Fri, 4 Sep 2020 00:50:44 -0500
> > > > > From: Jordan Hargrave
> > > >
> > > > A few hints below...
> > > >
> > > > > > > +
>
> diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC
> index 1d6397391..a69c72c26 100644
> --- a/sys/arch/amd64/conf/GENERIC
> +++ b/sys/arch/amd64/conf/GENERIC
> @@ -45,6 +45,7 @@ acpibtn*at acpi?
> acpicpu* at acpi?
> acpicmos*at acpi?
> acpidock*at acpi?
> +acpidmar0at acpi?
> acpiec* at acpi?
> acpipci* at acpi?
> acpiprt* at acpi?
> diff --git a/sys/arch/amd64/include/pci_machdep.h
> b/sys/arch/amd64/include/pci_machdep.h
> index bc295cc22..ea09f1abc 100644
> --- a/sys/arch/amd64/include/pci_machdep.h
> +++ b/sys/arch/amd64/include/pci_machdep.h
> @@ -91,7 +91,12 @@ void
> *pci_intr_establish_cpu(pci_chipset_tag_t, pci_intr_handle_t,
> int, struct cpu_info *,
> int (*)(void *), void *, const char *);
> void pci_intr_disestablish(pci_chipset_tag_t, void *);
> +#if NACPIDMAR > 0
> +int pci_probe_device_hook(pci_chipset_tag_t,
> + struct pci_attach_args *);
> +#else
> #define pci_probe_device_hook(c, a) (0)
> +#endif
>
> void pci_dev_postattach(struct device *, struct
> pci_attach_args *);
>
> diff --git a/sys/arch/amd64/pci/pci_machdep.c
> b/sys/arch/amd64/pci/pci_machdep.c
> index cf4e835de..d590c3514 100644
> --- a/sys/arch/amd64/pci/pci_machdep.c
> +++ b/sys/arch/amd64/pci/pci_machdep.c
> @@ -89,6 +89,11 @@
> #include
> #endif
>
> +#include "acpi.h"
> +#if NACPIDMAR > 0
> +#include
> +#endif
> +
> /*
> * Memory Mapped Configuration space access.
> *
> @@ -797,7 +802,15 @@ pci_init_extents(void)
> }
> }
>
> -#include "acpi.h"
> +#if NACPIDMAR > 0
> +int
> +pci_probe_device_hook(pci_chipset_tag_t pc, struct pci_attach_args *pa)
> +{
> + acpidmar_pci_hook(pc, pa);
> + return 0;
> +}
> +#endif
> +
> #if NACPI > 0
> void acpi_pci_match(struct device *, struct pci_attach_args *);
> pcireg_t acpi_pci_min_powerstate(pci_chipset_tag_t, pcitag_t);
> diff --git a/sys/dev/acpi/acpidmar.c b/sys/dev/acpi/acpidmar.c
> new file mode 100644
> index 0..4519abc26
> --- /dev/null
> +++ b/sys/dev/acpi/acpidmar.c
> @@ -0,0 +1,3041 @@
> +/*
> + * Copyright (c) 2015 Jordan Hargrave
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +
> +#include "ioapic.h"
> +
> +#include "acpidmar.h"
> +#include "amd_iommu.h"
> +
> +//#define IOMMU_DEBUG
> +
> +#ifdef IOMMU_DEBUG
> +#define dprintf(x...) printf(x)
> +#else
> +#define dprintf(x...)
> +#endif
> +
> +#ifdef DDB
> +int acpidmar_ddb = 0;
> +#endif
> +
> +int intel_iommu_gfx_mapped = 0;
> +int force_cm = 1;
> +
> +void showahci(void *);
> +
> +/* Page Table Entry per domain */
> +struct iommu_softc;
> +
> +static inline int
> +mksid(int b, int d, int f)
> +{
> + return (b << 8) + (d << 3) + f;
> +}
> +
> +static inline int
> +sid_devfn(int sid)
> +{
> + return sid & 0xff;
> +}
> +
> +static inline int
> +sid_bus(int sid)
> +{
> + return (sid >> 8) & 0xff;
> +}
> +
> +static inline int
> +sid_dev(int sid)
> +{
> + return (sid >> 3) & 0x1f;
> +}
> +
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
Ok made more changes
On Mon, Sep 14, 2020 at 08:19:18PM +0200, Mark Kettenis wrote:
> > Date: Tue, 8 Sep 2020 21:43:39 -0500
> > From: Jordan Hargrave
> >
> > Made changes for the iommu_readq -> iommu_read_8 and also now
> > dynamically allocate the hwdte for AMD IOMMU.
>
> Some more bits...
>
> > On Fri, Sep 04, 2020 at 09:17:18PM +0200, Mark Kettenis wrote:
> > > > Date: Fri, 4 Sep 2020 00:50:44 -0500
> > > > From: Jordan Hargrave
> > >
> > > A few hints below...
> > >
> > > > > > +
> > > > > > +/* Page Table Entry per domain */
> > > > > > +static struct ivhd_dte hwdte[65536] __aligned(PAGE_SIZE);
> > > > > > +
> > > > > > +/* Alias mapping */
> > > > > > +#define SID_INVALID 0x8000L
> > > > > > +static uint32_t sid_flag[65536];
> > > > >
> > > > > Can we avoid having these large arrays, or at least allocate them
> > > > > dynamically? That would also avoid the explicit alignment which is
> > > > > somewhat nasty since it affects the entire kernel.
> > > >
> > > > OK. But the hwdte does need the 2M area to be all contiguous but it is
> > > > not
> > > > needed for DMAR/Intel. You *can* have up to 8 different device table
> > > > entries
> > > > though to split up the area.
> > >
> > > The appropriate interface to use in this context is
> > > bus_dmamem_alloc(9). You can specify alignment, and if you set nsegs
> > > to 1, you will get memory that is physicaly contiguous.
> > >
> > > To map the memory into kernel address space you'll need create a map
> > > using bus_dmamap_create(9) and map it using bus_dmamem_map(9). Then
> > > instead of using pmap_extract(9) you use bus_dmamap_load_raw(9) which
> > > then populates the physical addresses.
> > >
> > > Many of the drivers written by dlg@ define convenience functions to do
> > > all these steps, although interestingly enough he tends to use
> > > bus_dmamap_load(9) instead of bus_dmamap_load_raw(9) which is
> > > sub-optimal.
> > >
> > > > > > +
> > > > > > +struct domain_dev {
> > > > > > + int sid;
> > > > > > + int sec;
> > > > > > + int sub;
> > > > > > + TAILQ_ENTRY(domain_dev) link;
> > > > > > +};
> > > > > > +
> > > > > > +struct domain {
> > > > > > + struct iommu_softc *iommu;
> > > > > > + int did;
> > > > > > + int gaw;
> > > > > > + struct pte_entry*pte;
> > > > > > + paddr_t ptep;
> > > > > > + struct bus_dma_tag dmat;
> > > > > > + int flag;
> > > > > > +
> > > > > > + struct mutexexlck;
> > > > > > + charexname[32];
> > > > > > + struct extent *iovamap;
> > > > > > + TAILQ_HEAD(,domain_dev) devices;
> > > > > > + TAILQ_ENTRY(domain) link;
> > > > > > +};
> > > > > > +
> > > > > > +#define DOM_DEBUG 0x1
> > > > > > +#define DOM_NOMAP 0x2
> > > > > > +
> > > > > > +struct dmar_devlist {
> > > > > > + int type;
> > > > > > + int bus;
> > > > > > + int ndp;
> > > > > > + struct acpidmar_devpath *dp;
> > > > > > + TAILQ_ENTRY(dmar_devlist) link;
> > > > > > +};
> > > > > > +
> > > > > > +TAILQ_HEAD(devlist_head, dmar_devlist);
> > > > > > +
> > > > > > +struct ivhd_devlist {
> > > > > > + int start_id;
> > > > > > + int end_id;
> > > > > > + int cfg;
> > > > > > + TAILQ_ENTRY(ivhd_devlist) link;
> > > > > > +};
> > > > > > +
> > > > > > +struct rmrr_softc {
> > > > > > + TAILQ_ENTRY(rmrr_softc) link;
> > > > > > + struct devlist_head devices;
> > > > > > + int segment;
> > > > > > + uint64_tstart;
> > > > > > + uint64_tend;
> > > > > > +};
> > > > > > +
> > > > > > +struct atsr_softc {
> > > > > > + TAILQ_ENTRY(atsr_softc) link;
> > > > > > + struct devlist_head devices;
> > > > > > + int segment;
> > > > > > + int flags;
> > > > > > +};
> > > > > > +
> > > > > > +struct iommu_pic {
> > > > > > + struct pic pic;
> > > > > > + struct iommu_softc *iommu;
> > > > > > +};
> > > > > > +
> > > > > > +#define IOMMU_FLAGS_CATCHALL 0x1
> > > > > > +#define IOMMU_FLAGS_BAD0x2
> > > > > > +#define IOMMU_FLAGS_SUSPEND0x4
> > > > > > +
> > > > > > +struct iommu_softc {
> > > > > > + TAILQ_ENTRY(iommu_softc)link;
> > > > > > + struct devlist_head devices;
> > > > > > + int id;
> > > > > > + int flags;
> > > > > > + int segment;
> > > > > > +
> > > > > > + struct mutexreg_lock;
> > > > > > +
> > > > > > + bus_space_tag_t iot;
> > > > > > + bus_space_handle_t ioh;
> > > > > > +
> > > > > > + uint64_t
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
> Date: Tue, 8 Sep 2020 21:43:39 -0500
> From: Jordan Hargrave
>
> Made changes for the iommu_readq -> iommu_read_8 and also now
> dynamically allocate the hwdte for AMD IOMMU.
Some more bits...
> On Fri, Sep 04, 2020 at 09:17:18PM +0200, Mark Kettenis wrote:
> > > Date: Fri, 4 Sep 2020 00:50:44 -0500
> > > From: Jordan Hargrave
> >
> > A few hints below...
> >
> > > > > +
> > > > > +/* Page Table Entry per domain */
> > > > > +static struct ivhd_dte hwdte[65536] __aligned(PAGE_SIZE);
> > > > > +
> > > > > +/* Alias mapping */
> > > > > +#define SID_INVALID 0x8000L
> > > > > +static uint32_t sid_flag[65536];
> > > >
> > > > Can we avoid having these large arrays, or at least allocate them
> > > > dynamically? That would also avoid the explicit alignment which is
> > > > somewhat nasty since it affects the entire kernel.
> > >
> > > OK. But the hwdte does need the 2M area to be all contiguous but it is not
> > > needed for DMAR/Intel. You *can* have up to 8 different device table
> > > entries
> > > though to split up the area.
> >
> > The appropriate interface to use in this context is
> > bus_dmamem_alloc(9). You can specify alignment, and if you set nsegs
> > to 1, you will get memory that is physicaly contiguous.
> >
> > To map the memory into kernel address space you'll need create a map
> > using bus_dmamap_create(9) and map it using bus_dmamem_map(9). Then
> > instead of using pmap_extract(9) you use bus_dmamap_load_raw(9) which
> > then populates the physical addresses.
> >
> > Many of the drivers written by dlg@ define convenience functions to do
> > all these steps, although interestingly enough he tends to use
> > bus_dmamap_load(9) instead of bus_dmamap_load_raw(9) which is
> > sub-optimal.
> >
> > > > > +
> > > > > +struct domain_dev {
> > > > > + int sid;
> > > > > + int sec;
> > > > > + int sub;
> > > > > + TAILQ_ENTRY(domain_dev) link;
> > > > > +};
> > > > > +
> > > > > +struct domain {
> > > > > + struct iommu_softc *iommu;
> > > > > + int did;
> > > > > + int gaw;
> > > > > + struct pte_entry*pte;
> > > > > + paddr_t ptep;
> > > > > + struct bus_dma_tag dmat;
> > > > > + int flag;
> > > > > +
> > > > > + struct mutexexlck;
> > > > > + charexname[32];
> > > > > + struct extent *iovamap;
> > > > > + TAILQ_HEAD(,domain_dev) devices;
> > > > > + TAILQ_ENTRY(domain) link;
> > > > > +};
> > > > > +
> > > > > +#define DOM_DEBUG 0x1
> > > > > +#define DOM_NOMAP 0x2
> > > > > +
> > > > > +struct dmar_devlist {
> > > > > + int type;
> > > > > + int bus;
> > > > > + int ndp;
> > > > > + struct acpidmar_devpath *dp;
> > > > > + TAILQ_ENTRY(dmar_devlist) link;
> > > > > +};
> > > > > +
> > > > > +TAILQ_HEAD(devlist_head, dmar_devlist);
> > > > > +
> > > > > +struct ivhd_devlist {
> > > > > + int start_id;
> > > > > + int end_id;
> > > > > + int cfg;
> > > > > + TAILQ_ENTRY(ivhd_devlist) link;
> > > > > +};
> > > > > +
> > > > > +struct rmrr_softc {
> > > > > + TAILQ_ENTRY(rmrr_softc) link;
> > > > > + struct devlist_head devices;
> > > > > + int segment;
> > > > > + uint64_tstart;
> > > > > + uint64_tend;
> > > > > +};
> > > > > +
> > > > > +struct atsr_softc {
> > > > > + TAILQ_ENTRY(atsr_softc) link;
> > > > > + struct devlist_head devices;
> > > > > + int segment;
> > > > > + int flags;
> > > > > +};
> > > > > +
> > > > > +struct iommu_pic {
> > > > > + struct pic pic;
> > > > > + struct iommu_softc *iommu;
> > > > > +};
> > > > > +
> > > > > +#define IOMMU_FLAGS_CATCHALL 0x1
> > > > > +#define IOMMU_FLAGS_BAD 0x2
> > > > > +#define IOMMU_FLAGS_SUSPEND 0x4
> > > > > +
> > > > > +struct iommu_softc {
> > > > > + TAILQ_ENTRY(iommu_softc)link;
> > > > > + struct devlist_head devices;
> > > > > + int id;
> > > > > + int flags;
> > > > > + int segment;
> > > > > +
> > > > > + struct mutexreg_lock;
> > > > > +
> > > > > + bus_space_tag_t iot;
> > > > > + bus_space_handle_t ioh;
> > > > > +
> > > > > + uint64_tcap;
> > > > > + uint64_tecap;
> > > > > + uint32_tgcmd;
> > > > > +
> > > > > + int mgaw;
> > > > > + int agaw;
> > > > > + int ndoms;
> > > > > +
> > >
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
Made changes for the iommu_readq -> iommu_read_8 and also now dynamically
allocate
the hwdte for AMD IOMMU.
On Fri, Sep 04, 2020 at 09:17:18PM +0200, Mark Kettenis wrote:
> > Date: Fri, 4 Sep 2020 00:50:44 -0500
> > From: Jordan Hargrave
>
> A few hints below...
>
> > > > +
> > > > +/* Page Table Entry per domain */
> > > > +static struct ivhd_dte hwdte[65536] __aligned(PAGE_SIZE);
> > > > +
> > > > +/* Alias mapping */
> > > > +#define SID_INVALID 0x8000L
> > > > +static uint32_t sid_flag[65536];
> > >
> > > Can we avoid having these large arrays, or at least allocate them
> > > dynamically? That would also avoid the explicit alignment which is
> > > somewhat nasty since it affects the entire kernel.
> >
> > OK. But the hwdte does need the 2M area to be all contiguous but it is not
> > needed for DMAR/Intel. You *can* have up to 8 different device table
> > entries
> > though to split up the area.
>
> The appropriate interface to use in this context is
> bus_dmamem_alloc(9). You can specify alignment, and if you set nsegs
> to 1, you will get memory that is physicaly contiguous.
>
> To map the memory into kernel address space you'll need create a map
> using bus_dmamap_create(9) and map it using bus_dmamem_map(9). Then
> instead of using pmap_extract(9) you use bus_dmamap_load_raw(9) which
> then populates the physical addresses.
>
> Many of the drivers written by dlg@ define convenience functions to do
> all these steps, although interestingly enough he tends to use
> bus_dmamap_load(9) instead of bus_dmamap_load_raw(9) which is
> sub-optimal.
>
> > > > +
> > > > +struct domain_dev {
> > > > + int sid;
> > > > + int sec;
> > > > + int sub;
> > > > + TAILQ_ENTRY(domain_dev) link;
> > > > +};
> > > > +
> > > > +struct domain {
> > > > + struct iommu_softc *iommu;
> > > > + int did;
> > > > + int gaw;
> > > > + struct pte_entry*pte;
> > > > + paddr_t ptep;
> > > > + struct bus_dma_tag dmat;
> > > > + int flag;
> > > > +
> > > > + struct mutexexlck;
> > > > + charexname[32];
> > > > + struct extent *iovamap;
> > > > + TAILQ_HEAD(,domain_dev) devices;
> > > > + TAILQ_ENTRY(domain) link;
> > > > +};
> > > > +
> > > > +#define DOM_DEBUG 0x1
> > > > +#define DOM_NOMAP 0x2
> > > > +
> > > > +struct dmar_devlist {
> > > > + int type;
> > > > + int bus;
> > > > + int ndp;
> > > > + struct acpidmar_devpath *dp;
> > > > + TAILQ_ENTRY(dmar_devlist) link;
> > > > +};
> > > > +
> > > > +TAILQ_HEAD(devlist_head, dmar_devlist);
> > > > +
> > > > +struct ivhd_devlist {
> > > > + int start_id;
> > > > + int end_id;
> > > > + int cfg;
> > > > + TAILQ_ENTRY(ivhd_devlist) link;
> > > > +};
> > > > +
> > > > +struct rmrr_softc {
> > > > + TAILQ_ENTRY(rmrr_softc) link;
> > > > + struct devlist_head devices;
> > > > + int segment;
> > > > + uint64_tstart;
> > > > + uint64_tend;
> > > > +};
> > > > +
> > > > +struct atsr_softc {
> > > > + TAILQ_ENTRY(atsr_softc) link;
> > > > + struct devlist_head devices;
> > > > + int segment;
> > > > + int flags;
> > > > +};
> > > > +
> > > > +struct iommu_pic {
> > > > + struct pic pic;
> > > > + struct iommu_softc *iommu;
> > > > +};
> > > > +
> > > > +#define IOMMU_FLAGS_CATCHALL 0x1
> > > > +#define IOMMU_FLAGS_BAD0x2
> > > > +#define IOMMU_FLAGS_SUSPEND0x4
> > > > +
> > > > +struct iommu_softc {
> > > > + TAILQ_ENTRY(iommu_softc)link;
> > > > + struct devlist_head devices;
> > > > + int id;
> > > > + int flags;
> > > > + int segment;
> > > > +
> > > > + struct mutexreg_lock;
> > > > +
> > > > + bus_space_tag_t iot;
> > > > + bus_space_handle_t ioh;
> > > > +
> > > > + uint64_tcap;
> > > > + uint64_tecap;
> > > > + uint32_tgcmd;
> > > > +
> > > > + int mgaw;
> > > > + int agaw;
> > > > + int ndoms;
> > > > +
> > > > + struct root_entry *root;
> > > > + struct context_entry*ctx[256];
> > > > +
> > > > + void*intr;
> > > > + struct iommu_picpic;
> > > > + int
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
> Date: Fri, 4 Sep 2020 00:50:44 -0500
> From: Jordan Hargrave
A few hints below...
> > > +
> > > +/* Page Table Entry per domain */
> > > +static struct ivhd_dte hwdte[65536] __aligned(PAGE_SIZE);
> > > +
> > > +/* Alias mapping */
> > > +#define SID_INVALID 0x8000L
> > > +static uint32_t sid_flag[65536];
> >
> > Can we avoid having these large arrays, or at least allocate them
> > dynamically? That would also avoid the explicit alignment which is
> > somewhat nasty since it affects the entire kernel.
>
> OK. But the hwdte does need the 2M area to be all contiguous but it is not
> needed for DMAR/Intel. You *can* have up to 8 different device table entries
> though to split up the area.
The appropriate interface to use in this context is
bus_dmamem_alloc(9). You can specify alignment, and if you set nsegs
to 1, you will get memory that is physicaly contiguous.
To map the memory into kernel address space you'll need create a map
using bus_dmamap_create(9) and map it using bus_dmamem_map(9). Then
instead of using pmap_extract(9) you use bus_dmamap_load_raw(9) which
then populates the physical addresses.
Many of the drivers written by dlg@ define convenience functions to do
all these steps, although interestingly enough he tends to use
bus_dmamap_load(9) instead of bus_dmamap_load_raw(9) which is
sub-optimal.
> > > +
> > > +struct domain_dev {
> > > + int sid;
> > > + int sec;
> > > + int sub;
> > > + TAILQ_ENTRY(domain_dev) link;
> > > +};
> > > +
> > > +struct domain {
> > > + struct iommu_softc *iommu;
> > > + int did;
> > > + int gaw;
> > > + struct pte_entry*pte;
> > > + paddr_t ptep;
> > > + struct bus_dma_tag dmat;
> > > + int flag;
> > > +
> > > + struct mutexexlck;
> > > + charexname[32];
> > > + struct extent *iovamap;
> > > + TAILQ_HEAD(,domain_dev) devices;
> > > + TAILQ_ENTRY(domain) link;
> > > +};
> > > +
> > > +#define DOM_DEBUG 0x1
> > > +#define DOM_NOMAP 0x2
> > > +
> > > +struct dmar_devlist {
> > > + int type;
> > > + int bus;
> > > + int ndp;
> > > + struct acpidmar_devpath *dp;
> > > + TAILQ_ENTRY(dmar_devlist) link;
> > > +};
> > > +
> > > +TAILQ_HEAD(devlist_head, dmar_devlist);
> > > +
> > > +struct ivhd_devlist {
> > > + int start_id;
> > > + int end_id;
> > > + int cfg;
> > > + TAILQ_ENTRY(ivhd_devlist) link;
> > > +};
> > > +
> > > +struct rmrr_softc {
> > > + TAILQ_ENTRY(rmrr_softc) link;
> > > + struct devlist_head devices;
> > > + int segment;
> > > + uint64_tstart;
> > > + uint64_tend;
> > > +};
> > > +
> > > +struct atsr_softc {
> > > + TAILQ_ENTRY(atsr_softc) link;
> > > + struct devlist_head devices;
> > > + int segment;
> > > + int flags;
> > > +};
> > > +
> > > +struct iommu_pic {
> > > + struct pic pic;
> > > + struct iommu_softc *iommu;
> > > +};
> > > +
> > > +#define IOMMU_FLAGS_CATCHALL 0x1
> > > +#define IOMMU_FLAGS_BAD 0x2
> > > +#define IOMMU_FLAGS_SUSPEND 0x4
> > > +
> > > +struct iommu_softc {
> > > + TAILQ_ENTRY(iommu_softc)link;
> > > + struct devlist_head devices;
> > > + int id;
> > > + int flags;
> > > + int segment;
> > > +
> > > + struct mutexreg_lock;
> > > +
> > > + bus_space_tag_t iot;
> > > + bus_space_handle_t ioh;
> > > +
> > > + uint64_tcap;
> > > + uint64_tecap;
> > > + uint32_tgcmd;
> > > +
> > > + int mgaw;
> > > + int agaw;
> > > + int ndoms;
> > > +
> > > + struct root_entry *root;
> > > + struct context_entry*ctx[256];
> > > +
> > > + void*intr;
> > > + struct iommu_picpic;
> > > + int fedata;
> > > + uint64_tfeaddr;
> > > + uint64_trtaddr;
> > > +
> > > + // Queued Invalidation
> > > + int qi_head;
> > > + int qi_tail;
> > > + paddr_t qip;
> > > + struct qi_entry *qi;
> > > +
> > > + struct domain *unity;
> > > + TAILQ_HEAD(,domain) domains;
> > > +
> > > + // AMD iommu
> > > + struct ivhd_dte *dte;
> > > + void*cmd_tbl;
> > > + void*evt_tbl;
> > > + paddr_t cmd_tblp;
> > > + paddr_t evt_tblp;
> > > + uint64_twv[128] __aligned(4096);
> >
> > This wv array isn't used as far as I can tell.
>
> Ah I was doing some testing on the comman
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
On Thu, Sep 03, 2020 at 09:06:59PM +0200, Mark Kettenis wrote: > > Date: Tue, 1 Sep 2020 17:20:19 -0500 > > From: Jordan Hargrave > > > > [PATCH] Add IOMMU support for Intel VT-d and AMD Vi > > > > This hooks each pci device and overrides bus_dmamap_xxx to issue > > remap of DMA requests to virtual DMA space. It protects devices > > from issuing I/O requests to memory in the system that is outside > > the requested DMA space. > > --- > > sys/arch/amd64/conf/GENERIC |1 + > > sys/arch/amd64/conf/RAMDISK |1 + > > sys/arch/amd64/conf/RAMDISK_CD |1 + > > sys/arch/amd64/include/pci_machdep.h |3 +- > > sys/arch/amd64/pci/pci_machdep.c | 15 +- > > sys/dev/acpi/acpi.c |5 + > > sys/dev/acpi/acpidmar.c | 2988 ++ > > sys/dev/acpi/acpidmar.h | 534 + > > sys/dev/acpi/acpireg.h | 21 +- > > sys/dev/acpi/amd_iommu.h | 358 +++ > > sys/dev/acpi/files.acpi |5 + > > sys/dev/pci/pci.c| 28 + > > sys/dev/pci/pcivar.h |2 + > > 13 files changed, 3959 insertions(+), 3 deletions(-) > > create mode 100644 sys/dev/acpi/acpidmar.c > > create mode 100644 sys/dev/acpi/acpidmar.h > > create mode 100644 sys/dev/acpi/amd_iommu.h > > This needs some further cleanup and style love. But let's leave that > alone for now. > > How much of this code is really shared between DMAR and IVRS? It > would be nice to split it out between those two if we can avoid code > duplication. > Yes that could be possible, and have a common iommu attach function? I wrote that Intel code like 5 years ago... then kinda bolted the AMD stuff on top last year. > iommu_writel(), iommu_readl(), iommu_writeq() etc., are a bit too > Linuxy; iommu_write_4(), iommu_read_4(), iommu_write_8() would be > better names. Fair enough > > I don't fully grasp why you need acpidmar_intr_establish(). I can see > that MSI interrupts from devices behind the IOMMU need to go through > the IOMMU since they're essentially memory transaction. But your code > seems to only deal with the IOMMU's error interrupt. Does the IOMMU > interrupt itself go through the IOMMU as well? > The Intel interrupt is a bit weird. It's not on a PCI device, so pci_map_msi or something similar doesn't work. There isn't a vector number or anything that's provided in the DMAR structure. So I went with what Linux was doing for establishing the fault handler IRQ. > Why do you need to explicitly call acpidmar_sw()? Naively I would > think that you need to call this fairly late, but you call it before > config_suspend_all(DVACT_SUSPEND) happens. Is there a reason why this > can't happen as part of normal config_suspend_all(DVACT_SUSPEND) > processing? I haven't looked at the suspend/resume in a long while. I did have it working on Intel at one point, but only one system worked and years ago. Suspend/Resume doesn't even work at all on my current laptops, even without these patches. > > I think the way you use pci_probe_device_hook() is fine. > > What is the point of having function that start with an underscore? > Feels like another Linux-ism to me... > > A few more random things in the code below... > > > > diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC > > index 2c49f91a1..1eda12bc9 100644 > > --- a/sys/arch/amd64/conf/GENERIC > > +++ b/sys/arch/amd64/conf/GENERIC > > @@ -45,6 +45,7 @@ acpibtn* at acpi? > > acpicpu* at acpi? > > acpicmos* at acpi? > > acpidock* at acpi? > > +acpidmar0 at acpi? > > acpiec*at acpi? > > acpipci* at acpi? > > acpiprt* at acpi? > > diff --git a/sys/arch/amd64/conf/RAMDISK b/sys/arch/amd64/conf/RAMDISK > > index 10148add1..7ab48f32e 100644 > > --- a/sys/arch/amd64/conf/RAMDISK > > +++ b/sys/arch/amd64/conf/RAMDISK > > @@ -34,6 +34,7 @@ acpipci* at acpi? > > acpiprt* at acpi? > > acpimadt0 at acpi? > > #acpitz* at acpi? > > +acpidmar* at acpi? disable > > > > mpbios0at bios0 > > > > diff --git a/sys/arch/amd64/conf/RAMDISK_CD b/sys/arch/amd64/conf/RAMDISK_CD > > index 91022751e..82a24e210 100644 > > --- a/sys/arch/amd64/conf/RAMDISK_CD > > +++ b/sys/arch/amd64/conf/RAMDISK_CD > > @@ -48,6 +48,7 @@ sdhc* at acpi? > > acpihve* at acpi? > > chvgpio*at acpi? > > glkgpio* at acpi? > > +acpidmar* at acpi? disable > > > >
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
> Date: Tue, 1 Sep 2020 17:20:19 -0500 > From: Jordan Hargrave > > [PATCH] Add IOMMU support for Intel VT-d and AMD Vi > > This hooks each pci device and overrides bus_dmamap_xxx to issue > remap of DMA requests to virtual DMA space. It protects devices > from issuing I/O requests to memory in the system that is outside > the requested DMA space. > --- > sys/arch/amd64/conf/GENERIC |1 + > sys/arch/amd64/conf/RAMDISK |1 + > sys/arch/amd64/conf/RAMDISK_CD |1 + > sys/arch/amd64/include/pci_machdep.h |3 +- > sys/arch/amd64/pci/pci_machdep.c | 15 +- > sys/dev/acpi/acpi.c |5 + > sys/dev/acpi/acpidmar.c | 2988 ++ > sys/dev/acpi/acpidmar.h | 534 + > sys/dev/acpi/acpireg.h | 21 +- > sys/dev/acpi/amd_iommu.h | 358 +++ > sys/dev/acpi/files.acpi |5 + > sys/dev/pci/pci.c| 28 + > sys/dev/pci/pcivar.h |2 + > 13 files changed, 3959 insertions(+), 3 deletions(-) > create mode 100644 sys/dev/acpi/acpidmar.c > create mode 100644 sys/dev/acpi/acpidmar.h > create mode 100644 sys/dev/acpi/amd_iommu.h This needs some further cleanup and style love. But let's leave that alone for now. How much of this code is really shared between DMAR and IVRS? It would be nice to split it out between those two if we can avoid code duplication. iommu_writel(), iommu_readl(), iommu_writeq() etc., are a bit too Linuxy; iommu_write_4(), iommu_read_4(), iommu_write_8() would be better names. I don't fully grasp why you need acpidmar_intr_establish(). I can see that MSI interrupts from devices behind the IOMMU need to go through the IOMMU since they're essentially memory transaction. But your code seems to only deal with the IOMMU's error interrupt. Does the IOMMU interrupt itself go through the IOMMU as well? Why do you need to explicitly call acpidmar_sw()? Naively I would think that you need to call this fairly late, but you call it before config_suspend_all(DVACT_SUSPEND) happens. Is there a reason why this can't happen as part of normal config_suspend_all(DVACT_SUSPEND) processing? I think the way you use pci_probe_device_hook() is fine. What is the point of having function that start with an underscore? Feels like another Linux-ism to me... A few more random things in the code below... > diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC > index 2c49f91a1..1eda12bc9 100644 > --- a/sys/arch/amd64/conf/GENERIC > +++ b/sys/arch/amd64/conf/GENERIC > @@ -45,6 +45,7 @@ acpibtn*at acpi? > acpicpu* at acpi? > acpicmos*at acpi? > acpidock*at acpi? > +acpidmar0at acpi? > acpiec* at acpi? > acpipci* at acpi? > acpiprt* at acpi? > diff --git a/sys/arch/amd64/conf/RAMDISK b/sys/arch/amd64/conf/RAMDISK > index 10148add1..7ab48f32e 100644 > --- a/sys/arch/amd64/conf/RAMDISK > +++ b/sys/arch/amd64/conf/RAMDISK > @@ -34,6 +34,7 @@ acpipci*at acpi? > acpiprt* at acpi? > acpimadt0at acpi? > #acpitz* at acpi? > +acpidmar*at acpi? disable > > mpbios0 at bios0 > > diff --git a/sys/arch/amd64/conf/RAMDISK_CD b/sys/arch/amd64/conf/RAMDISK_CD > index 91022751e..82a24e210 100644 > --- a/sys/arch/amd64/conf/RAMDISK_CD > +++ b/sys/arch/amd64/conf/RAMDISK_CD > @@ -48,6 +48,7 @@ sdhc* at acpi? > acpihve* at acpi? > chvgpio*at acpi? > glkgpio* at acpi? > +acpidmar*at acpi? disable > > mpbios0 at bios0 > > diff --git a/sys/arch/amd64/include/pci_machdep.h > b/sys/arch/amd64/include/pci_machdep.h > index bc295cc22..c725bdc73 100644 > --- a/sys/arch/amd64/include/pci_machdep.h > +++ b/sys/arch/amd64/include/pci_machdep.h > @@ -91,7 +91,8 @@ void > *pci_intr_establish_cpu(pci_chipset_tag_t, pci_intr_handle_t, > int, struct cpu_info *, > int (*)(void *), void *, const char *); > void pci_intr_disestablish(pci_chipset_tag_t, void *); > -#define pci_probe_device_hook(c, a) (0) > +int pci_probe_device_hook(pci_chipset_tag_t, > + struct pci_attach_args *); > > void pci_dev_postattach(struct device *, struct > pci_attach_args *); > > diff --git a/sys/arch/amd64/pci/pci_machdep.c > b/sys/arch/amd64/pci/pci_machdep.c > index cf4e835de..b700946a4 100644 > --- a/sys/arch/amd64/pci/pci_machdep.c > +++ b/sys/arch/amd64/pci/pci_machdep.c > @@ -89,6 +89,11 @@ > #include > #endif > > +#include "acpi.h" > +#if NACPI > 0 > +#include > +#endif > + > /*
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
Oh good catch thanks. Weird, it does compile! From: Daniel Dickman Sent: Tuesday, September 1, 2020 11:23 PM To: Jordan Hargrave Cc: tech@openbsd.org Subject: Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi > [PATCH] Add IOMMU support for Intel VT-d and AMD Vi > > This hooks each pci device and overrides bus_dmamap_xxx to issue > remap of DMA requests to virtual DMA space. It protects devices > from issuing I/O requests to memory in the system that is outside > the requested DMA space. Hi Jordan, thanks for working on this. I would like to see iommu support... > + uint64_tefr; > + uint8_t reserved[8]; > +} __packd; ...that being said, is the above a typo?
Re: [PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
> [PATCH] Add IOMMU support for Intel VT-d and AMD Vi > > This hooks each pci device and overrides bus_dmamap_xxx to issue > remap of DMA requests to virtual DMA space. It protects devices > from issuing I/O requests to memory in the system that is outside > the requested DMA space. Hi Jordan, thanks for working on this. I would like to see iommu support... > + uint64_tefr; > + uint8_t reserved[8]; > +} __packd; ...that being said, is the above a typo?
[PATCH] Add IOMMU support for Intel VT-d and AMD-Vi
[PATCH] Add IOMMU support for Intel VT-d and AMD Vi
This hooks each pci device and overrides bus_dmamap_xxx to issue
remap of DMA requests to virtual DMA space. It protects devices
from issuing I/O requests to memory in the system that is outside
the requested DMA space.
---
sys/arch/amd64/conf/GENERIC |1 +
sys/arch/amd64/conf/RAMDISK |1 +
sys/arch/amd64/conf/RAMDISK_CD |1 +
sys/arch/amd64/include/pci_machdep.h |3 +-
sys/arch/amd64/pci/pci_machdep.c | 15 +-
sys/dev/acpi/acpi.c |5 +
sys/dev/acpi/acpidmar.c | 2988 ++
sys/dev/acpi/acpidmar.h | 534 +
sys/dev/acpi/acpireg.h | 21 +-
sys/dev/acpi/amd_iommu.h | 358 +++
sys/dev/acpi/files.acpi |5 +
sys/dev/pci/pci.c| 28 +
sys/dev/pci/pcivar.h |2 +
13 files changed, 3959 insertions(+), 3 deletions(-)
create mode 100644 sys/dev/acpi/acpidmar.c
create mode 100644 sys/dev/acpi/acpidmar.h
create mode 100644 sys/dev/acpi/amd_iommu.h
diff --git a/sys/arch/amd64/conf/GENERIC b/sys/arch/amd64/conf/GENERIC
index 2c49f91a1..1eda12bc9 100644
--- a/sys/arch/amd64/conf/GENERIC
+++ b/sys/arch/amd64/conf/GENERIC
@@ -45,6 +45,7 @@ acpibtn* at acpi?
acpicpu* at acpi?
acpicmos* at acpi?
acpidock* at acpi?
+acpidmar0 at acpi?
acpiec*at acpi?
acpipci* at acpi?
acpiprt* at acpi?
diff --git a/sys/arch/amd64/conf/RAMDISK b/sys/arch/amd64/conf/RAMDISK
index 10148add1..7ab48f32e 100644
--- a/sys/arch/amd64/conf/RAMDISK
+++ b/sys/arch/amd64/conf/RAMDISK
@@ -34,6 +34,7 @@ acpipci* at acpi?
acpiprt* at acpi?
acpimadt0 at acpi?
#acpitz* at acpi?
+acpidmar* at acpi? disable
mpbios0at bios0
diff --git a/sys/arch/amd64/conf/RAMDISK_CD b/sys/arch/amd64/conf/RAMDISK_CD
index 91022751e..82a24e210 100644
--- a/sys/arch/amd64/conf/RAMDISK_CD
+++ b/sys/arch/amd64/conf/RAMDISK_CD
@@ -48,6 +48,7 @@ sdhc* at acpi?
acpihve* at acpi?
chvgpio*at acpi?
glkgpio* at acpi?
+acpidmar* at acpi? disable
mpbios0at bios0
diff --git a/sys/arch/amd64/include/pci_machdep.h
b/sys/arch/amd64/include/pci_machdep.h
index bc295cc22..c725bdc73 100644
--- a/sys/arch/amd64/include/pci_machdep.h
+++ b/sys/arch/amd64/include/pci_machdep.h
@@ -91,7 +91,8 @@ void *pci_intr_establish_cpu(pci_chipset_tag_t,
pci_intr_handle_t,
int, struct cpu_info *,
int (*)(void *), void *, const char *);
void pci_intr_disestablish(pci_chipset_tag_t, void *);
-#definepci_probe_device_hook(c, a) (0)
+intpci_probe_device_hook(pci_chipset_tag_t,
+ struct pci_attach_args *);
void pci_dev_postattach(struct device *, struct pci_attach_args *);
diff --git a/sys/arch/amd64/pci/pci_machdep.c b/sys/arch/amd64/pci/pci_machdep.c
index cf4e835de..b700946a4 100644
--- a/sys/arch/amd64/pci/pci_machdep.c
+++ b/sys/arch/amd64/pci/pci_machdep.c
@@ -89,6 +89,11 @@
#include
#endif
+#include "acpi.h"
+#if NACPI > 0
+#include
+#endif
+
/*
* Memory Mapped Configuration space access.
*
@@ -797,7 +802,15 @@ pci_init_extents(void)
}
}
-#include "acpi.h"
+int
+pci_probe_device_hook(pci_chipset_tag_t pc, struct pci_attach_args *pa)
+{
+#if NACPI > 0
+ acpidmar_pci_hook(pc, pa);
+#endif
+ return 0;
+}
+
#if NACPI > 0
void acpi_pci_match(struct device *, struct pci_attach_args *);
pcireg_t acpi_pci_min_powerstate(pci_chipset_tag_t, pcitag_t);
diff --git a/sys/dev/acpi/acpi.c b/sys/dev/acpi/acpi.c
index a6239198e..ea11483ad 100644
--- a/sys/dev/acpi/acpi.c
+++ b/sys/dev/acpi/acpi.c
@@ -49,6 +49,7 @@
#include
#include
#include
+#include
#include
#include
@@ -2448,6 +2449,8 @@ acpi_sleep_pm(struct acpi_softc *sc, int state)
sc->sc_fadt->pm2_cnt_blk && sc->sc_fadt->pm2_cnt_len)
acpi_write_pmreg(sc, ACPIREG_PM2_CNT, 0, ACPI_PM2_ARB_DIS);
+ acpidmar_sw(DVACT_SUSPEND);
+
/* Write SLP_TYPx values */
rega = acpi_read_pmreg(sc, ACPIREG_PM1A_CNT, 0);
regb = acpi_read_pmreg(sc, ACPIREG_PM1B_CNT, 0);
@@ -2483,6 +2486,8 @@ acpi_resume_pm(struct acpi_softc *sc, int fromstate)
{
uint16_t rega, regb, en;
+ acpidmar_sw(DVACT_RESUME);
+
/* Write SLP_TYPx values */
rega = acpi_read_pmreg(sc, ACPIREG_PM1A_CNT, 0);
regb = acpi_read_pmreg(sc, ACPIREG_PM1B_CNT, 0);
diff --git a/sys/dev/acpi/acpidmar.c b/sys/dev/acpi/acpidmar.c
new file mode 100644
index 0..48506e1b1
--- /dev/null
+++ b/sys/dev/acpi/acpidmar.c
@@ -0,0 +1,2988 @@
+/*
+ * Copyright (c) 2015 Jordan Hargrave
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is here
