Received-SPF: pass (google.com: domain of linux-kernel+bounces-38470-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <0aa0664a-21fc-48d6-bc88-c752f5a10ebe@windriver.com>
Date: Thu, 25 Jan 2024 12:56:08 +0200
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] iommu: iommu_group_alloc_default_domain: Fix
 ops->default_domain codepath
Content-Language: en-US
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: linux-kernel@vger.kernel.org, iommu@lists.linux.dev, robin.murphy@arm.com,
        will@kernel.org, joro@8bytes.org
References: <20240123165829.630276-1-ovidiu.panait@windriver.com>
 <20240123174905.GS50608@ziepe.ca>
From: Ovidiu Panait <ovidiu.panait@windriver.com>
In-Reply-To: <20240123174905.GS50608@ziepe.ca>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?ZFVkbU9vYXltUTBPQThNWDdvS2QrL2Zuei9kaG81c01YTUVNYU9ZWmM3UmVR?=
 =?utf-8?B?OE5vZXIwRU50ZUs2RXBNb0xVaW1HRmh6b2JqOGQwU3VRV2lvWC9uc3p1Ui9E?=
 =?utf-8?B?K2V6MWh2LzAwRG05SlhQK0MwKytqYWoxZld4cEFhQUY1YW9UaDRucnVJVm14?=
 =?utf-8?B?TDFpbHVZMWlVRDU0VWc1STk3QzZrbU8rMmR5SUJhUE5LNUdDbW1nRlRwaEd4?=
 =?utf-8?B?VDNqQTNOR1drYXFNd1VOYUU1M2c1WTlZTjA4a1NyM3Q0SThwSGVDQS9EQUk0?=
 =?utf-8?B?Y3VPL2dTWmRDYkRTZ0VLVGJmNjZEMEhTd29teTNtcEwwT2lBdEJ0Rm13SGc0?=
 =?utf-8?B?d0VWUktWSXU3WE04K2FISGlDVTd6citwQ2hJVUhXZ1puQlRXMDh3NnZ4Qjkv?=
 =?utf-8?B?enBLZnNiZjFLMHdGNDFwMVk0Qjh6dkV4S2RQdEI4VHA0eWlPWlEzdk5mWkFP?=
 =?utf-8?B?OHhINTBPU3AxU2hwTmxGa1ZDL0xuZjJtQnJ0V3lCZlUyckhTMHJzaU5Pa1Q4?=
 =?utf-8?B?aUZaeklJQUJzay8rQ1p4YjlPS0dYZ1hUdXNBa1R0NHBvKzlHL3c5RnZnblFV?=
 =?utf-8?B?UkNnZHZJWE9PeEZ6RWNHQk9jaGVXRytaNElBQ0ZDQmhUZHNFNHFkdkp2OE9N?=
 =?utf-8?B?MTFNZW01YVBFazlSazJoVmVMblI1ajRJckVrTitOeGtyOTJlODNrQ3RMKzJq?=
 =?utf-8?B?NnVXR3hVaktUalh1ZVJLaTdiMzZhaGt3MWUyRkQ0MWx4anpsYlRNSDlBMVl5?=
 =?utf-8?B?c2VEa1FIYWNpY0VmMUszY3hQWmhEOWdKYUxLelNLU0ZjOHdwSFBKRDlCTUNo?=
 =?utf-8?B?VndpVmhuV3IwaVpmTkdQTWJyTFNMdFg0WW50OENVL3MvdlJoQjAwN0UvRmRl?=
 =?utf-8?B?bVhZVmIwQ1NlWXFlc0UwWjJyeUJ5RnFaYjVrOEdyVHFWMVF0OVNSYVQ0Tm50?=
 =?utf-8?B?SDBqYzJLczFTQjhQSEpuRXlkdnRGK1IxMkpHUkFpa0dhL2cwb3JxdE1XN0RF?=
 =?utf-8?B?WUcvZmM4VlRLVGNHSXd0R1MxZzRzSG0zaGRNQzgwL3RnTjRlcXRRNkVQLzls?=
 =?utf-8?B?UEU2N3ZHOWxra1Jjc0w3eWRRQ1lsb3o3NlZXbVdYd1g2NFB4NVlOSlZ0WWJJ?=
 =?utf-8?B?cjFkT25oM04wL1NNbGVkNXFuR0RYcTBxNnI1NXBjZ3RYa21MSEp1VUoyVVFx?=
 =?utf-8?B?UUppMGRRTmp5SmlGS1E5UnhuR3U0U3hFYlR0cFVNdkJ0Lzl0UzIvaGw0RjFB?=
 =?utf-8?B?eUNRRExzRFlnZDR5OElUV3hlN2hVbmMxN01mbmlVQXBDQjVMTXQ5R3BKTVZU?=
 =?utf-8?B?NGw4bnJUak1kbjNhQk8xMzFDaTRjTzM2cGFnZnp0T2FBVFBVMmt5d2VPL2c0?=
 =?utf-8?B?S3VnQlBHd09mLzdIb2I5Um1kaWc0UEE5TElDeUw4anhmMmtsMzY0N2VRTlRP?=
 =?utf-8?B?NU9GYnFZbEVLaTZYMGR6Qi9oL050RXBkNnByclBHYWdiNkdTYW85RzBpZ0ZX?=
 =?utf-8?B?RFpKYW81WUN2dCsrb2FBbEw1Z3VETHhrekhXNDdYeFN5cFd2eGl0T0VkMWI5?=
 =?utf-8?B?Y0owQmJTVTF3c001WXAzYnAxSHdlZmZxWm9YazN6ZEdLei94anR0QVFXcWVX?=
 =?utf-8?B?YjdkNjZuR1FOWXRXU3hub2FoUXlEQkZIeklIU2FLa3VEbGJqRHpoMnloNG9o?=
 =?utf-8?B?WlN4YU1tb3dRUS9XNHhBUVBWbjQ0YTV3cXVZTWJqWnZJR2YwcUUzbTlLOHhU?=
 =?utf-8?B?aGc4Y2tQcHBlVVk2T20vbjBFM2NJcmhmZk1zbHd1eitKRlllZEt4V1JmTFlB?=
 =?utf-8?B?U1dtcktMWUEzZldMNEREQnA0NXJhMUVlOU02bngrU3U0cTFaRyt5dDNXR2Zy?=
 =?utf-8?B?cCtKYXhyS2RtanBHeTdHVEdNWENrTlVVN2Y3LzllZVN6dDU4NEw1RVp5UTk4?=
 =?utf-8?B?Yk1UdFE0d0NGNE84L2hJNDJ3dmlxRUEzc09IMk5VOW5Mb2didXo1UG1uMVhP?=
 =?utf-8?B?Uk9jdmVnaE1PekwybUZUNit0SXRTeXc0c1EzTWs4UXBLVWRSdDh5R25wNzA4?=
 =?utf-8?B?Z3N1cXRuai9mQ0ZyZGdSMnM2bzZkQmlzZ2Q5QzFGQXNpdkkzbWpBVSsrZXE2?=
 =?utf-8?B?SVJEblozU2o0QS9LM05iMmt3RzA5K0tNWm9rdFlSdkpsYkNwdDdkWHI4VFpk?=
 =?utf-8?B?eXc9PQ==?=
X-OriginatorOrg: windriver.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c9c5b240-c84d-46e4-a068-08dc1d950819
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5327.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Jan 2024 11:01:50.6001
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ddb2873-a1ad-4a18-ae4e-4644631433be
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LqvUSrsZAUjxQUPnXaVjgCFGBqDI7OM0KrBVA46QstGKODLhp8eiadhKZfQIbBpOfe5+TyFzaWORXBf/24RFOrIDglIEvpZj/joeZroZ860=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB6423
X-Proofpoint-GUID: rQtY41uuo8ZqzruOwId8QdDaD90Qo5hU
X-Proofpoint-ORIG-GUID: rQtY41uuo8ZqzruOwId8QdDaD90Qo5hU
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-01-25_06,2024-01-25_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 suspectscore=0
 bulkscore=0 adultscore=0 mlxscore=0 malwarescore=0 phishscore=0
 impostorscore=0 mlxlogscore=999 priorityscore=1501 lowpriorityscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.19.0-2401190000 definitions=main-2401250075

Hi Jason,

On 23.01.2024 19:49, Jason Gunthorpe wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Tue, Jan 23, 2024 at 06:58:29PM +0200, ovidiu.panait@windriver.com wrote:
>> From: Ovidiu Panait <ovidiu.panait@windriver.com>
>>
>> Since commit [1], FSL_PAMU initialization on t4240rdb board fails
>> with: "fsl-pamu-domain: pamu_domain_init: Can't register iommu device".
>>
>> Commit [1] changed the behavior for drivers that set ops->default_domain,
>> as the function iommu_group_alloc_default_domain() is now called with
>> "req_type == IOMMU_DOMAIN_IDENTITY", rather than "req_type == 0". This
>> causes the following EINVAL condition to be hit during FSL_PAMU
>> initialization:
>>    ...
>>         if (ops->default_domain) {
>>                 if (req_type)
>>                         return ERR_PTR(-EINVAL);
>>                 return ops->default_domain;
>>         }
>>    ...
>>
>> To avoid this issue when ops->default_domain != NULL, remove the check for
>> non-zero req_type and always return the proper default domain, as set in
>> the driver.
>>
>>    [1] 0f6a90436a57 ("iommu: Do not use IOMMU_DOMAIN_DMA [...]")
>>
>> Fixes: 0f6a90436a57 ("iommu: Do not use IOMMU_DOMAIN_DMA if CONFIG_IOMMU_DMA is not enabled")
>> Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
>> ---
>>   drivers/iommu/iommu.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
> Oh, that is a neat combination of things
>
> Removing the test will cause problems for the sys flow - I think the
> right thing is to get the correct req_type earlier. paml wants to use
> a specific domain type so it should not be switching to identity at
> all.
>
> Does the below work for you?
Yes, this works, thanks a lot!
> Also, what is your interest here? I have been wanting to delete this
> driver, would your system still be OK for your usage if it was
> removed?
CONFIG_FSL_PAMU is enabled by default in corenet64_smp_defconfig, and it 
looks
like many platforms are based on this config. I actually noticed this issue
while boot testing the mainline kernel on the t4240rdb board with the 
default
corenet64_smp_defconfig.

In this particular case, the board still boots fine with CONFIG_FSL_PAMU 
disabled,
but I'm not sure how/if removing the PAMU driver will impact other 
platforms.

Ovidiu
> Thanks,
> Jason
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 68e648b5576706..7fa3ccdc3286e6 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1799,7 +1799,7 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
>           * domain. Do not use in new drivers.
>           */
>          if (ops->default_domain) {
> -               if (req_type)
> +               if (req_type != ops->default_domain->type)
>                          return ERR_PTR(-EINVAL);
>                  return ops->default_domain;
>          }
> @@ -1871,10 +1871,18 @@ static int iommu_get_def_domain_type(struct iommu_group *group,
>          const struct iommu_ops *ops = dev_iommu_ops(dev);
>          int type;
>
> -       if (!ops->def_domain_type)
> -               return cur_type;
> -
> -       type = ops->def_domain_type(dev);
> +       if (ops->default_domain) {
> +               /*
> +                * Drivers that declare a global static default_domain will
> +                * always choose that.
> +                */
> +               type = ops->default_domain->type;
> +       } else {
> +               if (ops->def_domain_type)
> +                       type = ops->def_domain_type(dev);
> +               else
> +                       type = cur_type;
> +       }
>          if (!type || cur_type == type)
>                  return cur_type;
>          if (!cur_type)