| Message ID | 20220222114711.19209-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On 22.02.2022 12:47, Andrew Cooper wrote: > Most IOMMU hooks are already altcall for performance reasons. Convert the > rest of them so we can harden all the hooks in Control Flow Integrity > configurations. This necessitates the use of iommu_{v,}call() in debug builds > too. Switch to using an ASSERT() as all forms should resolve to &iommu_ops. > > Move the root iommu_ops from __read_mostly to __ro_after_init now that the > latter exists. > > Since c/s 3330013e6739 ("VT-d / x86: re-arrange cache syncing"), vtd_ops is > not modified and doesn't need a forward declaration, so we can use > __initconst_cf_clobber for both VT-d and AMD. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
On 22.02.2022 12:47, Andrew Cooper wrote: > --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c > +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c > @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) > hd->arch.amd.paging_mode, 0, 0); > } > > -static const struct iommu_ops __initconstrel _iommu_ops = { > +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { Following my initcall related remark on x86'es time.c I'm afraid I don't see how this and ... > @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) > return rc; > } > > -static struct iommu_ops __initdata vtd_ops = { > +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { ... this actually works. But I guess I must be overlooking something, as I'm sure that you did test the change. Both ops structures reference a function, through .adjust_irq_affinities, which isn't __init but which is used (besides here) for an initcall. With the ENDBR removed by the time initcalls are run, these should cause #CP. Jan
On 25/02/2022 08:24, Jan Beulich wrote: > On 22.02.2022 12:47, Andrew Cooper wrote: >> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c >> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c >> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) >> hd->arch.amd.paging_mode, 0, 0); >> } >> >> -static const struct iommu_ops __initconstrel _iommu_ops = { >> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { > Following my initcall related remark on x86'es time.c I'm afraid I don't > see how this and ... > >> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) >> return rc; >> } >> >> -static struct iommu_ops __initdata vtd_ops = { >> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { > ... this actually works. But I guess I must be overlooking something, as > I'm sure that you did test the change. > > Both ops structures reference a function, through .adjust_irq_affinities, > which isn't __init but which is used (besides here) for an initcall. With > the ENDBR removed by the time initcalls are run, these should cause #CP. This doesn't explode because the indirect calls are resolved to direct calls before the ENDBR's are clobbered to NOP4. But I really do need to write a proper sphinx doc explaining how this works and the safety considerations. ~Andrew
On 01.03.2022 15:58, Andrew Cooper wrote: > On 25/02/2022 08:24, Jan Beulich wrote: >> On 22.02.2022 12:47, Andrew Cooper wrote: >>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c >>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c >>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) >>> hd->arch.amd.paging_mode, 0, 0); >>> } >>> >>> -static const struct iommu_ops __initconstrel _iommu_ops = { >>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { >> Following my initcall related remark on x86'es time.c I'm afraid I don't >> see how this and ... >> >>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) >>> return rc; >>> } >>> >>> -static struct iommu_ops __initdata vtd_ops = { >>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { >> ... this actually works. But I guess I must be overlooking something, as >> I'm sure that you did test the change. >> >> Both ops structures reference a function, through .adjust_irq_affinities, >> which isn't __init but which is used (besides here) for an initcall. With >> the ENDBR removed by the time initcalls are run, these should cause #CP. > > This doesn't explode because the indirect calls are resolved to direct > calls before the ENDBR's are clobbered to NOP4. I'm afraid I don't understand: The problematic call is in do_initcalls(): for ( call = __presmp_initcall_end; call < __initcall_end; call++ ) (*call)(); I don't see how this could be converted to a direct call. Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls() is called immediately ahead of alternative_branches(). Isn't this (previously?) working related to your "x86/spec-ctrl: Disable retpolines with CET-IBT"? With full retpoline, there wouldn't be an indirect branch, but RET. But with JMP or LFENCE thunks this ought to fault (already before depending on thunk selection, but unconditionally now that your change was committed), I would think. Jan
On 02/03/2022 08:10, Jan Beulich wrote: > On 01.03.2022 15:58, Andrew Cooper wrote: >> On 25/02/2022 08:24, Jan Beulich wrote: >>> On 22.02.2022 12:47, Andrew Cooper wrote: >>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c >>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c >>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) >>>> hd->arch.amd.paging_mode, 0, 0); >>>> } >>>> >>>> -static const struct iommu_ops __initconstrel _iommu_ops = { >>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { >>> Following my initcall related remark on x86'es time.c I'm afraid I don't >>> see how this and ... >>> >>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) >>>> return rc; >>>> } >>>> >>>> -static struct iommu_ops __initdata vtd_ops = { >>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { >>> ... this actually works. But I guess I must be overlooking something, as >>> I'm sure that you did test the change. >>> >>> Both ops structures reference a function, through .adjust_irq_affinities, >>> which isn't __init but which is used (besides here) for an initcall. With >>> the ENDBR removed by the time initcalls are run, these should cause #CP. >> This doesn't explode because the indirect calls are resolved to direct >> calls before the ENDBR's are clobbered to NOP4. > I'm afraid I don't understand: The problematic call is in do_initcalls(): > > for ( call = __presmp_initcall_end; call < __initcall_end; call++ ) > (*call)(); > > I don't see how this could be converted to a direct call. Oh. iov_adjust_irq_affinities()'s double use is hiding here. The safety rule for cf_clobber is that there must not be any non-alt-called callers. We need to fix it: diff --git a/xen/drivers/passthrough/amd/iommu_init.c b/xen/drivers/passthrough/amd/iommu_init.c index 657c7f619a51..b1af5085efda 100644 --- a/xen/drivers/passthrough/amd/iommu_init.c +++ b/xen/drivers/passthrough/amd/iommu_init.c @@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void) return 0; } -__initcall(iov_adjust_irq_affinities); + +int cf_check __init initcall_iov_adjust_irq_affinities(void) +{ + return iommu_call(&iommu_ops, adjust_irq_affinities); +} +__initcall(initcall_iov_adjust_irq_affinities); /* * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall Translations) > Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls() > is called immediately ahead of alternative_branches(). > > Isn't this (previously?) working related to your "x86/spec-ctrl: Disable > retpolines with CET-IBT"? No. It's because AMD CPUs don't have CET-IBT at this juncture, and will never encounter a faulting situation. This is exactly what the UD1 adjustment in Linux are intended to spot, because that would cause all AMD hardware to explode, not just the IBT-enabled ones. ~Andrew
On 02.03.2022 11:12, Andrew Cooper wrote: > On 02/03/2022 08:10, Jan Beulich wrote: >> On 01.03.2022 15:58, Andrew Cooper wrote: >>> On 25/02/2022 08:24, Jan Beulich wrote: >>>> On 22.02.2022 12:47, Andrew Cooper wrote: >>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c >>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c >>>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) >>>>> hd->arch.amd.paging_mode, 0, 0); >>>>> } >>>>> >>>>> -static const struct iommu_ops __initconstrel _iommu_ops = { >>>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { >>>> Following my initcall related remark on x86'es time.c I'm afraid I don't >>>> see how this and ... >>>> >>>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) >>>>> return rc; >>>>> } >>>>> >>>>> -static struct iommu_ops __initdata vtd_ops = { >>>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { >>>> ... this actually works. But I guess I must be overlooking something, as >>>> I'm sure that you did test the change. >>>> >>>> Both ops structures reference a function, through .adjust_irq_affinities, >>>> which isn't __init but which is used (besides here) for an initcall. With >>>> the ENDBR removed by the time initcalls are run, these should cause #CP. >>> This doesn't explode because the indirect calls are resolved to direct >>> calls before the ENDBR's are clobbered to NOP4. >> I'm afraid I don't understand: The problematic call is in do_initcalls(): >> >> for ( call = __presmp_initcall_end; call < __initcall_end; call++ ) >> (*call)(); >> >> I don't see how this could be converted to a direct call. > > Oh. iov_adjust_irq_affinities()'s double use is hiding here. > > The safety rule for cf_clobber is that there must not be any > non-alt-called callers. We need to fix it: > > diff --git a/xen/drivers/passthrough/amd/iommu_init.c > b/xen/drivers/passthrough/amd/iommu_init.c > index 657c7f619a51..b1af5085efda 100644 > --- a/xen/drivers/passthrough/amd/iommu_init.c > +++ b/xen/drivers/passthrough/amd/iommu_init.c > @@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void) > > return 0; > } > -__initcall(iov_adjust_irq_affinities); > + > +int cf_check __init initcall_iov_adjust_irq_affinities(void) > +{ > + return iommu_call(&iommu_ops, adjust_irq_affinities); > +} > +__initcall(initcall_iov_adjust_irq_affinities); > > /* > * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall > Translations) > > >> Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls() >> is called immediately ahead of alternative_branches(). >> >> Isn't this (previously?) working related to your "x86/spec-ctrl: Disable >> retpolines with CET-IBT"? > > No. It's because AMD CPUs don't have CET-IBT at this juncture, and will > never encounter a faulting situation. I'm still lost. An exactly matching construct exists in VT-d code (and my initial comment also was on VT-d). The AMD one is actually a clone of that much older one. The initcall really wants to move to vendor independent code, but I'd still like to understand why no fault was ever observed. Jan
On 02/03/2022 10:34, Jan Beulich wrote: > On 02.03.2022 11:12, Andrew Cooper wrote: >> On 02/03/2022 08:10, Jan Beulich wrote: >>> On 01.03.2022 15:58, Andrew Cooper wrote: >>>> On 25/02/2022 08:24, Jan Beulich wrote: >>>>> On 22.02.2022 12:47, Andrew Cooper wrote: >>>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c >>>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c >>>>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) >>>>>> hd->arch.amd.paging_mode, 0, 0); >>>>>> } >>>>>> >>>>>> -static const struct iommu_ops __initconstrel _iommu_ops = { >>>>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { >>>>> Following my initcall related remark on x86'es time.c I'm afraid I don't >>>>> see how this and ... >>>>> >>>>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) >>>>>> return rc; >>>>>> } >>>>>> >>>>>> -static struct iommu_ops __initdata vtd_ops = { >>>>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { >>>>> ... this actually works. But I guess I must be overlooking something, as >>>>> I'm sure that you did test the change. >>>>> >>>>> Both ops structures reference a function, through .adjust_irq_affinities, >>>>> which isn't __init but which is used (besides here) for an initcall. With >>>>> the ENDBR removed by the time initcalls are run, these should cause #CP. >>>> This doesn't explode because the indirect calls are resolved to direct >>>> calls before the ENDBR's are clobbered to NOP4. >>> I'm afraid I don't understand: The problematic call is in do_initcalls(): >>> >>> for ( call = __presmp_initcall_end; call < __initcall_end; call++ ) >>> (*call)(); >>> >>> I don't see how this could be converted to a direct call. >> Oh. iov_adjust_irq_affinities()'s double use is hiding here. >> >> The safety rule for cf_clobber is that there must not be any >> non-alt-called callers. We need to fix it: >> >> diff --git a/xen/drivers/passthrough/amd/iommu_init.c >> b/xen/drivers/passthrough/amd/iommu_init.c >> index 657c7f619a51..b1af5085efda 100644 >> --- a/xen/drivers/passthrough/amd/iommu_init.c >> +++ b/xen/drivers/passthrough/amd/iommu_init.c >> @@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void) >> >> return 0; >> } >> -__initcall(iov_adjust_irq_affinities); >> + >> +int cf_check __init initcall_iov_adjust_irq_affinities(void) >> +{ >> + return iommu_call(&iommu_ops, adjust_irq_affinities); >> +} >> +__initcall(initcall_iov_adjust_irq_affinities); >> >> /* >> * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall >> Translations) >> >> >>> Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls() >>> is called immediately ahead of alternative_branches(). >>> >>> Isn't this (previously?) working related to your "x86/spec-ctrl: Disable >>> retpolines with CET-IBT"? >> No. It's because AMD CPUs don't have CET-IBT at this juncture, and will >> never encounter a faulting situation. > I'm still lost. An exactly matching construct exists in VT-d code (and > my initial comment also was on VT-d). The AMD one is actually a clone > of that much older one. The initcall really wants to move to vendor > independent code, but I'd still like to understand why no fault was > ever observed. Lovely. It's got a vtd infix which is why it escaped my grep. And yes, I really would expect that to explode on my test system... ~Andrew
On 02/03/2022 13:39, Andrew Cooper wrote: > On 02/03/2022 10:34, Jan Beulich wrote: >> On 02.03.2022 11:12, Andrew Cooper wrote: >>> On 02/03/2022 08:10, Jan Beulich wrote: >>>> On 01.03.2022 15:58, Andrew Cooper wrote: >>>>> On 25/02/2022 08:24, Jan Beulich wrote: >>>>>> On 22.02.2022 12:47, Andrew Cooper wrote: >>>>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c >>>>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c >>>>>>> @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) >>>>>>> hd->arch.amd.paging_mode, 0, 0); >>>>>>> } >>>>>>> >>>>>>> -static const struct iommu_ops __initconstrel _iommu_ops = { >>>>>>> +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { >>>>>> Following my initcall related remark on x86'es time.c I'm afraid I don't >>>>>> see how this and ... >>>>>> >>>>>>> @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) >>>>>>> return rc; >>>>>>> } >>>>>>> >>>>>>> -static struct iommu_ops __initdata vtd_ops = { >>>>>>> +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { >>>>>> ... this actually works. But I guess I must be overlooking something, as >>>>>> I'm sure that you did test the change. >>>>>> >>>>>> Both ops structures reference a function, through .adjust_irq_affinities, >>>>>> which isn't __init but which is used (besides here) for an initcall. With >>>>>> the ENDBR removed by the time initcalls are run, these should cause #CP. >>>>> This doesn't explode because the indirect calls are resolved to direct >>>>> calls before the ENDBR's are clobbered to NOP4. >>>> I'm afraid I don't understand: The problematic call is in do_initcalls(): >>>> >>>> for ( call = __presmp_initcall_end; call < __initcall_end; call++ ) >>>> (*call)(); >>>> >>>> I don't see how this could be converted to a direct call. >>> Oh. iov_adjust_irq_affinities()'s double use is hiding here. >>> >>> The safety rule for cf_clobber is that there must not be any >>> non-alt-called callers. We need to fix it: >>> >>> diff --git a/xen/drivers/passthrough/amd/iommu_init.c >>> b/xen/drivers/passthrough/amd/iommu_init.c >>> index 657c7f619a51..b1af5085efda 100644 >>> --- a/xen/drivers/passthrough/amd/iommu_init.c >>> +++ b/xen/drivers/passthrough/amd/iommu_init.c >>> @@ -831,7 +831,12 @@ int cf_check iov_adjust_irq_affinities(void) >>> >>> return 0; >>> } >>> -__initcall(iov_adjust_irq_affinities); >>> + >>> +int cf_check __init initcall_iov_adjust_irq_affinities(void) >>> +{ >>> + return iommu_call(&iommu_ops, adjust_irq_affinities); >>> +} >>> +__initcall(initcall_iov_adjust_irq_affinities); >>> >>> /* >>> * Family15h Model 10h-1fh erratum 746 (IOMMU Logging May Stall >>> Translations) >>> >>> >>>> Afaics only pre-SMP initcalls are safe in this regard: do_presmp_initcalls() >>>> is called immediately ahead of alternative_branches(). >>>> >>>> Isn't this (previously?) working related to your "x86/spec-ctrl: Disable >>>> retpolines with CET-IBT"? >>> No. It's because AMD CPUs don't have CET-IBT at this juncture, and will >>> never encounter a faulting situation. >> I'm still lost. An exactly matching construct exists in VT-d code (and >> my initial comment also was on VT-d). The AMD one is actually a clone >> of that much older one. The initcall really wants to move to vendor >> independent code, but I'd still like to understand why no fault was >> ever observed. > Lovely. It's got a vtd infix which is why it escaped my grep. > > And yes, I really would expect that to explode on my test system... And the answer is that the linker script collects .init.rodata.* ahead of the dedicated .init.rodata.cf_clobber section. Meaning that __initdata_cf_clobber works as expected but __initconst_cf_clobber is a no-op. ~Andrew
diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h index 8a96ba1f097f..d38c33408766 100644 --- a/xen/arch/x86/include/asm/iommu.h +++ b/xen/arch/x86/include/asm/iommu.h @@ -72,18 +72,16 @@ struct arch_iommu extern struct iommu_ops iommu_ops; -#ifdef NDEBUG # include <asm/alternative.h> # define iommu_call(ops, fn, args...) ({ \ - (void)(ops); \ + ASSERT((ops) == &iommu_ops); \ alternative_call(iommu_ops.fn, ## args); \ }) # define iommu_vcall(ops, fn, args...) ({ \ - (void)(ops); \ + ASSERT((ops) == &iommu_ops); \ alternative_vcall(iommu_ops.fn, ## args); \ }) -#endif static inline const struct iommu_ops *iommu_get_ops(void) { @@ -106,7 +104,7 @@ int iommu_setup_hpet_msi(struct msi_desc *); static inline int iommu_adjust_irq_affinities(void) { return iommu_ops.adjust_irq_affinities - ? iommu_ops.adjust_irq_affinities() + ? iommu_call(&iommu_ops, adjust_irq_affinities) : 0; } @@ -122,7 +120,7 @@ int iommu_enable_x2apic(void); static inline void iommu_disable_x2apic(void) { if ( x2apic_enabled && iommu_ops.disable_x2apic ) - iommu_ops.disable_x2apic(); + iommu_vcall(&iommu_ops, disable_x2apic); } int iommu_identity_mapping(struct domain *d, p2m_access_t p2ma, diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index e57f555d00d1..4b59a4efe9b6 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -628,7 +628,7 @@ static void cf_check amd_dump_page_tables(struct domain *d) hd->arch.amd.paging_mode, 0, 0); } -static const struct iommu_ops __initconstrel _iommu_ops = { +static const struct iommu_ops __initconst_cf_clobber _iommu_ops = { .init = amd_iommu_domain_init, .hwdom_init = amd_iommu_hwdom_init, .quarantine_init = amd_iommu_quarantine_init, diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c index e220fea72c2f..c6b2c384d1dd 100644 --- a/xen/drivers/passthrough/iommu.c +++ b/xen/drivers/passthrough/iommu.c @@ -540,7 +540,7 @@ int __init iommu_setup(void) int iommu_suspend() { if ( iommu_enabled ) - return iommu_get_ops()->suspend(); + return iommu_call(iommu_get_ops(), suspend); return 0; } @@ -548,7 +548,7 @@ int iommu_suspend() void iommu_resume() { if ( iommu_enabled ) - iommu_get_ops()->resume(); + iommu_vcall(iommu_get_ops(), resume); } int iommu_do_domctl( @@ -578,7 +578,8 @@ void iommu_crash_shutdown(void) return; if ( iommu_enabled ) - iommu_get_ops()->crash_shutdown(); + iommu_vcall(iommu_get_ops(), crash_shutdown); + iommu_enabled = false; #ifndef iommu_intremap iommu_intremap = iommu_intremap_off; diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 56968a06a100..6a65ba1d8271 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -56,7 +56,6 @@ bool __read_mostly iommu_snoop = true; static unsigned int __read_mostly nr_iommus; -static struct iommu_ops vtd_ops; static struct tasklet vtd_fault_tasklet; static int cf_check setup_hwdom_device(u8 devfn, struct pci_dev *); @@ -2794,7 +2793,7 @@ static int __init cf_check intel_iommu_quarantine_init(struct domain *d) return rc; } -static struct iommu_ops __initdata vtd_ops = { +static const struct iommu_ops __initconst_cf_clobber vtd_ops = { .init = intel_iommu_domain_init, .hwdom_init = intel_iommu_hwdom_init, .quarantine_init = intel_iommu_quarantine_init, diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index ad5f44e13d98..58a422fb5f88 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -27,7 +27,7 @@ #include <asm/setup.h> const struct iommu_init_ops *__initdata iommu_init_ops; -struct iommu_ops __read_mostly iommu_ops; +struct iommu_ops __ro_after_init iommu_ops; bool __read_mostly iommu_non_coherent; enum iommu_intremap __read_mostly iommu_intremap = iommu_intremap_full; @@ -129,7 +129,7 @@ int iommu_enable_x2apic(void) if ( !iommu_ops.enable_x2apic ) return -EOPNOTSUPP; - return iommu_ops.enable_x2apic(); + return iommu_call(&iommu_ops, enable_x2apic); } void iommu_update_ire_from_apic(
Most IOMMU hooks are already altcall for performance reasons. Convert the rest of them so we can harden all the hooks in Control Flow Integrity configurations. This necessitates the use of iommu_{v,}call() in debug builds too. Switch to using an ASSERT() as all forms should resolve to &iommu_ops. Move the root iommu_ops from __read_mostly to __ro_after_init now that the latter exists. Since c/s 3330013e6739 ("VT-d / x86: re-arrange cache syncing"), vtd_ops is not modified and doesn't need a forward declaration, so we can use __initconst_cf_clobber for both VT-d and AMD. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> v2.1: * New v2.2: * Add ASSERT(). Fix indirection of the passed pointer in enable_x2apic(). * Adjust commit message. --- xen/arch/x86/include/asm/iommu.h | 10 ++++------ xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +- xen/drivers/passthrough/iommu.c | 7 ++++--- xen/drivers/passthrough/vtd/iommu.c | 3 +-- xen/drivers/passthrough/x86/iommu.c | 4 ++-- 5 files changed, 12 insertions(+), 14 deletions(-)