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

[Jason Wang <jasowang@redhat.com>]

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