On Mon, Jun 16, 2025 at 02:12:04PM +0800, Baolu Lu wrote:
On 6/14/25 15:14, Nicolin Chen wrote:
- if (!viommu->ops || !viommu->ops->get_hw_queue_size ||
!viommu->ops->hw_queue_init_phys) {rc = -EOPNOTSUPP;goto out_put_viommu;- }
Hmm, here it does abort when !viommu->ops->hw_queue_init_phys ..
- /*
* FIXME once ops->hw_queue_init is introduced, this should check "if* ops->hw_queue_init_phys". And "access" should be initialized to NULL.*/I just don't follow here. Up until now, only viommu->ops-> hw_queue_init_phys has been added, which means the current code only supports hardware queues that access guest memory using physical addresses. The access object is not needed for the other type of hardware queue that uses guest IOVA.
So, why not just abort here if ops->hw_queue_init_phys is not supported by the IOMMU driver?
.. so, it already does.
Leave other logics to the patches that introduce ops->hw_queue_init? I guess that would make this patch more readible.
The patch is doing in the way to support the hw_queue_init_phys case only. It is just adding some extra FIXMEs as the guideline for the future patch adding hw_queue_init op.
I personally don't feel these are confusing. Maybe you can help point out what specific wording feels odd here? Maybe "FIXME"s should be "TODO"s?
I could also drop all of these guideline if they are considered very unnecessary.
Thanks Nicolin