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

[Peter Xu <peterx@redhat.com>]

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?

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

-- 
Peter Xu