Re: [PATCH v1 3/6] intel_iommu: Add a framework to check and sync host IOMMU cap/ecap

[Michael S. Tsirkin]

On Wed, Mar 13, 2024 at 02:52:39AM +0000, Duan, Zhenzhong wrote:
> Hi Michael,
> 
> >-----Original Message-----
> >From: Michael S. Tsirkin <mst@redhat.com>
> >Subject: Re: [PATCH v1 3/6] intel_iommu: Add a framework to check and
> >sync host IOMMU cap/ecap
> >
> >On Wed, Feb 28, 2024 at 05:44:29PM +0800, 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.
> >>
> >> The sequence will be:
> >>
> >> vtd_cap_init() initializes iommu->cap/ecap.
> >> vtd_check_hdev() update iommu->cap/ecap based on host cap/ecap.
> >> iommu->cap_frozen set when machine create done, iommu->cap/ecap
> >become readonly.
> >>
> >> Implementation details for different backends will be in following patches.
> >>
> >> 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 |  1 +
> >>  hw/i386/intel_iommu.c         | 50
> >++++++++++++++++++++++++++++++++++-
> >>  2 files changed, 50 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/hw/i386/intel_iommu.h
> >b/include/hw/i386/intel_iommu.h
> >> index bbc7b96add..c71a133820 100644
> >> --- a/include/hw/i386/intel_iommu.h
> >> +++ b/include/hw/i386/intel_iommu.h
> >> @@ -283,6 +283,7 @@ struct IntelIOMMUState {
> >>
> >>      uint64_t cap;                   /* The value of capability reg */
> >>      uint64_t ecap;                  /* The value of extended capability 
> >> reg */
> >> +    bool cap_frozen;                /* cap/ecap become read-only after 
> >> frozen */
> >>
> >>      uint32_t context_cache_gen;     /* Should be in [1,MAX] */
> >>      GHashTable *iotlb;              /* IOTLB */
> >> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> >> index ffa1ad6429..a9f9dfd6a7 100644
> >> --- a/hw/i386/intel_iommu.c
> >> +++ b/hw/i386/intel_iommu.c
> >> @@ -35,6 +35,8 @@
> >>  #include "sysemu/kvm.h"
> >>  #include "sysemu/dma.h"
> >>  #include "sysemu/sysemu.h"
> >> +#include "hw/vfio/vfio-common.h"
> >> +#include "sysemu/iommufd.h"
> >>  #include "hw/i386/apic_internal.h"
> >>  #include "kvm/kvm_i386.h"
> >>  #include "migration/vmstate.h"
> >> @@ -3819,6 +3821,38 @@ VTDAddressSpace
> >*vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus,
> >>      return vtd_dev_as;
> >>  }
> >>
> >> +static int vtd_check_legacy_hdev(IntelIOMMUState *s,
> >> +                                 IOMMULegacyDevice *ldev,
> >> +                                 Error **errp)
> >> +{
> >> +    return 0;
> >> +}
> >> +
> >> +static int vtd_check_iommufd_hdev(IntelIOMMUState *s,
> >> +                                  IOMMUFDDevice *idev,
> >> +                                  Error **errp)
> >> +{
> >> +    return 0;
> >> +}
> >> +
> >> +static int vtd_check_hdev(IntelIOMMUState *s, VTDHostIOMMUDevice
> >*vtd_hdev,
> >> +                          Error **errp)
> >> +{
> >> +    HostIOMMUDevice *base_dev = vtd_hdev->dev;
> >> +    IOMMUFDDevice *idev;
> >> +
> >> +    if (base_dev->type == HID_LEGACY) {
> >> +        IOMMULegacyDevice *ldev = container_of(base_dev,
> >> +                                               IOMMULegacyDevice, base);
> >> +
> >> +        return vtd_check_legacy_hdev(s, ldev, errp);
> >> +    }
> >> +
> >> +    idev = container_of(base_dev, IOMMUFDDevice, base);
> >> +
> >> +    return vtd_check_iommufd_hdev(s, idev, errp);
> >> +}
> >> +
> >>  static int vtd_dev_set_iommu_device(PCIBus *bus, void *opaque, int
> >devfn,
> >>                                      HostIOMMUDevice *base_dev, Error 
> >> **errp)
> >>  {
> >> @@ -3829,6 +3863,7 @@ static int vtd_dev_set_iommu_device(PCIBus
> >*bus, void *opaque, int devfn,
> >>          .devfn = devfn,
> >>      };
> >>      struct vtd_as_key *new_key;
> >> +    int ret;
> >>
> >>      assert(base_dev);
> >>
> >> @@ -3848,6 +3883,13 @@ static int vtd_dev_set_iommu_device(PCIBus
> >*bus, void *opaque, int devfn,
> >>      vtd_hdev->iommu_state = s;
> >>      vtd_hdev->dev = base_dev;
> >>
> >> +    ret = vtd_check_hdev(s, vtd_hdev, errp);
> >> +    if (ret) {
> >> +        g_free(vtd_hdev);
> >> +        vtd_iommu_unlock(s);
> >> +        return ret;
> >> +    }
> >> +
> >>      new_key = g_malloc(sizeof(*new_key));
> >>      new_key->bus = bus;
> >>      new_key->devfn = devfn;
> >
> >
> >Okay. So when VFIO device is created, it will call vtd_dev_set_iommu_device
> >and that in turn will update caps.
> >
> >
> >
> >
> >> @@ -4083,7 +4125,9 @@ static void vtd_init(IntelIOMMUState *s)
> >>      s->iq_dw = false;
> >>      s->next_frcd_reg = 0;
> >>
> >> -    vtd_cap_init(s);
> >> +    if (!s->cap_frozen) {
> >> +        vtd_cap_init(s);
> >> +    }
> >>
> >
> >If it's fronzen it's because VFIO was added after machine done.
> >And then what? I think caps are just wrong?
> 
> Not quite get your question on caps being wrong. But try to explains:
> 
> When a hot plugged vfio device's host iommu cap isn't compatible with
> vIOMMU's, hotplug should fail. Currently there is no check for this and
> allow hotplug to succeed, but then some issue will reveal later,
> e.g., vIOMMU's MGAW > host IOMMU's MGAW, guest can setup iova
> mapping beyond host supported iova range, then DMA will fail.
> 
> In fact, before this series, cap is not impacted by VFIO, so it's same effect 
> of
> frozen after machine done.
> 
> >
> >
> >I think the way to approach this is just by specifying this
> >as an option on command line.
> 
> Do you mean add a cap_frozen property to intel_iommu?
> Vtd_init() is called in realize() and system reset(), so I utilize realize() 
> to init cap
> and froze cap before system reset(). If cap_frozen is an option, when it's 
> set to
> false, cap could be updated every system reset and it's not a fix value any 
> more.
> This may break migration.

No, I mean either
1. add some kind of vfio-iommu device that is not exposed to guest
   but is not hot pluggable

or

2. add a property to specify ecap, rely on management to set it correctly


> >
> >So if one wants VFIO one has to sync caps with host.
> >No?
> 
> Yes, check for compatibility. But it's not preventing the usage of VFIO
> with vIOMMU, it finds the incompatible issue earlier and fail hotplug instead 
> of
> surprising guest driver failure.
> 
> Thanks
> Zhenzhong


I don't see where the check for compatibility and hotplug failure are.
Did I miss it?

> >
> >
> >
> >>      /*
> >>       * Rsvd field masks for spte
> >> @@ -4254,6 +4298,10 @@ 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_frozen = true;
> >> +
> >>      object_child_foreach_recursive(object_get_root(),
> >>                                     vtd_machine_done_notify_one, NULL);
> >>  }
> >> --
> >> 2.34.1