RE: [PATCH 1/1] iommu/vt-d: Support asynchronous IOMMU nested capabilities

[Tian, Kevin]

> From: Lu Baolu
> Sent: Wednesday, May 12, 2021 7:31 PM
> 
> Hi Kevin,
> 
> On 5/12/21 4:30 PM, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >> Sent: Wednesday, May 12, 2021 3:04 PM
> >>
> >> Current VT-d implementation supports nested translation only if all
> >> underlying IOMMUs support the nested capability. This is unnecessary
> >> as the upper layer is allowed to create different containers and set
> >> them with different type of iommu backend. The IOMMU driver needs to
> >> guarantee that devices attached to a nested mode iommu_domain should
> >> support nested capabilility.
> >
> > so the consistency check is now applied only to the IOMMUs that are
> > spanned by a given iommu_domain?
> 
> Yes.
> 
> >
> >>
> >> Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> >> ---
> >>   drivers/iommu/intel/iommu.c | 21 +++++++++++++++++++--
> >>   1 file changed, 19 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> >> index f1742da42478..1cd4840e6f9f 100644
> >> --- a/drivers/iommu/intel/iommu.c
> >> +++ b/drivers/iommu/intel/iommu.c
> >> @@ -4755,6 +4755,13 @@ static int
> prepare_domain_attach_device(struct
> >> iommu_domain *domain,
> >>   	if (!iommu)
> >>   		return -ENODEV;
> >>
> >> +	if ((dmar_domain->flags & DOMAIN_FLAG_NESTING_MODE) &&
> >> +	    !ecap_nest(iommu->ecap)) {
> >> +		dev_err(dev, "%s: iommu not support nested translation\n",
> >> +			iommu->name);
> >> +		return -EINVAL;
> >> +	}
> >> +
> >>   	/* check if this iommu agaw is sufficient for max mapped address */
> >>   	addr_width = agaw_to_width(iommu->agaw);
> >>   	if (addr_width > cap_mgaw(iommu->cap))
> >> @@ -5451,11 +5458,21 @@ static int
> >>   intel_iommu_enable_nesting(struct iommu_domain *domain)
> >>   {
> >>   	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> >> +	struct dmar_drhd_unit *drhd;
> >> +	struct intel_iommu *iommu;
> >> +	bool has_nesting = false;
> >>   	unsigned long flags;
> >> -	int ret = -ENODEV;
> >> +	int ret = -EINVAL;
> >> +
> >> +	for_each_active_iommu(iommu, drhd)
> >> +		if (ecap_nest(iommu->ecap))
> >> +			has_nesting = true;
> >> +
> >> +	if (!has_nesting)
> >> +		return -ENODEV;
> >
> > Isn't above still doing global consistency check?
> 
> The logic is if nested mode is globally unsupported, return false.

why is this check required? anyway you already have the check when
prepare device attachment, and only at that point you have accurate
info for which iommu to be checked. This check looks meaningless
as even if some IOMMUs support nesting it doesn't mean the IOMMU
relevant to this domain support it.

> 
> >
> >>
> >>   	spin_lock_irqsave(&device_domain_lock, flags);
> >> -	if (nested_mode_support() && list_empty(&dmar_domain->devices))
> >> {
> >> +	if (list_empty(&dmar_domain->devices)) {
> >>   		dmar_domain->flags |= DOMAIN_FLAG_NESTING_MODE;
> >>   		dmar_domain->flags &= ~DOMAIN_FLAG_USE_FIRST_LEVEL;
> >>   		ret = 0;
> >> --
> >> 2.25.1
> >
> 
> Best regards,
> baolu
> _______________________________________________
> iommu mailing list
> iommu@xxxxxxxxxxxxxxxxxxxxxxxxxx
> https://lists.linuxfoundation.org/mailman/listinfo/iommu