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

[Jason Wang]

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.

> > > @@ -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?

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

> Then, at some point, we can switch to on by default, with a touch-up
> in include/hw/compat.h I guess?

Yes.

Thanks
> > > +
> > >       if (s->caching_mode) {
> > >           s->cap |= VTD_CAP_CM;
> > >       }
> > > diff --git a/hw/i386/intel_iommu_internal.h b/hw/i386/intel_iommu_internal.h
> > > index 29d6707..0e73a65 100644
> > > --- a/hw/i386/intel_iommu_internal.h
> > > +++ b/hw/i386/intel_iommu_internal.h
> > > @@ -187,6 +187,7 @@
> > >   /* Interrupt Remapping support */
> > >   #define VTD_ECAP_IR                 (1ULL << 3)
> > >   #define VTD_ECAP_EIM                (1ULL << 4)
> > > +#define VTD_ECAP_PT                 (1ULL << 6)
> > >   #define VTD_ECAP_MHMV               (15ULL << 20)
> > >   /* CAP_REG */
> > > diff --git a/hw/i386/trace-events b/hw/i386/trace-events
> > > index 04a6980..867ad0b 100644
> > > --- a/hw/i386/trace-events
> > > +++ b/hw/i386/trace-events
> > > @@ -38,6 +38,7 @@ vtd_page_walk_skip_perm(uint64_t iova, uint64_t next) "Page walk 
> > > skip iova 0x%"P
> > >   vtd_page_walk_skip_reserve(uint64_t iova, uint64_t next) "Page walk skip iova 0x%"PRIx64" - 
> > > 0x%"PRIx64" due to rsrv set"
> > >   vtd_switch_address_space(uint8_t bus, uint8_t slot, uint8_t fn, bool on) "Device 
> > > %02x:%02x.%x switching address space (iommu enabled=%d)"
> > >   vtd_as_unmap_whole(uint8_t bus, uint8_t slot, uint8_t fn, uint64_t iova, uint64_t size) 
> > > "Device %02x:%02x.%x start 0x%"PRIx64" size 0x%"PRIx64
> > > +vtd_translate_pt(uint16_t sid, uint64_t addr) "source id 0x%"PRIu16", iova 
> > > 0x%"PRIx64
> > >   # hw/i386/amd_iommu.c
> > >   amdvi_evntlog_fail(uint64_t addr, uint32_t head) "error: fail to write at addr 
> > > 0x%"PRIx64" +  offset 0x%"PRIx32
> > > diff --git a/hw/i386/x86-iommu.c b/hw/i386/x86-iommu.c
> > > index 02b8825..293caf8 100644
> > > --- a/hw/i386/x86-iommu.c
> > > +++ b/hw/i386/x86-iommu.c
> > > @@ -91,6 +91,7 @@ static void x86_iommu_realize(DeviceState *dev, Error **errp)
> > >   static Property x86_iommu_properties[] = {
> > >       DEFINE_PROP_BOOL("intremap", X86IOMMUState, intr_supported, false),
> > >       DEFINE_PROP_BOOL("device-iotlb", X86IOMMUState, dt_supported, false),
> > > +    DEFINE_PROP_BOOL("pt", X86IOMMUState, pt_supported, true),
> > Do you know if AMD IOMMU support this?
> AMD IOMMU should support this. IIUC it's the first bit of Device Table Entry.
>
> Thanks,