On Thu, Oct 24, 2019 at 08:34:29AM -0400, Liu Yi L wrote:
> This patch adds get_iommu_context() callback to return an iommu_context
> Intel VT-d platform.
>
> Cc: Kevin Tian <kevin.tian@xxxxxxxxx>
> Cc: Jacob Pan <jacob.jun.pan@xxxxxxxxxxxxxxx>
> Cc: Peter Xu <peterx@xxxxxxxxxx>
> Cc: Yi Sun <yi.y.sun@xxxxxxxxxxxxxxx>
> Signed-off-by: Liu Yi L <yi.l.liu@xxxxxxxxx>
> ---
> hw/i386/intel_iommu.c | 57 ++++++++++++++++++++++++++++++++++++++-----
> include/hw/i386/intel_iommu.h | 14 ++++++++++-
> 2 files changed, 64 insertions(+), 7 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 67a7836..e9f8692 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3288,22 +3288,33 @@ static const MemoryRegionOps vtd_mem_ir_ops = {
> },
> };
>
> -VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
> +static VTDBus *vtd_find_add_bus(IntelIOMMUState *s, PCIBus *bus)
> {
> uintptr_t key = (uintptr_t)bus;
> - VTDBus *vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key);
> - VTDAddressSpace *vtd_dev_as;
> - char name[128];
> + VTDBus *vtd_bus;
>
> + vtd_iommu_lock(s);
Why explicitly take the IOMMU lock here? I mean, it's fine to take
it, but if so why not take it to cover the whole vtd_find_add_as()?
For now it'll be fine in either way because I believe iommu_lock is
not really functioning when we're still with BQL here, however if you
add that explicitly then I don't see why it's not covering that.
> + vtd_bus = g_hash_table_lookup(s->vtd_as_by_busptr, &key);
> if (!vtd_bus) {
> uintptr_t *new_key = g_malloc(sizeof(*new_key));
> *new_key = (uintptr_t)bus;
> /* No corresponding free() */
> - vtd_bus = g_malloc0(sizeof(VTDBus) + sizeof(VTDAddressSpace *) * \
> - PCI_DEVFN_MAX);
> + vtd_bus = g_malloc0(sizeof(VTDBus) + PCI_DEVFN_MAX * \
> + (sizeof(VTDAddressSpace *) + sizeof(VTDIOMMUContext *)));
Should this be as simple as g_malloc0(sizeof(VTDBus) since [1]?
Otherwise the patch looks sane to me.
> vtd_bus->bus = bus;
> g_hash_table_insert(s->vtd_as_by_busptr, new_key, vtd_bus);
> }
> + vtd_iommu_unlock(s);
> + return vtd_bus;
> +}
[...]
> struct VTDBus {
> PCIBus* bus; /* A reference to the bus to provide translation for */
> - VTDAddressSpace *dev_as[0]; /* A table of VTDAddressSpace objects indexed by devfn */
> + /* A table of VTDAddressSpace objects indexed by devfn */
> + VTDAddressSpace *dev_as[PCI_DEVFN_MAX];
> + /* A table of VTDIOMMUContext objects indexed by devfn */
> + VTDIOMMUContext *dev_ic[PCI_DEVFN_MAX];
[1]
> };
>
> struct VTDIOTLBEntry {
> @@ -282,5 +293,6 @@ struct IntelIOMMUState {
> * create a new one if none exists
> */
> VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn);
> +VTDIOMMUContext *vtd_find_add_ic(IntelIOMMUState *s, PCIBus *bus, int devfn);
>
> #endif
> --
> 2.7.4
>
Regards,
--
Peter Xu
