Re: [Qemu-devel] [RFC v1 11/18] intel_iommu: create VTDAddressSpace per BDF+PASID

[Peter Xu]

On Fri, Jul 05, 2019 at 07:01:44PM +0800, Liu Yi L wrote:

[...]

> +/**
> + * This function finds or adds a VTDAddressSpace for a device when
> + * it is bound to a pasid
> + */
> +static VTDAddressSpace *vtd_add_find_pasid_as(IntelIOMMUState *s,
> +                                              PCIBus *bus,
> +                                              int devfn,
> +                                              uint32_t pasid,
> +                                              bool allocate)
> +{
> +    char key[32];
> +    char *new_key;
> +    VTDAddressSpace *vtd_pasid_as;
> +    uint16_t sid;
> +
> +    sid = vtd_make_source_id(pci_bus_num(bus), devfn);
> +    vtd_get_pasid_key(&key[0], 32, pasid, sid);
> +    vtd_pasid_as = g_hash_table_lookup(s->vtd_pasid_as, &key[0]);
> +
> +    if (!vtd_pasid_as && allocate) {
> +        new_key = g_malloc(32);
> +        vtd_get_pasid_key(&new_key[0], 32, pasid, sid);
> +        /*
> +         * Initiate the vtd_pasid_as structure.
> +         *
> +         * This structure here is used to track the guest pasid
> +         * binding and also serves as pasid-cache mangement entry.
> +         *
> +         * TODO: in future, if wants to support the SVA-aware DMA
> +         *       emulation, the vtd_pasid_as should be fully initialized.
> +         *       e.g. the address_space and memory region fields.
> +         */

I'm not very sure about this part.  IMHO all those memory regions are
used to inlay the whole IOMMU idea into QEMU's memory API framework.
Now even without the whole PASID support we've already have a workable
vtd_iommu_translate() that will intercept device DMA operations and we
can try to translate the IOVA to anything we want.  Now the iommu_idx
parameter of vtd_iommu_translate() is never used (I'd say until now I
still don't sure on whether the "iommu_idx" idea is the best we can
have... I've tried to debate on that but... anyway I assume for Intel
we can think it as the "pasid" information or at least contains it),
however in the further we can have that PASID/iommu_idx/whatever
passed into this translate() function too, then we can walk the 1st
level page table there if we found that this device had enabled the
1st level mapping (or even nested).  I don't see what else we need to
do to play with extra memory regions.

Conclusion: I feel like SVA can use its own structure here instead of
reusing VTDAddressSpace, because I think those memory regions can
probably be useless.  Even it will, we can refactor the code later,
but I really doubt it...

> +        vtd_pasid_as = g_malloc0(sizeof(VTDAddressSpace));
> +        vtd_pasid_as->iommu_state = s;
> +        vtd_pasid_as->bus = bus;
> +        vtd_pasid_as->devfn = devfn;
> +        vtd_pasid_as->context_cache_entry.context_cache_gen = 0;
> +        vtd_pasid_as->pasid = pasid;
> +        vtd_pasid_as->pasid_allocated = true;
> +        vtd_pasid_as->pasid_cache_entry.pasid_cache_gen = 0;
> +        g_hash_table_insert(s->vtd_pasid_as, new_key, vtd_pasid_as);
> +    }
> +    return vtd_pasid_as;
> +}

Regards,

-- 
Peter Xu