Hi Zhenzhong,
On 1/15/24 11:13, Zhenzhong Duan wrote:
> From: Yi Liu <yi.l....@intel.com>
>
> Add a framework to check and synchronize host IOMMU cap/ecap with
> vIOMMU cap/ecap.
>
> Currently only stage-2 translation is supported which is backed by
> shadow page table on host side. So we don't need exact matching of
> each bit of cap/ecap between vIOMMU and host. However, we can still
> utilize this framework to ensure compatibility of host and vIOMMU's
> address width at least, i.e., vIOMMU's aw_bits <= host aw_bits,
> which is missed before.
>
> When stage-1 translation is supported in future, a.k.a. scalable
> modern mode, we need to ensure compatibility of each bits. Some
> bits are user controllable, they should be checked with host side
> to ensure compatibility. Other bits are not, they should be synced
> into vIOMMU cap/ecap for compatibility.
>
> The sequence will be:
>
> vtd_cap_init() initializes iommu->cap/ecap. ---- vtd_cap_init()
> iommu->host_cap/ecap is initialized as iommu->cap/ecap. ---- vtd_init()
> iommu->host_cap/ecap is checked and updated some bits with host cap/ecap.
> ---- vtd_sync_hw_info()
> iommu->cap/ecap is finalized as iommu->host_cap/ecap. ----
> vtd_machine_done_hook()
>
> iommu->host_cap/ecap is a temporary storage to hold intermediate value
> when synthesize host cap/ecap and vIOMMU's initial configured cap/ecap.
>
> Signed-off-by: Yi Liu <yi.l....@intel.com>
> Signed-off-by: Yi Sun <yi.y....@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
> ---
> include/hw/i386/intel_iommu.h | 4 ++
> hw/i386/intel_iommu.c | 78 +++++++++++++++++++++++++++++++----
> 2 files changed, 75 insertions(+), 7 deletions(-)
>
> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h
> index c65fdde56f..b8abbcce12 100644
> --- a/include/hw/i386/intel_iommu.h
> +++ b/include/hw/i386/intel_iommu.h
> @@ -292,6 +292,9 @@ struct IntelIOMMUState {
> uint64_t cap; /* The value of capability reg */
> uint64_t ecap; /* The value of extended capability reg
> */
>
> + uint64_t host_cap; /* The value of host capability reg */
> + uint64_t host_ecap; /* The value of host ext-capability reg
> */
> +
> uint32_t context_cache_gen; /* Should be in [1,MAX] */
> GHashTable *iotlb; /* IOTLB */
>
> @@ -314,6 +317,7 @@ struct IntelIOMMUState {
> bool dma_translation; /* Whether DMA translation supported */
> bool pasid; /* Whether to support PASID */
>
> + bool cap_finalized; /* Whether VTD capability finalized */
> /*
> * Protects IOMMU states in general. Currently it protects the
> * per-IOMMU IOTLB cache, and context entry cache in VTDAddressSpace.
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 4c1d058ebd..be03fcbf52 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3819,6 +3819,47 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s,
> PCIBus *bus,
> return vtd_dev_as;
> }
>
> +static bool vtd_sync_hw_info(IntelIOMMUState *s, struct iommu_hw_info_vtd
> *vtd,
> + Error **errp)
> +{
> + uint64_t addr_width;
> +
> + addr_width = (vtd->cap_reg >> 16) & 0x3fULL;
> + if (s->aw_bits > addr_width) {
> + error_setg(errp, "User aw-bits: %u > host address width: %lu",
> + s->aw_bits, addr_width);
> + return false;
> + }
> +
> + /* TODO: check and sync host cap/ecap into vIOMMU cap/ecap */
> +
> + return true;
> +}
> +
> +/*
> + * virtual VT-d which wants nested needs to check the host IOMMU
> + * nesting cap info behind the assigned devices. Thus that vIOMMU
> + * could bind guest page table to host.
> + */
> +static bool vtd_check_idev(IntelIOMMUState *s, IOMMUFDDevice *idev,
> + Error **errp)
> +{
> + struct iommu_hw_info_vtd vtd;
> + enum iommu_hw_info_type type = IOMMU_HW_INFO_TYPE_INTEL_VTD;
> +
> + if (iommufd_device_get_info(idev, &type, sizeof(vtd), &vtd)) {
> + error_setg(errp, "Failed to get IOMMU capability!!!");
> + return false;
> + }
> +
> + if (type != IOMMU_HW_INFO_TYPE_INTEL_VTD) {
> + error_setg(errp, "IOMMU hardware is not compatible!!!");
> + return false;
> + }
> +
> + return vtd_sync_hw_info(s, &vtd, errp);
> +}
> +
> static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int32_t devfn,
> IOMMUFDDevice *idev, Error **errp)
> {
> @@ -3837,6 +3878,10 @@ static int vtd_dev_set_iommu_device(PCIBus *bus, void
> *opaque, int32_t devfn,
> return 0;
> }
>
> + if (!vtd_check_idev(s, idev, errp)) {In
In
[RFC 0/7] VIRTIO-IOMMU/VFIO: Fix host iommu geometry handling for
hotplugged devices
https://lore.kernel.org/all/20240117080414.316890-1-eric.au...@redhat.com/
I also attempt to pass host iommu info to the virtio-iommu but with
legacy BE. In my case I want to pass the reserved memory regions which
also model the aw.
So this is a pretty similar use case.
Why don't we pass the pointer to an opaque iommu_hw_info instead,
through the PCIIOMMUOps?
> + return -1;
> + }
> +
> vtd_iommu_lock(s);
>
> vtd_idev = g_hash_table_lookup(s->vtd_iommufd_dev, &key);
> @@ -4071,10 +4116,11 @@ static void vtd_init(IntelIOMMUState *s)
> {
> X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(s);
>
> - memset(s->csr, 0, DMAR_REG_SIZE);
> - memset(s->wmask, 0, DMAR_REG_SIZE);
> - memset(s->w1cmask, 0, DMAR_REG_SIZE);
> - memset(s->womask, 0, DMAR_REG_SIZE);
> + /* CAP/ECAP are initialized in machine create done stage */
> + memset(s->csr + DMAR_GCMD_REG, 0, DMAR_REG_SIZE - DMAR_GCMD_REG);
> + memset(s->wmask + DMAR_GCMD_REG, 0, DMAR_REG_SIZE - DMAR_GCMD_REG);
> + memset(s->w1cmask + DMAR_GCMD_REG, 0, DMAR_REG_SIZE - DMAR_GCMD_REG);
> + memset(s->womask + DMAR_GCMD_REG, 0, DMAR_REG_SIZE - DMAR_GCMD_REG);
This change is not documented in the commit msg.
Sorry I don't get why this is needed?
>
> s->root = 0;
> s->root_scalable = false;
> @@ -4110,13 +4156,16 @@ static void vtd_init(IntelIOMMUState *s)
> vtd_spte_rsvd_large[3] &= ~VTD_SPTE_SNP;
> }
>
> - vtd_cap_init(s);
> + if (!s->cap_finalized) {
> + vtd_cap_init(s);
> + s->host_cap = s->cap;
> + s->host_ecap = s->ecap;
> + }
> +
> vtd_reset_caches(s);
>
> /* Define registers with default values and bit semantics */
> vtd_define_long(s, DMAR_VER_REG, 0x10UL, 0, 0);
> - vtd_define_quad(s, DMAR_CAP_REG, s->cap, 0, 0);
> - vtd_define_quad(s, DMAR_ECAP_REG, s->ecap, 0, 0);
> vtd_define_long(s, DMAR_GCMD_REG, 0, 0xff800000UL, 0);
> vtd_define_long_wo(s, DMAR_GCMD_REG, 0xff800000UL);
> vtd_define_long(s, DMAR_GSTS_REG, 0, 0, 0);
> @@ -4241,6 +4290,12 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> Error **errp)
> return true;
> }
>
> +static void vtd_setup_capability_reg(IntelIOMMUState *s)
> +{
> + vtd_define_quad(s, DMAR_CAP_REG, s->cap, 0, 0);
> + vtd_define_quad(s, DMAR_ECAP_REG, s->ecap, 0, 0);
> +}
> +
> static int vtd_machine_done_notify_one(Object *child, void *unused)
> {
> IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
> @@ -4259,6 +4314,14 @@ static int vtd_machine_done_notify_one(Object *child,
> void *unused)
>
> static void vtd_machine_done_hook(Notifier *notifier, void *unused)
> {
> + IntelIOMMUState *iommu = INTEL_IOMMU_DEVICE(x86_iommu_get_default());
> +
> + iommu->cap = iommu->host_cap;
> + iommu->ecap = iommu->host_ecap;
> + iommu->cap_finalized = true;
I don't think you can change the defaults like this without taking care
of compats (migration).
Thanks
Eric
> +
> + vtd_setup_capability_reg(iommu);
> +
> object_child_foreach_recursive(object_get_root(),
> vtd_machine_done_notify_one, NULL);
> }
> @@ -4292,6 +4355,7 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>
> QLIST_INIT(&s->vtd_as_with_notifiers);
> qemu_mutex_init(&s->iommu_lock);
> + s->cap_finalized = false;
> memory_region_init_io(&s->csrmem, OBJECT(s), &vtd_mem_ops, s,
> "intel_iommu", DMAR_REG_SIZE);
> memory_region_add_subregion(get_system_memory(),
