| Message ID | 20240603061023.269738-10-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a host IOMMU device abstraction to check with vIOMMU | expand |
Hi Zhenzhong, On 6/3/24 08:10, Zhenzhong Duan wrote: > It calls iommufd_backend_get_device_info() to get host IOMMU > related information and translate it into HostIOMMUDeviceCaps > for query with .get_cap(). > > Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals to > (aw_bits - 1). > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/i386/intel_iommu.h | 1 + > hw/vfio/iommufd.c | 37 +++++++++++++++++++++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 7fa0a695c8..7d694b0813 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, INTEL_IOMMU_DEVICE) > #define VTD_HOST_AW_48BIT 48 > #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT > #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) > +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) > > #define DMAR_REPORT_F_INTR (1) > > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index e4a507d55c..9d2e95e20e 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -25,6 +25,7 @@ > #include "qemu/cutils.h" > #include "qemu/chardev_open.h" > #include "pci.h" > +#include "hw/i386/intel_iommu_internal.h" > > static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, hwaddr iova, > ram_addr_t size, void *vaddr, bool readonly) > @@ -619,6 +620,41 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) > vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; > }; > > +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, > + Error **errp) > +{ > + VFIODevice *vdev = opaque; > + HostIOMMUDeviceCaps *caps = &hiod->caps; > + enum iommu_hw_info_type type; > + union { > + struct iommu_hw_info_vtd vtd; > + } data; > + > + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, > + &type, &data, sizeof(data), errp)) { > + return false; > + } > + > + caps->type = type; > + > + switch (type) { > + case IOMMU_HW_INFO_TYPE_INTEL_VTD: > + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; Please can you remind me of why you can't reuse the iova_ranges method. isn't it generic enough? > + break; > + case IOMMU_HW_INFO_TYPE_NONE: so what about other types? Eric > + break; > + } > + > + return true; > +} > + > +static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data) > +{ > + HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc); > + > + hiodc->realize = hiod_iommufd_vfio_realize; > +}; > + > static const TypeInfo types[] = { > { > .name = TYPE_VFIO_IOMMU_IOMMUFD, > @@ -627,6 +663,7 @@ static const TypeInfo types[] = { > }, { > .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO, > .parent = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, > + .class_init = hiod_iommufd_vfio_class_init, > } > }; >
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >HostIOMMUDeviceClass::realize() handler > >Hi Zhenzhong, > >On 6/3/24 08:10, Zhenzhong Duan wrote: >> It calls iommufd_backend_get_device_info() to get host IOMMU >> related information and translate it into HostIOMMUDeviceCaps >> for query with .get_cap(). >> >> Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals to >> (aw_bits - 1). >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/i386/intel_iommu.h | 1 + >> hw/vfio/iommufd.c | 37 >+++++++++++++++++++++++++++++++++++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/include/hw/i386/intel_iommu.h >b/include/hw/i386/intel_iommu.h >> index 7fa0a695c8..7d694b0813 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, >INTEL_IOMMU_DEVICE) >> #define VTD_HOST_AW_48BIT 48 >> #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT >> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) >> +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) >> >> #define DMAR_REPORT_F_INTR (1) >> >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index e4a507d55c..9d2e95e20e 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -25,6 +25,7 @@ >> #include "qemu/cutils.h" >> #include "qemu/chardev_open.h" >> #include "pci.h" >> +#include "hw/i386/intel_iommu_internal.h" >> >> static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, >hwaddr iova, >> ram_addr_t size, void *vaddr, bool readonly) >> @@ -619,6 +620,41 @@ static void >vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) >> vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; >> }; >> >> +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void >*opaque, >> + Error **errp) >> +{ >> + VFIODevice *vdev = opaque; >> + HostIOMMUDeviceCaps *caps = &hiod->caps; >> + enum iommu_hw_info_type type; >> + union { >> + struct iommu_hw_info_vtd vtd; >> + } data; >> + >> + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, >> + &type, &data, sizeof(data), errp)) { >> + return false; >> + } >> + >> + caps->type = type; >> + >> + switch (type) { >> + case IOMMU_HW_INFO_TYPE_INTEL_VTD: >> + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; >Please can you remind me of why you can't reuse the iova_ranges method. >isn't it generic enough? Yes, iova_ranges method is only for iova_ranges, we want to make HostIOMMUDevice.get_cap() a common interface. When we want to pass iova_ranges, we can add HOST_IOMMU_DEVICE_CAP_IOVA_RANGES and HostIOMMUDevice.iova_ranges. >> + break; >> + case IOMMU_HW_INFO_TYPE_NONE: >so what about other types? There is no other types for now. When there is, we can easily add the code: case IOMMU_HW_INFO_TYPE_ARM_SMMU: caps->aw_bits = xxx; Thanks Zhenzhong > >Eric >> + break; >> + } >> + >> + return true; >> +} >> + >> +static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data) >> +{ >> + HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc); >> + >> + hiodc->realize = hiod_iommufd_vfio_realize; >> +}; >> + >> static const TypeInfo types[] = { >> { >> .name = TYPE_VFIO_IOMMU_IOMMUFD, >> @@ -627,6 +663,7 @@ static const TypeInfo types[] = { >> }, { >> .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO, >> .parent = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, >> + .class_init = hiod_iommufd_vfio_class_init, >> } >> }; >>
On 6/4/24 04:58, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >> HostIOMMUDeviceClass::realize() handler >> >> Hi Zhenzhong, >> >> On 6/3/24 08:10, Zhenzhong Duan wrote: >>> It calls iommufd_backend_get_device_info() to get host IOMMU >>> related information and translate it into HostIOMMUDeviceCaps >>> for query with .get_cap(). >>> >>> Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals to >>> (aw_bits - 1). >>> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/i386/intel_iommu.h | 1 + >>> hw/vfio/iommufd.c | 37 >> +++++++++++++++++++++++++++++++++++ >>> 2 files changed, 38 insertions(+) >>> >>> diff --git a/include/hw/i386/intel_iommu.h >> b/include/hw/i386/intel_iommu.h >>> index 7fa0a695c8..7d694b0813 100644 >>> --- a/include/hw/i386/intel_iommu.h >>> +++ b/include/hw/i386/intel_iommu.h >>> @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, >> INTEL_IOMMU_DEVICE) >>> #define VTD_HOST_AW_48BIT 48 >>> #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT >>> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) >>> +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) >>> >>> #define DMAR_REPORT_F_INTR (1) >>> >>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>> index e4a507d55c..9d2e95e20e 100644 >>> --- a/hw/vfio/iommufd.c >>> +++ b/hw/vfio/iommufd.c >>> @@ -25,6 +25,7 @@ >>> #include "qemu/cutils.h" >>> #include "qemu/chardev_open.h" >>> #include "pci.h" >>> +#include "hw/i386/intel_iommu_internal.h" >>> >>> static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, >> hwaddr iova, >>> ram_addr_t size, void *vaddr, bool readonly) >>> @@ -619,6 +620,41 @@ static void >> vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) >>> vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; >>> }; >>> >>> +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void >> *opaque, >>> + Error **errp) >>> +{ >>> + VFIODevice *vdev = opaque; >>> + HostIOMMUDeviceCaps *caps = &hiod->caps; >>> + enum iommu_hw_info_type type; >>> + union { >>> + struct iommu_hw_info_vtd vtd; >>> + } data; >>> + >>> + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, >>> + &type, &data, sizeof(data), errp)) { >>> + return false; >>> + } >>> + >>> + caps->type = type; >>> + >>> + switch (type) { >>> + case IOMMU_HW_INFO_TYPE_INTEL_VTD: >>> + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; >> Please can you remind me of why you can't reuse the iova_ranges method. >> isn't it generic enough? > Yes, iova_ranges method is only for iova_ranges, we want to make > HostIOMMUDevice.get_cap() a common interface. > > When we want to pass iova_ranges, we can add HOST_IOMMU_DEVICE_CAP_IOVA_RANGES > and HostIOMMUDevice.iova_ranges. I rather meant that iova_ranges is part of VFIOContainerBase and you could reuse the technics used in hiod_legacy_vfio_realize, relying on a common helper instead of using VTD_MGAW_FROM_CAP(data.vtd.cap_reg). Doesn't it work? > >>> + break; >>> + case IOMMU_HW_INFO_TYPE_NONE: >> so what about other types? > There is no other types for now. When there is, we can easily add the code Thanks Eric > > case IOMMU_HW_INFO_TYPE_ARM_SMMU: > caps->aw_bits = xxx; > > Thanks > Zhenzhong > >> Eric >>> + break; >>> + } >>> + >>> + return true; >>> +} >>> + >>> +static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data) >>> +{ >>> + HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc); >>> + >>> + hiodc->realize = hiod_iommufd_vfio_realize; >>> +}; >>> + >>> static const TypeInfo types[] = { >>> { >>> .name = TYPE_VFIO_IOMMU_IOMMUFD, >>> @@ -627,6 +663,7 @@ static const TypeInfo types[] = { >>> }, { >>> .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO, >>> .parent = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, >>> + .class_init = hiod_iommufd_vfio_class_init, >>> } >>> }; >>>
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >HostIOMMUDeviceClass::realize() handler > > > >On 6/4/24 04:58, Duan, Zhenzhong wrote: >> >>> -----Original Message----- >>> From: Eric Auger <eric.auger@redhat.com> >>> Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >>> HostIOMMUDeviceClass::realize() handler >>> >>> Hi Zhenzhong, >>> >>> On 6/3/24 08:10, Zhenzhong Duan wrote: >>>> It calls iommufd_backend_get_device_info() to get host IOMMU >>>> related information and translate it into HostIOMMUDeviceCaps >>>> for query with .get_cap(). >>>> >>>> Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals >to >>>> (aw_bits - 1). >>>> >>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>> --- >>>> include/hw/i386/intel_iommu.h | 1 + >>>> hw/vfio/iommufd.c | 37 >>> +++++++++++++++++++++++++++++++++++ >>>> 2 files changed, 38 insertions(+) >>>> >>>> diff --git a/include/hw/i386/intel_iommu.h >>> b/include/hw/i386/intel_iommu.h >>>> index 7fa0a695c8..7d694b0813 100644 >>>> --- a/include/hw/i386/intel_iommu.h >>>> +++ b/include/hw/i386/intel_iommu.h >>>> @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, >>> INTEL_IOMMU_DEVICE) >>>> #define VTD_HOST_AW_48BIT 48 >>>> #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT >>>> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) >>>> +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) >>>> >>>> #define DMAR_REPORT_F_INTR (1) >>>> >>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>>> index e4a507d55c..9d2e95e20e 100644 >>>> --- a/hw/vfio/iommufd.c >>>> +++ b/hw/vfio/iommufd.c >>>> @@ -25,6 +25,7 @@ >>>> #include "qemu/cutils.h" >>>> #include "qemu/chardev_open.h" >>>> #include "pci.h" >>>> +#include "hw/i386/intel_iommu_internal.h" >>>> >>>> static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, >>> hwaddr iova, >>>> ram_addr_t size, void *vaddr, bool readonly) >>>> @@ -619,6 +620,41 @@ static void >>> vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) >>>> vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; >>>> }; >>>> >>>> +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void >>> *opaque, >>>> + Error **errp) >>>> +{ >>>> + VFIODevice *vdev = opaque; >>>> + HostIOMMUDeviceCaps *caps = &hiod->caps; >>>> + enum iommu_hw_info_type type; >>>> + union { >>>> + struct iommu_hw_info_vtd vtd; >>>> + } data; >>>> + >>>> + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev- >>devid, >>>> + &type, &data, sizeof(data), errp)) { >>>> + return false; >>>> + } >>>> + >>>> + caps->type = type; >>>> + >>>> + switch (type) { >>>> + case IOMMU_HW_INFO_TYPE_INTEL_VTD: >>>> + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; >>> Please can you remind me of why you can't reuse the iova_ranges >method. >>> isn't it generic enough? >> Yes, iova_ranges method is only for iova_ranges, we want to make >> HostIOMMUDevice.get_cap() a common interface. >> >> When we want to pass iova_ranges, we can add >HOST_IOMMU_DEVICE_CAP_IOVA_RANGES >> and HostIOMMUDevice.iova_ranges. > >I rather meant that iova_ranges is part of VFIOContainerBase and you >could reuse the technics used in hiod_legacy_vfio_realize, relying on a >common helper instead of using > >VTD_MGAW_FROM_CAP(data.vtd.cap_reg). Doesn't it work? Get your point. Yes, It does work and should have same result. That means iommufd backend support two ways to get aw_bits. Only reason is I feel VTD_MGAW_FROM_CAP(data.vtd.cap_reg) is a bit simpler and there are other bits picked in nesting series, see: case IOMMU_HW_INFO_TYPE_INTEL_VTD: caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; caps->nesting = !!(data.vtd.ecap_reg & VTD_ECAP_NEST); caps->fs1gp = !!(data.vtd.cap_reg & VTD_CAP_FS1GP); caps->errata = data.vtd.flags & IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17; I'm fine to use iova_ranges to calculate aw_bits for iommufd backend if you prefer that. Thanks Zhenzhong
Hi, On 6/4/24 09:51, Duan, Zhenzhong wrote: > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >> HostIOMMUDeviceClass::realize() handler >> >> >> >> On 6/4/24 04:58, Duan, Zhenzhong wrote: >>>> -----Original Message----- >>>> From: Eric Auger <eric.auger@redhat.com> >>>> Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >>>> HostIOMMUDeviceClass::realize() handler >>>> >>>> Hi Zhenzhong, >>>> >>>> On 6/3/24 08:10, Zhenzhong Duan wrote: >>>>> It calls iommufd_backend_get_device_info() to get host IOMMU >>>>> related information and translate it into HostIOMMUDeviceCaps >>>>> for query with .get_cap(). >>>>> >>>>> Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals >> to >>>>> (aw_bits - 1). >>>>> >>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>> --- >>>>> include/hw/i386/intel_iommu.h | 1 + >>>>> hw/vfio/iommufd.c | 37 >>>> +++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 38 insertions(+) >>>>> >>>>> diff --git a/include/hw/i386/intel_iommu.h >>>> b/include/hw/i386/intel_iommu.h >>>>> index 7fa0a695c8..7d694b0813 100644 >>>>> --- a/include/hw/i386/intel_iommu.h >>>>> +++ b/include/hw/i386/intel_iommu.h >>>>> @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, >>>> INTEL_IOMMU_DEVICE) >>>>> #define VTD_HOST_AW_48BIT 48 >>>>> #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT >>>>> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) >>>>> +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) >>>>> >>>>> #define DMAR_REPORT_F_INTR (1) >>>>> >>>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>>>> index e4a507d55c..9d2e95e20e 100644 >>>>> --- a/hw/vfio/iommufd.c >>>>> +++ b/hw/vfio/iommufd.c >>>>> @@ -25,6 +25,7 @@ >>>>> #include "qemu/cutils.h" >>>>> #include "qemu/chardev_open.h" >>>>> #include "pci.h" >>>>> +#include "hw/i386/intel_iommu_internal.h" >>>>> >>>>> static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, >>>> hwaddr iova, >>>>> ram_addr_t size, void *vaddr, bool readonly) >>>>> @@ -619,6 +620,41 @@ static void >>>> vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) >>>>> vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; >>>>> }; >>>>> >>>>> +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void >>>> *opaque, >>>>> + Error **errp) >>>>> +{ >>>>> + VFIODevice *vdev = opaque; >>>>> + HostIOMMUDeviceCaps *caps = &hiod->caps; >>>>> + enum iommu_hw_info_type type; >>>>> + union { >>>>> + struct iommu_hw_info_vtd vtd; >>>>> + } data; >>>>> + >>>>> + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev- >>> devid, >>>>> + &type, &data, sizeof(data), errp)) { >>>>> + return false; >>>>> + } >>>>> + >>>>> + caps->type = type; >>>>> + >>>>> + switch (type) { >>>>> + case IOMMU_HW_INFO_TYPE_INTEL_VTD: >>>>> + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; >>>> Please can you remind me of why you can't reuse the iova_ranges >> method. >>>> isn't it generic enough? >>> Yes, iova_ranges method is only for iova_ranges, we want to make >>> HostIOMMUDevice.get_cap() a common interface. >>> >>> When we want to pass iova_ranges, we can add >> HOST_IOMMU_DEVICE_CAP_IOVA_RANGES >>> and HostIOMMUDevice.iova_ranges. >> I rather meant that iova_ranges is part of VFIOContainerBase and you >> could reuse the technics used in hiod_legacy_vfio_realize, relying on a >> common helper instead of using >> >> VTD_MGAW_FROM_CAP(data.vtd.cap_reg). Doesn't it work? > Get your point. > Yes, It does work and should have same result. > That means iommufd backend support two ways to get aw_bits. > > Only reason is I feel VTD_MGAW_FROM_CAP(data.vtd.cap_reg) is a bit simpler > and there are other bits picked in nesting series, see: > > case IOMMU_HW_INFO_TYPE_INTEL_VTD: > caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; > caps->nesting = !!(data.vtd.ecap_reg & VTD_ECAP_NEST); > caps->fs1gp = !!(data.vtd.cap_reg & VTD_CAP_FS1GP); > caps->errata = data.vtd.flags & IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17; > > I'm fine to use iova_ranges to calculate aw_bits for iommufd backend if you prefer that. Yes I think I would prefer because this technics also work for other iommus and not only VTD. It also can rely on common code between legacy and iommufd. The nesting series can bring the rest later Eric > > Thanks > Zhenzhong
>-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >HostIOMMUDeviceClass::realize() handler > > >Hi, >On 6/4/24 09:51, Duan, Zhenzhong wrote: >> >>> -----Original Message----- >>> From: Eric Auger <eric.auger@redhat.com> >>> Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >>> HostIOMMUDeviceClass::realize() handler >>> >>> >>> >>> On 6/4/24 04:58, Duan, Zhenzhong wrote: >>>>> -----Original Message----- >>>>> From: Eric Auger <eric.auger@redhat.com> >>>>> Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >>>>> HostIOMMUDeviceClass::realize() handler >>>>> >>>>> Hi Zhenzhong, >>>>> >>>>> On 6/3/24 08:10, Zhenzhong Duan wrote: >>>>>> It calls iommufd_backend_get_device_info() to get host IOMMU >>>>>> related information and translate it into HostIOMMUDeviceCaps >>>>>> for query with .get_cap(). >>>>>> >>>>>> Introduce macro VTD_MGAW_FROM_CAP to get MGAW which >equals >>> to >>>>>> (aw_bits - 1). >>>>>> >>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>>>>> --- >>>>>> include/hw/i386/intel_iommu.h | 1 + >>>>>> hw/vfio/iommufd.c | 37 >>>>> +++++++++++++++++++++++++++++++++++ >>>>>> 2 files changed, 38 insertions(+) >>>>>> >>>>>> diff --git a/include/hw/i386/intel_iommu.h >>>>> b/include/hw/i386/intel_iommu.h >>>>>> index 7fa0a695c8..7d694b0813 100644 >>>>>> --- a/include/hw/i386/intel_iommu.h >>>>>> +++ b/include/hw/i386/intel_iommu.h >>>>>> @@ -47,6 +47,7 @@ >OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, >>>>> INTEL_IOMMU_DEVICE) >>>>>> #define VTD_HOST_AW_48BIT 48 >>>>>> #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT >>>>>> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) >>>>>> +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) >>>>>> >>>>>> #define DMAR_REPORT_F_INTR (1) >>>>>> >>>>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>>>>> index e4a507d55c..9d2e95e20e 100644 >>>>>> --- a/hw/vfio/iommufd.c >>>>>> +++ b/hw/vfio/iommufd.c >>>>>> @@ -25,6 +25,7 @@ >>>>>> #include "qemu/cutils.h" >>>>>> #include "qemu/chardev_open.h" >>>>>> #include "pci.h" >>>>>> +#include "hw/i386/intel_iommu_internal.h" >>>>>> >>>>>> static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, >>>>> hwaddr iova, >>>>>> ram_addr_t size, void *vaddr, bool readonly) >>>>>> @@ -619,6 +620,41 @@ static void >>>>> vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) >>>>>> vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; >>>>>> }; >>>>>> >>>>>> +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, >void >>>>> *opaque, >>>>>> + Error **errp) >>>>>> +{ >>>>>> + VFIODevice *vdev = opaque; >>>>>> + HostIOMMUDeviceCaps *caps = &hiod->caps; >>>>>> + enum iommu_hw_info_type type; >>>>>> + union { >>>>>> + struct iommu_hw_info_vtd vtd; >>>>>> + } data; >>>>>> + >>>>>> + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev- >>>> devid, >>>>>> + &type, &data, sizeof(data), errp)) { >>>>>> + return false; >>>>>> + } >>>>>> + >>>>>> + caps->type = type; >>>>>> + >>>>>> + switch (type) { >>>>>> + case IOMMU_HW_INFO_TYPE_INTEL_VTD: >>>>>> + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + >1; >>>>> Please can you remind me of why you can't reuse the iova_ranges >>> method. >>>>> isn't it generic enough? >>>> Yes, iova_ranges method is only for iova_ranges, we want to make >>>> HostIOMMUDevice.get_cap() a common interface. >>>> >>>> When we want to pass iova_ranges, we can add >>> HOST_IOMMU_DEVICE_CAP_IOVA_RANGES >>>> and HostIOMMUDevice.iova_ranges. >>> I rather meant that iova_ranges is part of VFIOContainerBase and you >>> could reuse the technics used in hiod_legacy_vfio_realize, relying on a >>> common helper instead of using >>> >>> VTD_MGAW_FROM_CAP(data.vtd.cap_reg). Doesn't it work? >> Get your point. >> Yes, It does work and should have same result. >> That means iommufd backend support two ways to get aw_bits. >> >> Only reason is I feel VTD_MGAW_FROM_CAP(data.vtd.cap_reg) is a bit >simpler >> and there are other bits picked in nesting series, see: >> >> case IOMMU_HW_INFO_TYPE_INTEL_VTD: >> caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; >> caps->nesting = !!(data.vtd.ecap_reg & VTD_ECAP_NEST); >> caps->fs1gp = !!(data.vtd.cap_reg & VTD_CAP_FS1GP); >> caps->errata = data.vtd.flags & >IOMMU_HW_INFO_VTD_ERRATA_772415_SPR17; >> >> I'm fine to use iova_ranges to calculate aw_bits for iommufd backend if >you prefer that. >Yes I think I would prefer because this technics also work for other >iommus and not only VTD. It also can rely on common code between legacy >and iommufd. The nesting series can bring the rest later OK, will do. Thanks Zhenzhong
Hi Zhenzhong, On 6/3/24 08:10, Zhenzhong Duan wrote: > It calls iommufd_backend_get_device_info() to get host IOMMU > related information and translate it into HostIOMMUDeviceCaps > for query with .get_cap(). > > Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals to > (aw_bits - 1). > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/hw/i386/intel_iommu.h | 1 + > hw/vfio/iommufd.c | 37 +++++++++++++++++++++++++++++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h > index 7fa0a695c8..7d694b0813 100644 > --- a/include/hw/i386/intel_iommu.h > +++ b/include/hw/i386/intel_iommu.h > @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, INTEL_IOMMU_DEVICE) > #define VTD_HOST_AW_48BIT 48 > #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT > #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) > +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) > > #define DMAR_REPORT_F_INTR (1) > > diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c > index e4a507d55c..9d2e95e20e 100644 > --- a/hw/vfio/iommufd.c > +++ b/hw/vfio/iommufd.c > @@ -25,6 +25,7 @@ > #include "qemu/cutils.h" > #include "qemu/chardev_open.h" > #include "pci.h" > +#include "hw/i386/intel_iommu_internal.h" > > static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, hwaddr iova, > ram_addr_t size, void *vaddr, bool readonly) > @@ -619,6 +620,41 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) > vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; > }; > > +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, > + Error **errp) > +{ > + VFIODevice *vdev = opaque; I think it would make sense to store vdev in hiod. This would allow to postpone some computations in the HostIOMMUDevice ops instead of doing everything in the realize. For instance to retrieve the usable iova_ranges I will need to access the base container in the associated ops. Thanks Eric > + HostIOMMUDeviceCaps *caps = &hiod->caps; > + enum iommu_hw_info_type type; > + union { > + struct iommu_hw_info_vtd vtd; > + } data; > + > + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, > + &type, &data, sizeof(data), errp)) { > + return false; > + } > + > + caps->type = type; > + > + switch (type) { > + case IOMMU_HW_INFO_TYPE_INTEL_VTD: > + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; > + break; > + case IOMMU_HW_INFO_TYPE_NONE: > + break; > + } > + > + return true; > +} > + > +static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data) > +{ > + HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc); > + > + hiodc->realize = hiod_iommufd_vfio_realize; > +}; > + > static const TypeInfo types[] = { > { > .name = TYPE_VFIO_IOMMU_IOMMUFD, > @@ -627,6 +663,7 @@ static const TypeInfo types[] = { > }, { > .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO, > .parent = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, > + .class_init = hiod_iommufd_vfio_class_init, > } > }; >
On 6/6/24 11:26, Eric Auger wrote: > Hi Zhenzhong, > On 6/3/24 08:10, Zhenzhong Duan wrote: >> It calls iommufd_backend_get_device_info() to get host IOMMU >> related information and translate it into HostIOMMUDeviceCaps >> for query with .get_cap(). >> >> Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals to >> (aw_bits - 1). >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/hw/i386/intel_iommu.h | 1 + >> hw/vfio/iommufd.c | 37 +++++++++++++++++++++++++++++++++++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h >> index 7fa0a695c8..7d694b0813 100644 >> --- a/include/hw/i386/intel_iommu.h >> +++ b/include/hw/i386/intel_iommu.h >> @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, INTEL_IOMMU_DEVICE) >> #define VTD_HOST_AW_48BIT 48 >> #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT >> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) >> +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) >> >> #define DMAR_REPORT_F_INTR (1) >> >> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >> index e4a507d55c..9d2e95e20e 100644 >> --- a/hw/vfio/iommufd.c >> +++ b/hw/vfio/iommufd.c >> @@ -25,6 +25,7 @@ >> #include "qemu/cutils.h" >> #include "qemu/chardev_open.h" >> #include "pci.h" >> +#include "hw/i386/intel_iommu_internal.h" >> >> static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, hwaddr iova, >> ram_addr_t size, void *vaddr, bool readonly) >> @@ -619,6 +620,41 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) >> vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; >> }; >> >> +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, >> + Error **errp) >> +{ >> + VFIODevice *vdev = opaque; > I think it would make sense to store vdev in hiod. This would allow to > postpone some computations in the HostIOMMUDevice ops instead of doing > everything in the realize. > For instance to retrieve the usable iova_ranges I will need to access > the base container in the associated ops. this would need to be opaque since the agent device can be either VFIODevice or VDPA object though Eric > > Thanks > > Eric >> + HostIOMMUDeviceCaps *caps = &hiod->caps; >> + enum iommu_hw_info_type type; >> + union { >> + struct iommu_hw_info_vtd vtd; >> + } data; >> + >> + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, >> + &type, &data, sizeof(data), errp)) { >> + return false; >> + } >> + >> + caps->type = type; >> + >> + switch (type) { >> + case IOMMU_HW_INFO_TYPE_INTEL_VTD: >> + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; >> + break; >> + case IOMMU_HW_INFO_TYPE_NONE: >> + break; >> + } >> + >> + return true; >> +} >> + >> +static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data) >> +{ >> + HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc); >> + >> + hiodc->realize = hiod_iommufd_vfio_realize; >> +}; >> + >> static const TypeInfo types[] = { >> { >> .name = TYPE_VFIO_IOMMU_IOMMUFD, >> @@ -627,6 +663,7 @@ static const TypeInfo types[] = { >> }, { >> .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO, >> .parent = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, >> + .class_init = hiod_iommufd_vfio_class_init, >> } >> }; >> >
Hi Eric, >-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH v6 09/19] vfio/iommufd: Implement >HostIOMMUDeviceClass::realize() handler > > > >On 6/6/24 11:26, Eric Auger wrote: >> Hi Zhenzhong, >> On 6/3/24 08:10, Zhenzhong Duan wrote: >>> It calls iommufd_backend_get_device_info() to get host IOMMU >>> related information and translate it into HostIOMMUDeviceCaps >>> for query with .get_cap(). >>> >>> Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals to >>> (aw_bits - 1). >>> >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/hw/i386/intel_iommu.h | 1 + >>> hw/vfio/iommufd.c | 37 >+++++++++++++++++++++++++++++++++++ >>> 2 files changed, 38 insertions(+) >>> >>> diff --git a/include/hw/i386/intel_iommu.h >b/include/hw/i386/intel_iommu.h >>> index 7fa0a695c8..7d694b0813 100644 >>> --- a/include/hw/i386/intel_iommu.h >>> +++ b/include/hw/i386/intel_iommu.h >>> @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, >INTEL_IOMMU_DEVICE) >>> #define VTD_HOST_AW_48BIT 48 >>> #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT >>> #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) >>> +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) >>> >>> #define DMAR_REPORT_F_INTR (1) >>> >>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c >>> index e4a507d55c..9d2e95e20e 100644 >>> --- a/hw/vfio/iommufd.c >>> +++ b/hw/vfio/iommufd.c >>> @@ -25,6 +25,7 @@ >>> #include "qemu/cutils.h" >>> #include "qemu/chardev_open.h" >>> #include "pci.h" >>> +#include "hw/i386/intel_iommu_internal.h" >>> >>> static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, >hwaddr iova, >>> ram_addr_t size, void *vaddr, bool readonly) >>> @@ -619,6 +620,41 @@ static void >vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) >>> vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; >>> }; >>> >>> +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void >*opaque, >>> + Error **errp) >>> +{ >>> + VFIODevice *vdev = opaque; >> I think it would make sense to store vdev in hiod. This would allow to >> postpone some computations in the HostIOMMUDevice ops instead of >doing >> everything in the realize. >> For instance to retrieve the usable iova_ranges I will need to access >> the base container in the associated ops. > >this would need to be opaque since the agent device can be either >VFIODevice or VDPA object though This will give vIOMMU access to all VFIODevice or VDPA object elements and I'm not sure if VDPA supports iova_ranges. What about exposing only what we need, like below. If VDPA doesn't support iova_ranges, get_cap() should return 0. --- a/include/sysemu/host_iommu_device.h +++ b/include/sysemu/host_iommu_device.h @@ -32,6 +32,7 @@ typedef struct HostIOMMUDeviceCaps { bool nesting; bool fs1gp; uint32_t errata; + GList *iova_ranges; } HostIOMMUDeviceCaps; #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" @@ -96,6 +97,7 @@ struct HostIOMMUDeviceClass { #define HOST_IOMMU_DEVICE_CAP_NESTING 2 #define HOST_IOMMU_DEVICE_CAP_FS1GP 3 #define HOST_IOMMU_DEVICE_CAP_ERRATA 4 +#define HOST_IOMMU_DEVICE_CAP_IOVA_RANGES 5 /** * enum host_iommu_device_iommu_hw_info_type - IOMMU Hardware Info Types diff --git a/hw/vfio/container.c b/hw/vfio/container.c index 26e6f7fb4f..4c3e9e45c3 100644 --- a/hw/vfio/container.c +++ b/hw/vfio/container.c @@ -1145,6 +1145,7 @@ static bool hiod_legacy_vfio_realize(HostIOMMUDevice *hiod, void *opaque, hiod->name = g_strdup(vdev->name); hiod->caps.aw_bits = vfio_device_get_aw_bits(vdev); + hiod->caps.iova_ranges = vdev->bcontainer->iova_ranges; return true; } @@ -1157,6 +1158,8 @@ static int hiod_legacy_vfio_get_cap(HostIOMMUDevice *hiod, int cap, switch (cap) { case HOST_IOMMU_DEVICE_CAP_AW_BITS: return caps->aw_bits; + case HOST_IOMMU_DEVICE_CAP_IOVA_RANGES: + return 1; default: error_setg(errp, "%s: unsupported capability %x", hiod->name, cap); return -EINVAL; Thanks Zhenzhong
diff --git a/include/hw/i386/intel_iommu.h b/include/hw/i386/intel_iommu.h index 7fa0a695c8..7d694b0813 100644 --- a/include/hw/i386/intel_iommu.h +++ b/include/hw/i386/intel_iommu.h @@ -47,6 +47,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(IntelIOMMUState, INTEL_IOMMU_DEVICE) #define VTD_HOST_AW_48BIT 48 #define VTD_HOST_ADDRESS_WIDTH VTD_HOST_AW_39BIT #define VTD_HAW_MASK(aw) ((1ULL << (aw)) - 1) +#define VTD_MGAW_FROM_CAP(cap) ((cap >> 16) & 0x3fULL) #define DMAR_REPORT_F_INTR (1) diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c index e4a507d55c..9d2e95e20e 100644 --- a/hw/vfio/iommufd.c +++ b/hw/vfio/iommufd.c @@ -25,6 +25,7 @@ #include "qemu/cutils.h" #include "qemu/chardev_open.h" #include "pci.h" +#include "hw/i386/intel_iommu_internal.h" static int iommufd_cdev_map(const VFIOContainerBase *bcontainer, hwaddr iova, ram_addr_t size, void *vaddr, bool readonly) @@ -619,6 +620,41 @@ static void vfio_iommu_iommufd_class_init(ObjectClass *klass, void *data) vioc->pci_hot_reset = iommufd_cdev_pci_hot_reset; }; +static bool hiod_iommufd_vfio_realize(HostIOMMUDevice *hiod, void *opaque, + Error **errp) +{ + VFIODevice *vdev = opaque; + HostIOMMUDeviceCaps *caps = &hiod->caps; + enum iommu_hw_info_type type; + union { + struct iommu_hw_info_vtd vtd; + } data; + + if (!iommufd_backend_get_device_info(vdev->iommufd, vdev->devid, + &type, &data, sizeof(data), errp)) { + return false; + } + + caps->type = type; + + switch (type) { + case IOMMU_HW_INFO_TYPE_INTEL_VTD: + caps->aw_bits = VTD_MGAW_FROM_CAP(data.vtd.cap_reg) + 1; + break; + case IOMMU_HW_INFO_TYPE_NONE: + break; + } + + return true; +} + +static void hiod_iommufd_vfio_class_init(ObjectClass *oc, void *data) +{ + HostIOMMUDeviceClass *hiodc = HOST_IOMMU_DEVICE_CLASS(oc); + + hiodc->realize = hiod_iommufd_vfio_realize; +}; + static const TypeInfo types[] = { { .name = TYPE_VFIO_IOMMU_IOMMUFD, @@ -627,6 +663,7 @@ static const TypeInfo types[] = { }, { .name = TYPE_HOST_IOMMU_DEVICE_IOMMUFD_VFIO, .parent = TYPE_HOST_IOMMU_DEVICE_IOMMUFD, + .class_init = hiod_iommufd_vfio_class_init, } };
It calls iommufd_backend_get_device_info() to get host IOMMU related information and translate it into HostIOMMUDeviceCaps for query with .get_cap(). Introduce macro VTD_MGAW_FROM_CAP to get MGAW which equals to (aw_bits - 1). Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/hw/i386/intel_iommu.h | 1 + hw/vfio/iommufd.c | 37 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+)