Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and sync host IOMMU cap/ecap

[Eric Auger]

On 1/19/24 12:55, Duan, Zhenzhong wrote:
>
>> -----Original Message-----
>> From: Eric Auger <eric.auger@redhat.com>
>> Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check and
>> sync host IOMMU cap/ecap
>>
>>
>>
>> On 1/18/24 10:30, Duan, Zhenzhong wrote:
>>> Hi Eric,
>>>
>>>> -----Original Message-----
>>>> From: Eric Auger <eric.auger@redhat.com>
>>>> Subject: Re: [PATCH rfcv1 6/6] intel_iommu: add a framework to check
>> and
>>>> sync host IOMMU cap/ecap
>>>>
>>>> Hi Zhenzhong,
>>>>
>>>> On 1/15/24 11:13, Zhenzhong Duan wrote:
>>>>> From: Yi Liu <yi.l.liu@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.liu@intel.com>
>>>>> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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.auger@redhat.com/
>>>>
>>>> I also attempt to pass host iommu info to the virtio-iommu but with
>>>> legacy BE.
>>> I think your patch works with iommufd BE too😊 Because iommufd BE
>>> also fills bcontainer->iova_ranges in iommufd_cdev_get_info_iova_range().
>> correct. I wanted to emphasize that we also have the need to pass host
>> iommu info in legacy mode for instance. In this series you introduce an
>> object that works with the iommufd backed but I think if we go this way
>> we would need another one for the legacy device. So maybe introducing a
>> base object derived into 2 ones may be the most appropriate? Maybe,
>> given the assumption that we will use iommufd for new use cases this
>> legacy object will implement much fewer interfaces but still.
> How about this:
>
> enum IOMMU_LEGACY_DEVICE_TYPE {
>     IOMMU_LEGACY_VFIO_DEVICE,
>     IOMMU_LEGACY_VDPA_DEVICE,
> }
>
> typedef struct IOMMULegacyDevice {
>     enum IOMMU_LEGACY_DEVICE_TYPE type;
>
>     /* common field */
>
>     union {
>         ....
>     }
>
> } IOMMULegacyDevice;
>
> typedef struct IOMMUFDDevice {
>     IOMMUFDBackend *iommufd;
>     uint32_t dev_id;
>     uint32_t ioas_id;
> } IOMMUFDDevice;
>
> enum IOMMUDEVICE_TYPE {
>     IOMMUFD_DEVICE,
>     IOMMU_LEGACY_DEVICE,
> }
>
> struct IOMMUDevice {
>     enum IOMMU_DEVICE_TYPE type;
>
>     /* common field */
>     GList *iova_ranges;
>
>     union {
>         IOMMULegacyDevice legacy_dev;
yeah but that's not very nice to have this LegacyDevice def in an
iommufd.c file

Either we define an abstract HostAssignedDevice and derived objects for
both legacy and IOMMUFD or we consider using a different API for legacy
use cases (retrieving resv regions, page size mask, ...).

Eric



+int iommufd_device_get_info(IOMMUFDDevice *idev,
+                            enum iommu_hw_info_type *type,
+                            uint32_t len, void *data);
+void iommufd_device_init(void *_idev, size_t instance_size,
+                         IOMMUFDBackend *iommufd, uint32_t dev_id);

>         IOMMUFDDevice idev;
>     }
> }
>
>>>> In my case I want to pass the reserved memory regions which
>>>> also model the aw.
>>>> So this is a pretty similar use case.
>>> Yes.
>>>
>>>> Why don't we pass the pointer to an opaque iommu_hw_info instead,
>>>> through the PCIIOMMUOps?
>>> Passing iommu_hw_info is ok for this series, but we want more from
>>> IOMMUFDDevice in nesting series. I.e., allocate/free ioas/hwpt,
>>> attach/detach from hwpt, get dirty bitmap, etc. It's more flexible to
>>> let vIOMMU get what it want itself.
>> OK, would be interesting to define the class for this object. Worth to
>> be introduced either in the cover letter or in the 1st patch
> Not a QOM class because we don't want it showed out through
> query-qmp-schema.
>
> Thanks
> Zhenzhong
>
>> Eric
>>>>
>>>>> +        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?
>>> I'll doc it. Above we have one line to explain when cap/ecap are 
>>> initialized.
>>> vtd_init() is called in qemu init and guest reset. In qemu init,
>>> Cap/ecap is finalized, after that we don't want cap/ecap to be changed.
>>> So we bypass change to cap/ecap here.
>>>
>>>>>      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).
>>> Will bump vtd_vmstate .version_id works?
>>>
>>> Thanks
>>> Zhenzhong
>>>
>>>> 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(),