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

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2024/10/10 19:38, Jason Gunthorpe wrote:
> 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 🙂

Okay, seems that we are on the same page now. :-)

I have a series to add domain_alloc_paging support in the Intel iommu
driver. I will convert the change in a formal patch and put it in that
series for further review.

Thanks,
baolu