Hi Eric, Nicolin,
>-----Original Message-----
>From: Nicolin Chen <nicol...@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
