Re: [PATCH v8 07/15] iommu/amd: Setup GCR3 table in advance if domain is SVA capable

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Klara,


On 5/2/2024 6:46 PM, Klara Modin wrote:
> Hi,
> 
> On 2024-04-18 12:33, Vasant Hegde wrote:
>> SVA can be supported if domain is in passthrough mode or paging domain
>> with v2 page table. Current code sets up GCR3 table for domain with v2
>> page table only. Setup GCR3 table for all SVA capable domains.
>>
>>    - Move GCR3 init/destroy to separate function.
>>
>>    - Change default GCR3 table to use MAX supported PASIDs. Ideally it
>>      should use 1 level PASID table as its using PASID zero only. But we
>>      don't have support to extend PASID table yet. We will fix this later.
>>
>>    - When domain is configured with passthrough mode, allocate default GCR3
>>      table only if device is SVA capable.
>>
>> Note that in attach_device() path it will not know whether device will use
>> SVA or not. If device is attached to passthrough domain and if it doesn't
>> use SVA then GCR3 table will never be used. We will endup wasting memory
>> allocated for GCR3 table. This is done to avoid DTE update when
>> attaching PASID to device.
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> ---
>>   drivers/iommu/amd/iommu.c | 86 ++++++++++++++++++++++++++++++++-------
>>   1 file changed, 71 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 7daf6d75d964..288cf7485306 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -89,6 +89,21 @@ static inline bool pdom_is_v2_pgtbl_mode(struct
>> protection_domain *pdom)
>>       return (pdom && (pdom->pd_mode == PD_MODE_V2));
>>   }
>>   +static inline bool pdom_is_in_pt_mode(struct protection_domain *pdom)
>> +{
>> +    return (pdom->domain.type == IOMMU_DOMAIN_IDENTITY);
>> +}
>> +
>> +/*
>> + * We cannot support PASID w/ existing v1 page table in the same domain
>> + * since it will be nested. However, existing domain w/ v2 page table
>> + * or passthrough mode can be used for PASID.
>> + */
>> +static inline bool pdom_is_sva_capable(struct protection_domain *pdom)
>> +{
>> +    return pdom_is_v2_pgtbl_mode(pdom) || pdom_is_in_pt_mode(pdom);
>> +}
>> +
>>   static inline int get_acpihid_device_id(struct device *dev,
>>                       struct acpihid_map_entry **entry)
>>   {
>> @@ -1964,6 +1979,58 @@ void amd_iommu_dev_update_dte(struct iommu_dev_data
>> *dev_data, bool set)
>>       iommu_completion_wait(iommu);
>>   }
>>   +/*
>> + * If domain is SVA capable then initialize GCR3 table. Also if domain is
>> + * in v2 page table mode then update GCR3[0].
>> + */
>> +static int init_gcr3_table(struct iommu_dev_data *dev_data,
>> +               struct protection_domain *pdom)
>> +{
>> +    struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
>> +    int max_pasids = dev_data->max_pasids;
>> +    int ret = 0;
>> +
>> +     /*
>> +      * If domain is in pt mode then setup GCR3 table only if device
>> +      * is PASID capable
>> +      */
>> +    if (pdom_is_in_pt_mode(pdom) && !pdev_pasid_supported(dev_data))
>> +        return ret;
>> +
>> +    /*
>> +     * By default, setup GCR3 table to support MAX PASIDs
>> +     * supported by the device/IOMMU.
>> +     */
>> +    ret = setup_gcr3_table(&dev_data->gcr3_info, iommu,
>> +                   max_pasids > 0 ?  max_pasids : 1);
>> +    if (ret)
>> +        return ret;
>> +
>> +    /* Setup GCR3[0] only if domain is setup with v2 page table mode */
>> +    if (!pdom_is_v2_pgtbl_mode(pdom))
>> +        return ret;
>> +
>> +    ret = update_gcr3(dev_data, 0, iommu_virt_to_phys(pdom->iop.pgd), true);
>> +    if (ret)
>> +        free_gcr3_table(&dev_data->gcr3_info);
>> +
>> +    return ret;
>> +}
>> +
>> +static void destroy_gcr3_table(struct iommu_dev_data *dev_data,
>> +                   struct protection_domain *pdom)
>> +{
>> +    struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>> +
>> +    if (pdom_is_v2_pgtbl_mode(pdom))
>> +        update_gcr3(dev_data, 0, 0, false);
>> +
>> +    if (gcr3_info->gcr3_tbl == NULL)
>> +        return;
>> +
>> +    free_gcr3_table(gcr3_info);
>> +}
>> +
>>   static int do_attach(struct iommu_dev_data *dev_data,
>>                struct protection_domain *domain)
>>   {
>> @@ -1982,19 +2049,10 @@ static int do_attach(struct iommu_dev_data *dev_data,
>>       domain->dev_iommu[iommu->index] += 1;
>>       domain->dev_cnt                 += 1;
>>   -    /* Init GCR3 table and update device table */
>> -    if (domain->pd_mode == PD_MODE_V2) {
>> -        /* By default, setup GCR3 table to support single PASID */
>> -        ret = setup_gcr3_table(&dev_data->gcr3_info, iommu, 1);
>> +    if (pdom_is_sva_capable(domain)) {
>> +        ret = init_gcr3_table(dev_data, domain);
>>           if (ret)
>>               return ret;
>> -
>> -        ret = update_gcr3(dev_data, 0,
>> -                  iommu_virt_to_phys(domain->iop.pgd), true);
>> -        if (ret) {
>> -            free_gcr3_table(&dev_data->gcr3_info);
>> -            return ret;
>> -        }
>>       }
>>         /* Update device table */
>> @@ -2009,10 +2067,8 @@ static void do_detach(struct iommu_dev_data *dev_data)
>>       struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
>>         /* Clear GCR3 table */
>> -    if (domain->pd_mode == PD_MODE_V2) {
>> -        update_gcr3(dev_data, 0, 0, false);
>> -        free_gcr3_table(&dev_data->gcr3_info);
>> -    }
>> +    if (pdom_is_sva_capable(domain))
>> +        destroy_gcr3_table(dev_data, domain);
>>         /* Update data structures */
>>       dev_data->domain = NULL;
> 
> This change seems to interact poorly with the amdgpu driver. It fails to
> initialize properly (monitors go to sleep).
> 
> The full log is attached, this snippet happens immediately before the first
> warning:
> amdgpu 0000:2d:00.0: amdgpu: STB initialized to 2048 entries
> [drm] Loading DMUB firmware via PSP: version=0x02020020
> [drm] use_doorbell being set to: [true]
> [drm] use_doorbell being set to: [true]
> [drm] Found VCN firmware Version ENC: 1.30 DEC: 3 VEP: 0 Revision: 4
> amdgpu 0000:2d:00.0: amdgpu: Will use PSP to load VCN firmware
> iommu ivhd0: AMD-Vi: Event logged [ILLEGAL_DEV_TABLE_ENTRY device=0000:2d:00.0
> pasid=0x00000 address=0x11c300000 flags=0x0080]
> AMD-Vi: DTE[0]: 7d80000000000003
> AMD-Vi: DTE[1]: 0000100011700014
> AMD-Vi: DTE[2]: 0000000000000000
> AMD-Vi: DTE[3]: 0000000000000000
> r8169 0000:05:00.0 enp5s0: Link is Up - 1Gbps/Full - flow control rx/tx
> amdgpu 0000:2d:00.0: amdgpu: PSP load kdb failed!
> [drm:psp_v11_0_ring_destroy [amdgpu]] *ERROR* Fail to stop psp ring
> [drm:amdgpu_fill_buffer [amdgpu]] *ERROR* Trying to clear memory with ring
> turned off.
> 
> Please let me know if this is the wrong place to report this or there's anything
> else you need.

Thanks for the report.

Looking into the attached dmesg :

2024-04-29T10:35:05+02:00 soda.int.kasm.eu kernel: AMD-Vi: Using global IVHD
EFR:0x0, EFR2:0x0

--> This means BIOS didn't setup the EFR flags in ACPI IVRS table (its BIOS issue).

Commit 8e0179733172 (iommu/amd: Enable Guest Translation before registering
devices) changed GT feature enablement in control register.

Now it tries to enable CONTROL[GT] flag before reading IOMMU feature register
(late_iommu_features_init()).

In attach device path, it assumed GT is supported (based on IOMMU EFR register
value) and enabled DTE[GV]. Hence we hit ILLEGAL_DEV_TABLE_ENTRY error.


Can you please try below fix?


-Vasant

---


diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index fd3e76e43699..b292181995b7 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -2046,6 +2046,8 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
 			amd_iommu_max_glx_val = glxval;
 		else
 			amd_iommu_max_glx_val = min(amd_iommu_max_glx_val, glxval);
+
+		iommu_enable_gt(iommu);
 	}

 	if (check_feature(FEATURE_PPR) && amd_iommu_alloc_ppr_log(iommu))
@@ -2732,7 +2734,6 @@ static void early_enable_iommu(struct amd_iommu *iommu)
 	iommu_enable_command_buffer(iommu);
 	iommu_enable_event_buffer(iommu);
 	iommu_set_exclusion_range(iommu);
-	iommu_enable_gt(iommu);
 	iommu_enable_ga(iommu);
 	iommu_enable_xt(iommu);
 	iommu_enable_irtcachedis(iommu);
@@ -2789,7 +2790,6 @@ static void early_enable_iommus(void)
 			iommu_disable_irtcachedis(iommu);
 			iommu_enable_command_buffer(iommu);
 			iommu_enable_event_buffer(iommu);
-			iommu_enable_gt(iommu);
 			iommu_enable_ga(iommu);
 			iommu_enable_xt(iommu);
 			iommu_enable_irtcachedis(iommu);