| Message ID | 20230104084502.61734-7-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make x86 IOMMU driver support configurable | expand |
On 04.01.2023 09:45, Xenia Ragiadakou wrote: > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2143,6 +2143,14 @@ static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec) > return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc); > } > > +static int cf_check vmx_pi_update_irte(const struct vcpu *v, > + const struct pirq *pirq, uint8_t gvec) > +{ > + const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL; > + > + return pi_update_irte(pi_desc, pirq, gvec); > +} This being the only caller of pi_update_irte(), I don't see the point in having the extra wrapper. Adjust pi_update_irte() such that it can be used as the intended hook directly. Plus perhaps prefix it with vtd_. > @@ -2591,6 +2599,8 @@ static struct hvm_function_table __initdata_cf_clobber vmx_function_table = { > .tsc_scaling = { > .max_ratio = VMX_TSC_MULTIPLIER_MAX, > }, > + > + .pi_update_irte = vmx_pi_update_irte, You want to install this hook only when iommu_intpost (i.e. the only case when it can actually be called, and only when INTEL_IOMMU=y (avoiding the need for an inline stub of pi_update_irte() or whatever its final name is going to be. > @@ -250,6 +252,9 @@ struct hvm_function_table { > /* Architecture function to setup TSC scaling ratio */ > void (*setup)(struct vcpu *v); > } tsc_scaling; > + > + int (*pi_update_irte)(const struct vcpu *v, > + const struct pirq *pirq, uint8_t gvec); > }; Please can this be moved higher up, e.g. next to . > @@ -774,6 +779,16 @@ static inline void hvm_set_nonreg_state(struct vcpu *v, > alternative_vcall(hvm_funcs.set_nonreg_state, v, nrs); > } > > +static inline int hvm_pi_update_irte(const struct vcpu *v, > + const struct pirq *pirq, uint8_t gvec) > +{ > + if ( hvm_funcs.pi_update_irte ) > + return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec); > + > + return -EOPNOTSUPP; I don't think the conditional is needed, at least not with the other suggested adjustments. Plus the way alternative patching works, a NULL hook will be converted to some equivalent of BUG() anyway, so ASSERT_UNREACHABLE() should also be unnecessary. > +} > + > + > #else /* CONFIG_HVM */ Please don't add double blank lines. > --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h > +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h > @@ -146,6 +146,17 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc) > clear_bit(POSTED_INTR_SN, &pi_desc->control); > } > > +#ifdef CONFIG_INTEL_IOMMU > +int pi_update_irte(const struct pi_desc *pi_desc, > + const struct pirq *pirq, const uint8_t gvec); > +#else > +static inline int pi_update_irte(const struct pi_desc *pi_desc, > + const struct pirq *pirq, const uint8_t gvec) > +{ > + return -EOPNOTSUPP; > +} > +#endif This still is a VT-d function, so I think its declaration would better remain in asm/iommu.h. Jan
On 12.01.2023 13:16, Jan Beulich wrote: > On 04.01.2023 09:45, Xenia Ragiadakou wrote: >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -2143,6 +2143,14 @@ static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec) >> return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc); >> } >> >> +static int cf_check vmx_pi_update_irte(const struct vcpu *v, >> + const struct pirq *pirq, uint8_t gvec) >> +{ >> + const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL; >> + >> + return pi_update_irte(pi_desc, pirq, gvec); >> +} > > This being the only caller of pi_update_irte(), I don't see the point in > having the extra wrapper. Adjust pi_update_irte() such that it can be > used as the intended hook directly. Plus perhaps prefix it with vtd_. Plus move it to vtd/x86/hvm.c (!HVM builds shouldn't need it), albeit I realize this could be done independent of your work. In principle the function shouldn't be VT-d specific (and could hence live in x86/hvm.c), as msi_msg_write_remap_rte() is already available as IOMMU hook anyway, provided struct pi_desc turns out compatible with what's going to be needed for AMD. Jan
On 1/12/23 14:16, Jan Beulich wrote: > On 04.01.2023 09:45, Xenia Ragiadakou wrote: >> --- a/xen/arch/x86/hvm/vmx/vmx.c >> +++ b/xen/arch/x86/hvm/vmx/vmx.c >> @@ -2143,6 +2143,14 @@ static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec) >> return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc); >> } >> >> +static int cf_check vmx_pi_update_irte(const struct vcpu *v, >> + const struct pirq *pirq, uint8_t gvec) >> +{ >> + const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL; >> + >> + return pi_update_irte(pi_desc, pirq, gvec); >> +} > > This being the only caller of pi_update_irte(), I don't see the point in > having the extra wrapper. Adjust pi_update_irte() such that it can be > used as the intended hook directly. Plus perhaps prefix it with vtd_. Ok I will remove the extra wrapper. > >> @@ -2591,6 +2599,8 @@ static struct hvm_function_table __initdata_cf_clobber vmx_function_table = { >> .tsc_scaling = { >> .max_ratio = VMX_TSC_MULTIPLIER_MAX, >> }, >> + >> + .pi_update_irte = vmx_pi_update_irte, > > You want to install this hook only when iommu_intpost (i.e. the only case > when it can actually be called, and only when INTEL_IOMMU=y (avoiding the > need for an inline stub of pi_update_irte() or whatever its final name is > going to be. Ok will do. > >> @@ -250,6 +252,9 @@ struct hvm_function_table { >> /* Architecture function to setup TSC scaling ratio */ >> void (*setup)(struct vcpu *v); >> } tsc_scaling; >> + >> + int (*pi_update_irte)(const struct vcpu *v, >> + const struct pirq *pirq, uint8_t gvec); >> }; > > Please can this be moved higher up, e.g. next to . Right after handle_eoi would be ok? or higher up? > >> @@ -774,6 +779,16 @@ static inline void hvm_set_nonreg_state(struct vcpu *v, >> alternative_vcall(hvm_funcs.set_nonreg_state, v, nrs); >> } >> >> +static inline int hvm_pi_update_irte(const struct vcpu *v, >> + const struct pirq *pirq, uint8_t gvec) >> +{ >> + if ( hvm_funcs.pi_update_irte ) >> + return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec); >> + >> + return -EOPNOTSUPP; > > I don't think the conditional is needed, at least not with the other > suggested adjustments. Plus the way alternative patching works, a NULL > hook will be converted to some equivalent of BUG() anyway, so > ASSERT_UNREACHABLE() should also be unnecessary. Ok will remove it. > >> +} >> + >> + >> #else /* CONFIG_HVM */ > > Please don't add double blank lines. Ok will fix. > >> --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h >> +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h >> @@ -146,6 +146,17 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc) >> clear_bit(POSTED_INTR_SN, &pi_desc->control); >> } >> >> +#ifdef CONFIG_INTEL_IOMMU >> +int pi_update_irte(const struct pi_desc *pi_desc, >> + const struct pirq *pirq, const uint8_t gvec); >> +#else >> +static inline int pi_update_irte(const struct pi_desc *pi_desc, >> + const struct pirq *pirq, const uint8_t gvec) >> +{ >> + return -EOPNOTSUPP; >> +} >> +#endif > > This still is a VT-d function, so I think its declaration would better > remain in asm/iommu.h. > > Jan
On 1/12/23 14:37, Jan Beulich wrote: > On 12.01.2023 13:16, Jan Beulich wrote: >> On 04.01.2023 09:45, Xenia Ragiadakou wrote: >>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>> @@ -2143,6 +2143,14 @@ static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec) >>> return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc); >>> } >>> >>> +static int cf_check vmx_pi_update_irte(const struct vcpu *v, >>> + const struct pirq *pirq, uint8_t gvec) >>> +{ >>> + const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL; >>> + >>> + return pi_update_irte(pi_desc, pirq, gvec); >>> +} >> >> This being the only caller of pi_update_irte(), I don't see the point in >> having the extra wrapper. Adjust pi_update_irte() such that it can be >> used as the intended hook directly. Plus perhaps prefix it with vtd_. > > Plus move it to vtd/x86/hvm.c (!HVM builds shouldn't need it), albeit I > realize this could be done independent of your work. In principle the > function shouldn't be VT-d specific (and could hence live in x86/hvm.c), > as msi_msg_write_remap_rte() is already available as IOMMU hook anyway, > provided struct pi_desc turns out compatible with what's going to be > needed for AMD. Since the posted interrupt descriptor is vmx specific while msi_msg_write_remap_rte is iommu specific, can I propose the following: - Keep the name as is (i.e vmx_pi_update_irte) and keep its definition in xen/arch/x86/hvm/vmx/vmx.c - Open code pi_update_irte() inside the body of vmx_pi_update_irte() but replace intel-specific msi_msg_write_remap_rte() with generic iommu_update_ire_from_msi(). Does this approach make sense?
On 13.01.2023 08:30, Xenia Ragiadakou wrote: > On 1/12/23 14:16, Jan Beulich wrote: >> On 04.01.2023 09:45, Xenia Ragiadakou wrote: >>> @@ -250,6 +252,9 @@ struct hvm_function_table { >>> /* Architecture function to setup TSC scaling ratio */ >>> void (*setup)(struct vcpu *v); >>> } tsc_scaling; >>> + >>> + int (*pi_update_irte)(const struct vcpu *v, >>> + const struct pirq *pirq, uint8_t gvec); >>> }; >> >> Please can this be moved higher up, e.g. next to . > > Right after handle_eoi would be ok? Yes. I'm sorry for sending out the earlier mail without completing the sentence. Jan
On 13.01.2023 08:44, Xenia Ragiadakou wrote: > > On 1/12/23 14:37, Jan Beulich wrote: >> On 12.01.2023 13:16, Jan Beulich wrote: >>> On 04.01.2023 09:45, Xenia Ragiadakou wrote: >>>> --- a/xen/arch/x86/hvm/vmx/vmx.c >>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c >>>> @@ -2143,6 +2143,14 @@ static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec) >>>> return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc); >>>> } >>>> >>>> +static int cf_check vmx_pi_update_irte(const struct vcpu *v, >>>> + const struct pirq *pirq, uint8_t gvec) >>>> +{ >>>> + const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL; >>>> + >>>> + return pi_update_irte(pi_desc, pirq, gvec); >>>> +} >>> >>> This being the only caller of pi_update_irte(), I don't see the point in >>> having the extra wrapper. Adjust pi_update_irte() such that it can be >>> used as the intended hook directly. Plus perhaps prefix it with vtd_. >> >> Plus move it to vtd/x86/hvm.c (!HVM builds shouldn't need it), albeit I >> realize this could be done independent of your work. In principle the >> function shouldn't be VT-d specific (and could hence live in x86/hvm.c), >> as msi_msg_write_remap_rte() is already available as IOMMU hook anyway, >> provided struct pi_desc turns out compatible with what's going to be >> needed for AMD. > > Since the posted interrupt descriptor is vmx specific while > msi_msg_write_remap_rte is iommu specific, can I propose the following: > > - Keep the name as is (i.e vmx_pi_update_irte) and keep its definition > in xen/arch/x86/hvm/vmx/vmx.c > > - Open code pi_update_irte() inside the body of vmx_pi_update_irte() but > replace intel-specific msi_msg_write_remap_rte() with generic > iommu_update_ire_from_msi(). > > Does this approach make sense? Why not - decouples one place of the assumed "CPU vendor" == "IOMMU vendor". Jan
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c index 43a4865d1c..cb6b325e41 100644 --- a/xen/arch/x86/hvm/vmx/vmx.c +++ b/xen/arch/x86/hvm/vmx/vmx.c @@ -2143,6 +2143,14 @@ static bool cf_check vmx_test_pir(const struct vcpu *v, uint8_t vec) return pi_test_pir(vec, &v->arch.hvm.vmx.pi_desc); } +static int cf_check vmx_pi_update_irte(const struct vcpu *v, + const struct pirq *pirq, uint8_t gvec) +{ + const struct pi_desc *pi_desc = v ? &v->arch.hvm.vmx.pi_desc : NULL; + + return pi_update_irte(pi_desc, pirq, gvec); +} + static void cf_check vmx_handle_eoi(uint8_t vector, int isr) { uint8_t old_svi = set_svi(isr); @@ -2591,6 +2599,8 @@ static struct hvm_function_table __initdata_cf_clobber vmx_function_table = { .tsc_scaling = { .max_ratio = VMX_TSC_MULTIPLIER_MAX, }, + + .pi_update_irte = vmx_pi_update_irte, }; /* Handle VT-d posted-interrupt when VCPU is blocked. */ diff --git a/xen/arch/x86/include/asm/hvm/hvm.h b/xen/arch/x86/include/asm/hvm/hvm.h index 93254651f2..b3fe0663f9 100644 --- a/xen/arch/x86/include/asm/hvm/hvm.h +++ b/xen/arch/x86/include/asm/hvm/hvm.h @@ -28,6 +28,8 @@ #include <asm/x86_emulate.h> #include <asm/hvm/asid.h> +struct pirq; /* needed by pi_update_irte */ + #ifdef CONFIG_HVM_FEP /* Permit use of the Forced Emulation Prefix in HVM guests */ extern bool_t opt_hvm_fep; @@ -250,6 +252,9 @@ struct hvm_function_table { /* Architecture function to setup TSC scaling ratio */ void (*setup)(struct vcpu *v); } tsc_scaling; + + int (*pi_update_irte)(const struct vcpu *v, + const struct pirq *pirq, uint8_t gvec); }; extern struct hvm_function_table hvm_funcs; @@ -774,6 +779,16 @@ static inline void hvm_set_nonreg_state(struct vcpu *v, alternative_vcall(hvm_funcs.set_nonreg_state, v, nrs); } +static inline int hvm_pi_update_irte(const struct vcpu *v, + const struct pirq *pirq, uint8_t gvec) +{ + if ( hvm_funcs.pi_update_irte ) + return alternative_call(hvm_funcs.pi_update_irte, v, pirq, gvec); + + return -EOPNOTSUPP; +} + + #else /* CONFIG_HVM */ #define hvm_enabled false diff --git a/xen/arch/x86/include/asm/hvm/vmx/vmx.h b/xen/arch/x86/include/asm/hvm/vmx/vmx.h index 96a9f07ca5..e827fece07 100644 --- a/xen/arch/x86/include/asm/hvm/vmx/vmx.h +++ b/xen/arch/x86/include/asm/hvm/vmx/vmx.h @@ -146,6 +146,17 @@ static inline void pi_clear_sn(struct pi_desc *pi_desc) clear_bit(POSTED_INTR_SN, &pi_desc->control); } +#ifdef CONFIG_INTEL_IOMMU +int pi_update_irte(const struct pi_desc *pi_desc, + const struct pirq *pirq, const uint8_t gvec); +#else +static inline int pi_update_irte(const struct pi_desc *pi_desc, + const struct pirq *pirq, const uint8_t gvec) +{ + return -EOPNOTSUPP; +} +#endif + /* * Exit Reasons */ diff --git a/xen/arch/x86/include/asm/iommu.h b/xen/arch/x86/include/asm/iommu.h index fb5fe4e1bf..b432790d24 100644 --- a/xen/arch/x86/include/asm/iommu.h +++ b/xen/arch/x86/include/asm/iommu.h @@ -131,9 +131,6 @@ void iommu_identity_map_teardown(struct domain *d); extern bool untrusted_msi; #endif -int pi_update_irte(const struct pi_desc *pi_desc, const struct pirq *pirq, - const uint8_t gvec); - extern bool iommu_non_coherent, iommu_superpages; static inline void iommu_sync_cache(const void *addr, unsigned int size) diff --git a/xen/drivers/passthrough/x86/hvm.c b/xen/drivers/passthrough/x86/hvm.c index a16e0e5344..e720461a14 100644 --- a/xen/drivers/passthrough/x86/hvm.c +++ b/xen/drivers/passthrough/x86/hvm.c @@ -381,8 +381,7 @@ int pt_irq_create_bind( /* Use interrupt posting if it is supported. */ if ( iommu_intpost ) - pi_update_irte(vcpu ? &vcpu->arch.hvm.vmx.pi_desc : NULL, - info, pirq_dpci->gmsi.gvec); + hvm_pi_update_irte(vcpu, info, pirq_dpci->gmsi.gvec); if ( pt_irq_bind->u.msi.gflags & XEN_DOMCTL_VMSI_X86_UNMASKED ) { @@ -672,7 +671,7 @@ int pt_irq_destroy_bind( what = "bogus"; } else if ( pirq_dpci && pirq_dpci->gmsi.posted ) - pi_update_irte(NULL, pirq, 0); + hvm_pi_update_irte(NULL, pirq, 0); if ( pirq_dpci && (pirq_dpci->flags & HVM_IRQ_DPCI_MAPPED) && list_empty(&pirq_dpci->digl_list) )
Posted interrupt support in Xen is currently implemented only for the Intel platforms. Instead of calling directly pi_update_irte() from the common hvm code, add a pi_update_irte callback to the hvm_function_table. Then, create a wrapper function hvm_pi_update_irte() to be used by the common hvm code. In the pi_update_irte callback prototype, pass the vcpu as first parameter instead of the posted-interrupt descriptor that is platform specific, and remove the const qualifier from the parameter gvec since it is not needed and because it does not compile with the alternative code patching in use. Move the declaration of pi_update_irte() from asm/iommu.h to asm/hvm/vmx/vmx.h since it is hvm and Intel specific. No functional change intended. Signed-off-by: Xenia Ragiadakou <burzalodowa@gmail.com> --- Changes in v2: - remove the definition of hvm_pi_update_irte() for !CONFIG_HVM - replace CONFIG_INTEL_VTD with CONFIG_INTEL_IOMMU xen/arch/x86/hvm/vmx/vmx.c | 10 ++++++++++ xen/arch/x86/include/asm/hvm/hvm.h | 15 +++++++++++++++ xen/arch/x86/include/asm/hvm/vmx/vmx.h | 11 +++++++++++ xen/arch/x86/include/asm/iommu.h | 3 --- xen/drivers/passthrough/x86/hvm.c | 5 ++--- 5 files changed, 38 insertions(+), 6 deletions(-)