Page MenuHomeFreeBSD
Differential D26705

Manage MSI iommu pages
ClosedPublic
Actions

Authored by br on Oct 7 2020, 2:18 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jan 25, 7:39 PM
Unknown Object (File)
Wed, Jan 22, 5:21 PM
Unknown Object (File)
Wed, Jan 22, 2:34 PM
Unknown Object (File)
Tue, Jan 21, 6:57 PM
Unknown Object (File)
Tue, Jan 21, 6:05 PM
Unknown Object (File)
Sun, Jan 19, 12:33 PM
Unknown Object (File)
Sun, Jan 19, 12:30 PM
Unknown Object (File)
Sat, Jan 18, 10:24 PM
View All Files
Subscribers
imp

Details

Reviewers
kib
andrew
Commits
rS366833: Manage MSI iommu pages.
Summary

This allows the interrupt controller driver only need a small change to create a map for the page the device will write to raise an interrupt.

o Split-out GAS macroses to a separate header iommu_gas.h
o Add iommu_msi.h

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

br requested review of this revision.Oct 7 2020, 2:18 PM
br created this revision.
kib added a comment.Oct 7 2020, 4:49 PM

Convention for var.h is not suitable for your case IMO. We have signal.h and signalvar.h, where signal.h is usable by userspace and signalvar.h is for kernel private stuff.

Can you provide some enumeration of stuff you want to keep in iommu.h, and an explanation of why ?

br added a comment.Oct 7 2020, 7:34 PM
In D26705#595130, @kib wrote:

Convention for var.h is not suitable for your case IMO. We have signal.h and signalvar.h, where signal.h is usable by userspace and signalvar.h is for kernel private stuff.

Can you provide some enumeration of stuff you want to keep in iommu.h, and an explanation of why ?

I'm not insisting on this, Andrew wanted to include iommu.h from subr_intr.c, but he does not want to include pci_reg.h and pci_var.h as dependencies for iommu.h, so a split requested.

There are a few new prototypes needed from iommu.h to manage MSI iommu pages:
int iommu_map_msi(struct iommu_ctx *ctx, iommu_gaddr_t size, int offset, u_int eflags, u_int flags, vm_page_t *ma);
void iommu_translate_msi(struct iommu_domain *domain, uint64_t *addr);
struct iommu_domain *iommu_get_ctx_domain(struct iommu_ctx *ctx);
struct iommu_ctx *iommu_get_dev_ctx(device_t dev);

See for details https://reviews.freebsd.org/D24618

kib added a comment.Oct 7 2020, 7:50 PM

So would adding a header iommu_msi.h with just msi-related prototypes enough ? I suspect that x86/msi.c can also get rid of iommu.h then.

andrew added a comment.Oct 8 2020, 8:33 AM

I'd like to use some of the iommu functions in device code, e.g. non-pci devices on arm64 can be behind an iommu. I'd prefer to not have to teach these devices about PCI just to configure the iommu.

br added a comment.Oct 8 2020, 11:54 AM
In D26705#595261, @kib wrote:

So would adding a header iommu_msi.h with just msi-related prototypes enough ? I suspect that x86/msi.c can also get rid of iommu.h then.

I will try and see if it works

br updated this revision to Diff 78014.Oct 8 2020, 2:53 PM
br retitled this revision from Split iommu.h to Manage MSI iommu pages.
br edited the summary of this revision. (Show Details)

Manage MSI iommu pages

br added a comment.Oct 8 2020, 2:54 PM
In D26705#595414, @br wrote:
In D26705#595261, @kib wrote:

So would adding a header iommu_msi.h with just msi-related prototypes enough ? I suspect that x86/msi.c can also get rid of iommu.h then.

I will try and see if it works

It works but not sure what to do with typedef iommu_haddr_t ? I put it to sys/types.h since it is needed by iommu_msi.h and iommu.h

br updated this revision to Diff 78015.Oct 8 2020, 3:19 PM

context added

kib added inline comments.Oct 9 2020, 4:37 AM
sys/dev/iommu/busdma_iommu.c
275

I suggest to commit iommu_get_dev_ctx() addition right away.

893

I think there should be per-arch define like IOMMU_DOMAIN_UNLOAD_SLEEP, instead of listing arches directly.

sys/sys/types.h
443

This contamination of standard header is forbidden . May be add some header in dev/iommu, for instance iommu_types.h.

br updated this revision to Diff 78161.Oct 13 2020, 1:14 PM

Address kib's notes:

  • add iommu_types.h
  • add md macro IOMMU_UNLOAD_SLEEP
kib added inline comments.Oct 13 2020, 10:32 PM
sys/dev/iommu/busdma_iommu.c
64

What the gas.h header is need ? busdma_iommu.h includes it.

893

#ifndef

sys/dev/iommu/iommu.h
35

Please commit this separately.

sys/dev/iommu/iommu_gas.c
214

Commit this separately.

sys/dev/iommu/iommu_types.h
42 ↗(On Diff #78161)

Can you extract iommu_types.h addition into a separate patch ?

br updated this revision to Diff 78234.Oct 14 2020, 9:53 PM

Address kibs's notes

br updated this revision to Diff 78260.Oct 15 2020, 2:08 PM

Regenerate

br added inline comments.Oct 15 2020, 4:01 PM
sys/dev/iommu/busdma_iommu.c
64

Not needed, thanks!

sys/dev/iommu/iommu_gas.c
214

Committed!

sys/dev/iommu/iommu_types.h
42 ↗(On Diff #78161)

Yes, extracted and committed.

kib added a comment.Oct 18 2020, 6:02 AM

I am curious. There is no arch restriction on the value of MSI base address ? System can map MSI page anywhere, and just programming the correct address into MSI address register would make it work ?

This is very non-x86ish.

sys/dev/iommu/iommu.h
114

So these three fields are in fact MD ?

sys/dev/iommu/iommu_msi.h
8

Really ?

47

I do not think that this is an appropriate header for get_ctx_domain() and get_dev_cxt().

sys/kern/msi_if.m
81

This #ifdef makes the header correct usage critically depend on the previous include of opt_iommu.h. I do not see any problem to declare the method unconditionally.

br updated this revision to Diff 78394.Oct 18 2020, 8:29 PM

o Put Andrew's copyright
o Remove ifdef IOMMU

br added a comment.Oct 18 2020, 8:39 PM
In D26705#598296, @kib wrote:

I am curious. There is no arch restriction on the value of MSI base address ? System can map MSI page anywhere, and just programming the correct address into MSI address register would make it work ?

This is very non-x86ish.

No restrictions. MSI address could be anywhere

sys/dev/iommu/iommu.h
114

Since you don't have these on x86, then yes

sys/dev/iommu/iommu_msi.h
8

I put Andrew's copyright since he wrote this part

47

Agree

sys/kern/msi_if.m
81

Ok, removed

br updated this revision to Diff 78395.Oct 18 2020, 8:40 PM

o Move iommu_get_ctx_domain(), iommu_get_dev_ctx() to iommu.h

br updated this revision to Diff 78396.Oct 18 2020, 8:42 PM

Regenerate

kib added inline comments.Oct 18 2020, 9:12 PM
sys/dev/iommu/iommu.h
114

Then mark them so in comments.

sys/dev/iommu/iommu_gas.c
782

Don't you want to assert that address is below the msi_entry end ? In fact, address + 8.

br updated this revision to Diff 78417.Oct 19 2020, 10:20 AM

o Add a KASSERT
o Mark MSI fields in the iommu_domain as arch-specific

br added inline comments.Oct 19 2020, 10:21 AM
sys/dev/iommu/iommu.h
114

Maked

sys/dev/iommu/iommu_gas.c
782

Added

kib accepted this revision.Oct 19 2020, 11:06 AM
kib added inline comments.
sys/dev/iommu/busdma_iommu.c
63

Why this header is needed for busdma_iommu.c ?

sys/dev/iommu/iommu.h
112

Look at x86/iommu/intel_ctx.c:dmaor_domain_alloc(), the line under the
'Disable local apic region access'. I suggest to assign the resulting entry to msi_entry, as a follow-up commit.

This revision is now accepted and ready to land.Oct 19 2020, 11:06 AM
br added inline comments.Oct 19 2020, 1:09 PM
sys/dev/iommu/busdma_iommu.c
63

Not needed, thanks

Closed by commit rS366833: Manage MSI iommu pages. (authored by br). · Explain WhyOct 19 2020, 1:10 PM
This revision was automatically updated to reflect the committed changes.
br added a commit: rS366833: Manage MSI iommu pages..
Herald added a subscriber: imp. · View Herald TranscriptOct 19 2020, 1:10 PM

Revision Contents
Changeset List

PathSize
sys/
dev/
iommu/
1 line
30 lines
37 lines
60 lines
68 lines
47 lines
kern/
13 lines
46 lines
sys/
2 lines
5 lines

Diff 78015

View Options

sys/dev/iommu/busdma_iommu.h

Loading...
View Options

sys/dev/iommu/busdma_iommu.c

Loading...
View Options

sys/dev/iommu/iommu.h

Loading...
View Options

sys/dev/iommu/iommu_gas.h

Loading...
View Options

sys/dev/iommu/iommu_gas.c

Loading...
View Options

sys/dev/iommu/iommu_msi.h

Loading...
View Options

sys/kern/msi_if.m

Loading...
View Options

sys/kern/subr_intr.c

Loading...
View Options

sys/sys/intr.h

Loading...
View Options

sys/sys/types.h

Loading...