Re: [Qemu-devel] [PATCH v2 7/7] intel_iommu: support passthrough (PT)

[Peter Xu <peterx@redhat.com>]

On Wed, Apr 19, 2017 at 07:13:47AM +0000, Liu, Yi L wrote:
> Peter,
> 
> Besides the gPA->hPA mapping, pt mode enabling requires some sanity check on
> s->pt_supported. See comments inline.
> 
> > -----Original Message-----
> > From: Qemu-devel [mailto:qemu-devel-bounces+yi.l.liu=intel.com@nongnu.org] On
> > Behalf Of Peter Xu
> > Sent: Monday, April 17, 2017 7:32 PM
> > To: qemu-devel@nongnu.org
> > Cc: Lan, Tianyu <tianyu.lan@intel.com>; Michael S . Tsirkin <mst@redhat.com>;
> > Jason Wang <jasowang@redhat.com>; peterx@redhat.com; Marcel Apfelbaum
> > <marcel@redhat.com>; David Gibson <david@gibson.dropbear.id.au>
> > Subject: [Qemu-devel] [PATCH v2 7/7] intel_iommu: support passthrough (PT)
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/i386/intel_iommu.c          | 109 +++++++++++++++++++++++++++++++++--------
> >  hw/i386/intel_iommu_internal.h |   1 +
> >  hw/i386/trace-events           |   1 +
> >  hw/i386/x86-iommu.c            |   1 +
> >  include/hw/i386/x86-iommu.h    |   1 +
> >  5 files changed, 93 insertions(+), 20 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index
> > 05ae631..deb2007 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -872,7 +872,7 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s,
> > uint8_t bus_num,
> >      } else {
> >          switch (vtd_ce_get_type(ce)) {
> >          case VTD_CONTEXT_TT_MULTI_LEVEL:
> > -            /* fall through */
> > +        case VTD_CONTEXT_TT_PASS_THROUGH:
> 
> Passthru type is a reserved type if ecap.PT=0, so add a check here. If s->pt_supported
> is false, report this translation type as unsupported and report an invalid context 
> programming to guest.
> 
> >          case VTD_CONTEXT_TT_DEV_IOTLB:
> >              break;
> >          default:
> > @@ -883,6 +883,73 @@ static int vtd_dev_to_context_entry(IntelIOMMUState *s,
> > uint8_t bus_num,
> >      return 0;
> >  }
> > 
> > +/* Fetch translation type for specific device. Returns <0 if error
> > + * happens, otherwise return the shifted type to check against
> > + * VTD_CONTEXT_TT_*. */
> > +static int vtd_dev_get_trans_type(VTDAddressSpace *as) {
> > +    IntelIOMMUState *s;
> > +    VTDContextEntry ce;
> > +    int ret;
> > +
> > +    s = as->iommu_state;
> > +
> > +    ret = vtd_dev_to_context_entry(s, pci_bus_num(as->bus),
> > +                                   as->devfn, &ce);
> > +    if (ret) {
> > +        return ret;
> > +    }
> > +
> > +    return vtd_ce_get_type(&ce);
> > +}
> > +
> > +static bool vtd_dev_pt_enabled(VTDAddressSpace *as) {
> > +    int ret;
> > +
> > +    assert(as);
> > +
> > +    ret = vtd_dev_get_trans_type(as);
> > +    if (ret < 0) {
> > +        /*
> > +         * Possibly failed to parse the context entry for some reason
> > +         * (e.g., during init, or any guest configuration errors on
> > +         * context entries). We should assume PT not enabled for
> > +         * safety.
> > +         */
> > +        return false;
> > +    }
> > +
> > +    return ret == VTD_CONTEXT_TT_PASS_THROUGH; }
> > +
> > +static void vtd_switch_address_space(VTDAddressSpace *as) {
> > +    bool use_iommu;
> > +
> > +    assert(as);
> > +
> > +    use_iommu = as->iommu_state->dmar_enabled;
> > +    if (use_iommu) {
> 
> if s->pt_supported is false, no need to check the context.TT, even
> context.TT=pt, then it should be an invalid context programming.

Agree with you on both comments. Thanks. :-)

-- 
Peter Xu