Re: [PATCH v2 0/8] iommu: Domain allocation enhancements

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Oct 10, 2024 at 02:48:09PM +0800, Baolu Lu wrote:
> On 2024/10/10 14:40, Baolu Lu wrote:
> > On 2024/10/9 20:15, Jason Gunthorpe wrote:
> > > On Wed, Oct 09, 2024 at 03:23:16PM +0530, Vasant Hegde wrote:
> > > 
> > > > > This change might cause a functional regression when it comes to nested
> > > > > translation. In nested translation mode, the user page table (e.g.,
> > > > > created and managed by a guest VM for guest kernel DMA) must be in the
> > > > > first-stage page table format. Then, it can be nested on a second-stage
> > > > > page table managed by the host kernel.
> > > > > 
> > > > > Currently, the kernel automatically selects the page table formats. For
> > > > > example, the Intel IOMMU driver always uses the first-stage page table
> > > > > for guest kernel DMA. After this change, this assumption no
> > > > > longer holds
> > > > > true. This means the kernel might use a second-stage page table for
> > > > > guest kernel DMA, breaking nested translation.
> > > > Hmmm. I assumed after discussion in v1 series you are fine.
> > > > Looks like I misread it?
> > > It is a bug in the implementation in the intel driver.
> > > 
> > > intel_iommu_domain_alloc_user() should always allocate a page table
> > > that works, if you are in a guest context it must allocate a first
> > > level/guest page table otherwise nested VFIO will be broken.
> > > 
> > > For this reason Intel driver should always allocate the guest
> > > compatible page table unless IOMMU_HWPT_ALLOC_NEST_PARENT is
> > > specified.
> > > 
> > > Something like this:
> > 
> > This will break the existing dirty page tracking functionality. Intel
> > IOMMU only supports enabling or disabling dirty page tracking at the
> > second-stage page table.
> 
> So, perhaps something like below?
> 
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 1497f3112b12..e11dde259afa 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -544,6 +544,8 @@ enum {
>                                 ecap_slads((iommu)->ecap))
>  #define nested_supported(iommu)        (sm_supported(iommu) &&         \
>                                  ecap_nest((iommu)->ecap))
> +#define flts_supported(iommu)  (sm_supported(iommu) &&                 \
> +                                ecap_flts((iommu)->ecap))
> 
>  struct pasid_entry;
>  struct pasid_state_entry;
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 9f6b0780f2ef..d1a7378489a4 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -3530,6 +3530,7 @@ intel_iommu_domain_alloc_user(struct device *dev, u32
> flags,
>         struct intel_iommu *iommu = info->iommu;
>         struct dmar_domain *dmar_domain;
>         struct iommu_domain *domain;
> +       bool first_stage;
> 
>         /* Must be NESTING domain */
>         if (parent) {
> @@ -3546,8 +3547,14 @@ intel_iommu_domain_alloc_user(struct device *dev, u32
> flags,
>         if (user_data || (dirty_tracking && !ssads_supported(iommu)))
>                 return ERR_PTR(-EOPNOTSUPP);
> 
> -       /* Do not use first stage for user domain translation. */
> -       dmar_domain = paging_domain_alloc(dev, false);
> +       /*
> +        * Always allocate the guest compatible page table unless
> +        * IOMMU_HWPT_ALLOC_NEST_PARENT or IOMMU_HWPT_ALLOC_DIRTY_TRACKING
> +        * is specified.
> +        */
> +       first_stage = (nested_parent || dirty_tracking) ?
> +                       false : flts_supported(iommu);

That makes sense, but these flags still need to be rejected if the
second level is not supported in the HW.

+	if ((flags & (IOMMU_HWPT_ALLOC_NEST_PARENT | IOMMU_HWPT_ALLOC_DIRTY_TRACKING)) && !intel_cap_slts_sanity())
+		return ERR_PTR(-EOPNOTSUPP);

I also think my version was cleaner as we should be using
first_level_by_default() consistently to make that decision.

Also don't care for ternary expressions :)

Jason