>-----Original Message-----
>From: Michael S. Tsirkin <m...@redhat.com>
>Subject: Re: [PATCH v1 3/6] intel_iommu: Add a framework to check and
>sync host IOMMU cap/ecap
>
>On Wed, Mar 13, 2024 at 02:52:39AM +0000, Duan, Zhenzhong wrote:
>> Hi Michael,
>>
>> >-----Original Message-----
>> >From: Michael S. Tsirkin <m...@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....@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....@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 | 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
Not quite get, what will such vfio-iommu device be used for if not exposed to
guest.
If we just want to avoid vfio device hotplug, we can use
TYPE_VFIO_PCI_NOHOTPLUG device. But even if we use coldplugged
vfio device, we will face same compatibility issue of vIOMMU caps vs. host caps.
>
>or
>
>2. add a property to specify ecap, rely on management to set it correctly
Yes. This deep customization could works, but I think it's difficult to teach
user
about how to set each bit in cap/ecaps.
>
>
>> >
>> >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?
There is some check code in patch4, see
https://lists.gnu.org/archive/html/qemu-devel/2024-02/msg06419.html
And more widely used in nesting series's patch6, see
https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg02746.html
I have not sent a new version of nesting series yet, but the code related to
caps check/sync will be
similar.
Thanks
Zhenzhong
