Message ID | 20250219082228.3303163-7-zhenzhong.duan@intel.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | intel_iommu: Enable stage-1 translation for passthrough device | expand |
Hi Zhenzhong, On 2/19/25 9:22 AM, Zhenzhong Duan wrote: > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > --- > include/system/host_iommu_device.h | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/include/system/host_iommu_device.h b/include/system/host_iommu_device.h > index df782598f2..18f8b5e5cf 100644 > --- a/include/system/host_iommu_device.h > +++ b/include/system/host_iommu_device.h > @@ -22,10 +22,16 @@ > * > * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents > * the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl) > + * > + * @nesting: nesting page table support. > + * > + * @fs1gp: first stage(a.k.a, Stage-1) 1GB huge page support. > */ > typedef struct HostIOMMUDeviceCaps { > uint32_t type; > uint64_t hw_caps; > + bool nesting; > + bool fs1gp; this looks quite vtd specific, isn't it? Shouldn't we hide this is a vendor specific cap struct? > } HostIOMMUDeviceCaps; > > #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" > @@ -122,6 +128,8 @@ struct HostIOMMUDeviceClass { > */ > #define HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE 0 > #define HOST_IOMMU_DEVICE_CAP_AW_BITS 1 > +#define HOST_IOMMU_DEVICE_CAP_NESTING 2 > +#define HOST_IOMMU_DEVICE_CAP_FS1GP 3 > > #define HOST_IOMMU_DEVICE_CAP_AW_BITS_MAX 64 > #endif Maybe you could introduce the associated implementation of hiod_iommufd_get_cap in this patch too? Eric
On 2/20/25 7:41 PM, Eric Auger wrote: > Hi Zhenzhong, > > > On 2/19/25 9:22 AM, Zhenzhong Duan wrote: >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/system/host_iommu_device.h | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/include/system/host_iommu_device.h b/include/system/host_iommu_device.h >> index df782598f2..18f8b5e5cf 100644 >> --- a/include/system/host_iommu_device.h >> +++ b/include/system/host_iommu_device.h >> @@ -22,10 +22,16 @@ >> * >> * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents >> * the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl) >> + * >> + * @nesting: nesting page table support. >> + * >> + * @fs1gp: first stage(a.k.a, Stage-1) 1GB huge page support. >> */ >> typedef struct HostIOMMUDeviceCaps { >> uint32_t type; >> uint64_t hw_caps; >> + bool nesting; >> + bool fs1gp; > this looks quite vtd specific, isn't it? Shouldn't we hide this is a > vendor specific cap struct? >> } HostIOMMUDeviceCaps; >> >> #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" >> @@ -122,6 +128,8 @@ struct HostIOMMUDeviceClass { >> */ >> #define HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE 0 >> #define HOST_IOMMU_DEVICE_CAP_AW_BITS 1 >> +#define HOST_IOMMU_DEVICE_CAP_NESTING 2 >> +#define HOST_IOMMU_DEVICE_CAP_FS1GP 3 >> >> #define HOST_IOMMU_DEVICE_CAP_AW_BITS_MAX 64 >> #endif > > Maybe you could introduce the associated implementation of > hiod_iommufd_get_cap in this patch too? ignore this last comment :( Eric > > Eric
Hi Eric, >-----Original Message----- >From: Eric Auger <eric.auger@redhat.com> >Subject: Re: [PATCH rfcv2 06/20] host_iommu_device: Define two new >capabilities HOST_IOMMU_DEVICE_CAP_[NESTING|FS1GP] > >Hi Zhenzhong, > > >On 2/19/25 9:22 AM, Zhenzhong Duan wrote: >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >> --- >> include/system/host_iommu_device.h | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/include/system/host_iommu_device.h >b/include/system/host_iommu_device.h >> index df782598f2..18f8b5e5cf 100644 >> --- a/include/system/host_iommu_device.h >> +++ b/include/system/host_iommu_device.h >> @@ -22,10 +22,16 @@ >> * >> * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this >represents >> * the @out_capabilities value returned from IOMMU_GET_HW_INFO >ioctl) >> + * >> + * @nesting: nesting page table support. >> + * >> + * @fs1gp: first stage(a.k.a, Stage-1) 1GB huge page support. >> */ >> typedef struct HostIOMMUDeviceCaps { >> uint32_t type; >> uint64_t hw_caps; >> + bool nesting; >> + bool fs1gp; >this looks quite vtd specific, isn't it? Shouldn't we hide this is a >vendor specific cap struct? Yes? I guess ARM hw could also provide nesting support at least? There are some reasons I perfer a flatten struct even if some Elements may be vendor specific. 1. If a vendor doesn't support an capability for other vendor, corresponding element should be zero by default. 2. An element vendor specific may become generic in future and we don't need to update the structure when that happens. 3. vIOMMU calls get_cap() to query if a capability is supported, so a vIOMMU never query a vendor specific capability it doesn't recognize. Even if that happens, zero is returned hinting no support. Thanks Zhenzhong
On 2/28/25 9:29 AM, Duan, Zhenzhong wrote: > Hi Eric, > >> -----Original Message----- >> From: Eric Auger <eric.auger@redhat.com> >> Subject: Re: [PATCH rfcv2 06/20] host_iommu_device: Define two new >> capabilities HOST_IOMMU_DEVICE_CAP_[NESTING|FS1GP] >> >> Hi Zhenzhong, >> >> >> On 2/19/25 9:22 AM, Zhenzhong Duan wrote: >>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> >>> --- >>> include/system/host_iommu_device.h | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/include/system/host_iommu_device.h >> b/include/system/host_iommu_device.h >>> index df782598f2..18f8b5e5cf 100644 >>> --- a/include/system/host_iommu_device.h >>> +++ b/include/system/host_iommu_device.h >>> @@ -22,10 +22,16 @@ >>> * >>> * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this >> represents >>> * the @out_capabilities value returned from IOMMU_GET_HW_INFO >> ioctl) >>> + * >>> + * @nesting: nesting page table support. >>> + * >>> + * @fs1gp: first stage(a.k.a, Stage-1) 1GB huge page support. >>> */ >>> typedef struct HostIOMMUDeviceCaps { >>> uint32_t type; >>> uint64_t hw_caps; >>> + bool nesting; >>> + bool fs1gp; >> this looks quite vtd specific, isn't it? Shouldn't we hide this is a >> vendor specific cap struct? > Yes? I guess ARM hw could also provide nesting support at least > There are some reasons I perfer a flatten struct even if some > Elements may be vendor specific. > 1. If a vendor doesn't support an capability for other vendor, > corresponding element should be zero by default. > 2. An element vendor specific may become generic in future > and we don't need to update the structure when that happens. > 3. vIOMMU calls get_cap() to query if a capability is supported, > so a vIOMMU never query a vendor specific capability it doesn't > recognize. Even if that happens, zero is returned hinting no support. I will let others comment but in general this is frown upon and unions are prefered at least. Eric > > Thanks > Zhenzhong >
On Thu, Mar 06, 2025 at 04:59:39PM +0100, Eric Auger wrote: > >>> +++ b/include/system/host_iommu_device.h > >>> @@ -22,10 +22,16 @@ > >>> * > >>> * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this > >> represents > >>> * the @out_capabilities value returned from IOMMU_GET_HW_INFO > >> ioctl) > >>> + * > >>> + * @nesting: nesting page table support. > >>> + * > >>> + * @fs1gp: first stage(a.k.a, Stage-1) 1GB huge page support. > >>> */ > >>> typedef struct HostIOMMUDeviceCaps { > >>> uint32_t type; > >>> uint64_t hw_caps; > >>> + bool nesting; > >>> + bool fs1gp; > >> this looks quite vtd specific, isn't it? Shouldn't we hide this is a > >> vendor specific cap struct? > > Yes? I guess ARM hw could also provide nesting support at least > > There are some reasons I perfer a flatten struct even if some > > Elements may be vendor specific. > > 1. If a vendor doesn't support an capability for other vendor, > > corresponding element should be zero by default. > > 2. An element vendor specific may become generic in future > > and we don't need to update the structure when that happens. > > 3. vIOMMU calls get_cap() to query if a capability is supported, > > so a vIOMMU never query a vendor specific capability it doesn't > > recognize. Even if that happens, zero is returned hinting no support. > I will let others comment but in general this is frown upon and unions > are prefered at least. Yea, it feels odd to me that we stuff vendor specific thing in the public structure. It's okay if we want to store in HostIOMMUDeviceCaps the vendor specific data pointer (opaque), just for convenience. I think we can have another PCIIOMMUOps op for vendor code to run iommufd_backend_get_device_info() that returns the hw_caps for the core code to read. Or perhaps the vendor code can just return a HWPT directly? If IOMMU_HW_CAP_DIRTY_TRACKING is set in the hw_caps, the vendor code can allocate a HWPT for that. And if vendor code detects the "nesting" cap in vendor struct, then return a nest_parent HWPT. And returning NULL can let core code allocate a default HWPT (or just attach the device to IOAS for auto domain/hwpt). I am also hoping that this can handle a shared S2 nest_parent HWPT case. Could the core container structure or so store the HWPT? Thanks Nicolin
Hi Eric, Nicolin, >-----Original Message----- >From: Nicolin Chen <nicolinc@nvidia.com> >Subject: Re: [PATCH rfcv2 06/20] host_iommu_device: Define two new >capabilities HOST_IOMMU_DEVICE_CAP_[NESTING|FS1GP] > >On Thu, Mar 06, 2025 at 04:59:39PM +0100, Eric Auger wrote: >> >>> +++ b/include/system/host_iommu_device.h >> >>> @@ -22,10 +22,16 @@ >> >>> * >> >>> * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this >> >> represents >> >>> * the @out_capabilities value returned from >IOMMU_GET_HW_INFO >> >> ioctl) >> >>> + * >> >>> + * @nesting: nesting page table support. >> >>> + * >> >>> + * @fs1gp: first stage(a.k.a, Stage-1) 1GB huge page support. >> >>> */ >> >>> typedef struct HostIOMMUDeviceCaps { >> >>> uint32_t type; >> >>> uint64_t hw_caps; >> >>> + bool nesting; >> >>> + bool fs1gp; >> >> this looks quite vtd specific, isn't it? Shouldn't we hide this is a >> >> vendor specific cap struct? >> > Yes? I guess ARM hw could also provide nesting support at least >> > There are some reasons I perfer a flatten struct even if some >> > Elements may be vendor specific. >> > 1. If a vendor doesn't support an capability for other vendor, >> > corresponding element should be zero by default. >> > 2. An element vendor specific may become generic in future >> > and we don't need to update the structure when that happens. >> > 3. vIOMMU calls get_cap() to query if a capability is supported, >> > so a vIOMMU never query a vendor specific capability it doesn't >> > recognize. Even if that happens, zero is returned hinting no support. >> I will let others comment but in general this is frown upon and unions >> are prefered at least. > >Yea, it feels odd to me that we stuff vendor specific thing in >the public structure. > >It's okay if we want to store in HostIOMMUDeviceCaps the vendor >specific data pointer (opaque), just for convenience. > >I think we can have another PCIIOMMUOps op for vendor code to >run iommufd_backend_get_device_info() that returns the hw_caps >for the core code to read. > >Or perhaps the vendor code can just return a HWPT directly? If >IOMMU_HW_CAP_DIRTY_TRACKING is set in the hw_caps, the vendor >code can allocate a HWPT for that. And if vendor code detects >the "nesting" cap in vendor struct, then return a nest_parent >HWPT. And returning NULL can let core code allocate a default >HWPT (or just attach the device to IOAS for auto domain/hwpt). > >I am also hoping that this can handle a shared S2 nest_parent >HWPT case. Could the core container structure or so store the >HWPT? Thanks for your suggestions. It is becoming clear for me. I'll update the code and send a new version after I finish a higher priority work in my company. Thanks Zhenzhong
diff --git a/include/system/host_iommu_device.h b/include/system/host_iommu_device.h index df782598f2..18f8b5e5cf 100644 --- a/include/system/host_iommu_device.h +++ b/include/system/host_iommu_device.h @@ -22,10 +22,16 @@ * * @hw_caps: host platform IOMMU capabilities (e.g. on IOMMUFD this represents * the @out_capabilities value returned from IOMMU_GET_HW_INFO ioctl) + * + * @nesting: nesting page table support. + * + * @fs1gp: first stage(a.k.a, Stage-1) 1GB huge page support. */ typedef struct HostIOMMUDeviceCaps { uint32_t type; uint64_t hw_caps; + bool nesting; + bool fs1gp; } HostIOMMUDeviceCaps; #define TYPE_HOST_IOMMU_DEVICE "host-iommu-device" @@ -122,6 +128,8 @@ struct HostIOMMUDeviceClass { */ #define HOST_IOMMU_DEVICE_CAP_IOMMU_TYPE 0 #define HOST_IOMMU_DEVICE_CAP_AW_BITS 1 +#define HOST_IOMMU_DEVICE_CAP_NESTING 2 +#define HOST_IOMMU_DEVICE_CAP_FS1GP 3 #define HOST_IOMMU_DEVICE_CAP_AW_BITS_MAX 64 #endif
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- include/system/host_iommu_device.h | 8 ++++++++ 1 file changed, 8 insertions(+)