Re: [PATCH v2 6/8] x86/iommu: call pi_update_irte through an hvm_function callback

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

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
--
Xenia