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

[Jason Wang]

On 2017年04月18日 12:21, Peter Xu wrote:
> On Tue, Apr 18, 2017 at 12:00:13PM +0800, Jason Wang wrote:
> > On 2017年04月18日 11:50, Peter Xu wrote:
> > > On Tue, Apr 18, 2017 at 11:23:35AM +0800, Jason Wang wrote:
> > > > On 2017年04月17日 18:58, Peter Xu wrote:
> > > [...]
> > >
> > > > > +static void vtd_switch_address_space(VTDAddressSpace *as)
> > > > > +{
> > > > > +    bool use_iommu;
> > > > > +
> > > > > +    assert(as);
> > > > > +
> > > > > +    use_iommu = as->iommu_state->dmar_enabled;
> > > > > +    if (use_iommu) {
> > > > > +        /* Further checks per-device configuration */
> > > > > +        use_iommu &= !vtd_dev_pt_enabled(as);
> > > > > +    }
> > > > Looks like you can use as->iommu_state->dmar_enabled &&
> > > > !vtd_dev_pt_enabled(as)
> > > vtd_dev_pt_enalbed() needs to read the guest memory (starting from
> > > reading root entry), which is slightly slow. I was trying to avoid
> > > unecessary reads.
> > >
> > > [...]
> > I think compiler won't go to vtd_dev_pt_enabled() if dmar_enabled is false.
> You are right. I'll switch.
>
> > > > > @@ -991,6 +1058,18 @@ static void vtd_do_iommu_translate(VTDAddressSpace 
> > > > > *vtd_as, PCIBus *bus,
> > > > >           cc_entry->context_cache_gen = s->context_cache_gen;
> > > > >       }
> > > > > +    /*
> > > > > +     * We don't need to translate for pass-through context entries.
> > > > > +     * Also, let's ignore IOTLB caching as well for PT devices.
> > > > > +     */
> > > > > +    if (vtd_ce_get_type(&ce) == VTD_CONTEXT_TT_PASS_THROUGH) {
> > > > > +        entry->translated_addr = entry->iova;
> > > > > +        entry->addr_mask = VTD_PAGE_SIZE - 1;
> > > > > +        entry->perm = IOMMU_RW;
> > > > > +        trace_vtd_translate_pt(source_id, entry->iova);
> > > > > +        return;
> > > > > +    }
> > > > Several questions here:
> > > >
> > > > 1) Is this just for vhost?
> > > No. When caching mode is not enabled, all passthroughed devices should
> > > be using this path.
> > Ok, then it looks better to switch the address space if we've found it was
> > PT?
> Do you mean to switch in that if() above? Then when invalidate context
> entry, we switch back if needed?

Yes.


> > > > 2) Since this is done after IOTLB querying, do we need flush IOTLB during
> > > > address switching?
> > > IMHO if guest switches address space for a device, it is required to
> > > send IOTLB flush as well for that device/domain.
> > >
> > > [...]
> > Ok.
> >
> > > > >   static void vtd_switch_address_space_all(IntelIOMMUState *s)
> > > > >   {
> > > > >       GHashTableIter iter;
> > > > > @@ -2849,6 +2914,10 @@ static void vtd_init(IntelIOMMUState *s)
> > > > >           s->ecap |= VTD_ECAP_DT;
> > > > >       }
> > > > > +    if (x86_iommu->pt_supported) {
> > > > > +        s->ecap |= VTD_ECAP_PT;
> > > > > +    }
> > > > Since we support migration now, need compat this for pre 2.10.
> > > Oh yes. If I set pt=off by default, it should be okay then, right?
> > Right, but I think it's better to keep this on by default for performance
> > reason.
> Okay. Just to confirm, that'll need one entry for HW_COMPAT_2_9,
> right? (though it is still not there)

Right.

Thanks