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
