| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6bc2abd2-4d1d-4b94-b2f3-6d3638f3e8ba@collabora.com>
Date: Wed, 12 Mar 2025 14:49:03 -0300
From: Ariel D'Alessandro <ariel.dalessandro@...labora.com>
To: Boris Brezillon <boris.brezillon@...labora.com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
robh@...nel.org, steven.price@....com, maarten.lankhorst@...ux.intel.com,
mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, simona@...ll.ch,
kernel@...labora.com, linux-mediatek@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, sjoerd@...labora.com
Subject: Re: [PATCH v1 4/6] drm/panfrost: Add support for AARCH64_4K page
table format
Boris,
On 3/11/25 5:05 AM, Boris Brezillon wrote:
> On Mon, 10 Mar 2025 16:59:19 -0300
> Ariel D'Alessandro <ariel.dalessandro@...labora.com> wrote:
>
>> Currently, Panfrost only supports MMU configuration in "LEGACY" (as
>> Bifrost calls it) mode, a (modified) version of LPAE "Large Physical
>> Address Extension", which in Linux we've called "mali_lpae".
>>
>> This commit adds support for conditionally enabling AARCH64_4K page
>> table format. To achieve that, a "GPU optional configurations" field was
>> added to `struct panfrost_features` with the related flag.
>>
>> Note that, in order to enable AARCH64_4K mode, the GPU variant must have
>> the HW_FEATURE_AARCH64_MMU feature flag present.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel.dalessandro@...labora.com>
>> ---
>> drivers/gpu/drm/panfrost/panfrost_device.h | 16 +++
>> drivers/gpu/drm/panfrost/panfrost_mmu.c | 132 +++++++++++++++++++--
>> drivers/gpu/drm/panfrost/panfrost_regs.h | 34 ++++++
>> 3 files changed, 169 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
>> index cffcb0ac7c111..0385702aa43c7 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
>> @@ -42,6 +42,14 @@ enum panfrost_gpu_pm {
>> GPU_PM_VREG_OFF,
>> };
>>
>> +/**
>> + * enum panfrost_gpu_config - GPU optional configurations
>> + * @GPU_CONFIG_AARCH64_4K: Use AARCH64_4K page table format
>> + */
>> +enum panfrost_gpu_config {
>> + GPU_CONFIG_AARCH64_4K,
>> +};
>> +
>> struct panfrost_features {
>> u16 id;
>> u16 revision;
>> @@ -95,6 +103,9 @@ struct panfrost_compatible {
>>
>> /* Allowed PM features */
>> u8 pm_features;
>> +
>> + /* GPU features */
>> + u8 gpu_configs;
>
> I would probably name this gpu_quirks, with the GPU_CONFIG_AARCH64_4K
> flag renamed GPU_QUIRK_FORCE_AARCH64_PAGE_TABLE.
Will follow on this thread after latest responses.
>> };
>>
>> struct panfrost_device {
>> @@ -162,6 +173,11 @@ struct panfrost_mmu {
>> int as;
>> atomic_t as_count;
>> struct list_head list;
>> + struct {
>> + u64 transtab;
>> + u64 memattr;
>> + u64 transcfg;
>> + } cfg;
>> };
>>
>> struct panfrost_engine_usage {
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> index 31df3a96f89bd..4a9b8de2ff987 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
>> @@ -26,6 +26,48 @@
[snip]
>> +static void mmu_cfg_init_aarch64_4k(struct panfrost_mmu *mmu)
>> +{
>> + struct io_pgtable_cfg *pgtbl_cfg = &mmu->pgtbl_cfg;
>> +
>> + mmu->cfg.transtab = pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
>> + AS_TRANSTAB_AARCH64_4K_ADDR_MASK;
>
> Silently masking the low 4bits is not the solution, it's just papering
> over a real issue. If pgtbl_cfg->arm_lpae_s1_cfg.ttbr is not aligned on
> 16 bytes (PAGE_SIZE even) we have a problem, so I would drm_WARN_ON()
> here, and return an error so we can fail the probe.
Good point. I'll add something like this in v2:
if (drm_WARN_ON(pfdev->ddev, pgtbl_cfg->arm_lpae_s1_cfg.ttbr &
~AS_TRANSTAB_AARCH64_4K_ADDR_MASK))
return -EINVAL;
>> +
>> + mmu->cfg.memattr = mair_to_memattr(pgtbl_cfg->arm_lpae_s1_cfg.mair,
>> + pgtbl_cfg->coherent_walk);
>> +
>> + mmu->cfg.transcfg = AS_TRANSCFG_PTW_MEMATTR_WB |
>> + AS_TRANSCFG_PTW_RA |
>> + AS_TRANSCFG_ADRMODE_AARCH64_4K |
>> + AS_TRANSCFG_INA_BITS(55 - pgtbl_cfg->ias);
>> + if (pgtbl_cfg->coherent_walk)
>> + mmu->cfg.transcfg |= AS_TRANSCFG_PTW_SH_OS;
>> +}
[snip]
>> @@ -616,6 +709,7 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>> {
>> u32 va_bits = GPU_MMU_FEATURES_VA_BITS(pfdev->features.mmu_features);
>> u32 pa_bits = GPU_MMU_FEATURES_PA_BITS(pfdev->features.mmu_features);
>> + enum io_pgtable_fmt fmt = ARM_MALI_LPAE;
>> struct panfrost_mmu *mmu;
>>
>> mmu = kzalloc(sizeof(*mmu), GFP_KERNEL);
>> @@ -641,16 +735,28 @@ struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev)
>> .iommu_dev = pfdev->dev,
>> };
>>
>> - mmu->pgtbl_ops = alloc_io_pgtable_ops(ARM_MALI_LPAE, &mmu->pgtbl_cfg,
>> - mmu);
>> - if (!mmu->pgtbl_ops) {
>> - kfree(mmu);
>> - return ERR_PTR(-EINVAL);
>> + if (pfdev->comp->gpu_configs & BIT(GPU_CONFIG_AARCH64_4K)) {
>> + if (!panfrost_has_hw_feature(pfdev, HW_FEATURE_AARCH64_MMU)) {
>> + dev_err_once(pfdev->dev,
>> + "AARCH64_4K page table not supported\n");
>> + goto err_free_mmu;
>> + }
>> + fmt = ARM_64_LPAE_S1;
>> }
>
> How about moving this check before allocating the mmu object, so you
> don't have to free it if it fails?
Yes, will fix in v2.
Thanks!
--
Ariel D'Alessandro
Software Engineer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718
Powered by blists - more mailing lists