| 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: <99c6850d-0b62-4d1f-a1d1-e5a0065ca27f@collabora.com>
Date: Wed, 12 Mar 2025 15:49:44 +0100
From: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
To: Ariel D'Alessandro <ariel.dalessandro@...labora.com>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Cc: boris.brezillon@...labora.com, 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
Il 12/03/25 15:20, Ariel D'Alessandro ha scritto:
> Angelo,
>
> On 3/11/25 6:10 AM, AngeloGioacchino Del Regno wrote:
>> Il 10/03/25 20:59, Ariel D'Alessandro ha scritto:
>>> 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.
>
> [snip]
>
>>> 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
>
> [snip]
>
>>> +static void panfrost_mmu_cfg_init(struct panfrost_mmu *mmu,
>>> + enum io_pgtable_fmt fmt)
>>> +{
>>> + struct panfrost_device *pfdev = mmu->pfdev;
>>> +
>>> + switch (fmt) {
>>> + case ARM_64_LPAE_S1:
>>> + mmu_cfg_init_aarch64_4k(mmu);
>>> + break;
>>> + case ARM_MALI_LPAE:
>>> + mmu_cfg_init_mali_lpae(mmu);
>>> + break;
>>> + default:
>>
>> If you add a
>> /* That should never happen */
>>
>> ...it's clear-er why this function doesn't fail (but still raises some eyebrows,
>> because if the `default` case is not reachable, why does it even have a print?).
>
> Makes sense. In that case, makes sense to just simplify it as:
>
> default:
> /* This should never happen */
> WARN_ON(1);
> break;
>
>>> + dev_WARN_ONCE(pfdev->dev, 1, "Unhandled page table format\n");
>>> + break;
>>> + }
>>> +}
>>> +
>>> static void
>>> _panfrost_mmu_as_control_write(struct panfrost_device *pfdev, u32 as_nr,
>>> - u64 transtab, u64 memattr)
>>> + u64 transtab, u64 memattr, u64 transcfg)
>>> {
>>> mmu_hw_do_operation_locked(pfdev, as_nr, 0, ~0ULL, AS_COMMAND_FLUSH_MEM);
>>> @@ -133,25 +223,28 @@ _panfrost_mmu_as_control_write(struct panfrost_device
>>> *pfdev, u32 as_nr,
>>> mmu_write(pfdev, AS_MEMATTR_LO(as_nr), lower_32_bits(memattr));
>>> mmu_write(pfdev, AS_MEMATTR_HI(as_nr), upper_32_bits(memattr));
>>> + mmu_write(pfdev, AS_TRANSCFG_LO(as_nr), lower_32_bits(transcfg));
>>> + mmu_write(pfdev, AS_TRANSCFG_HI(as_nr), upper_32_bits(transcfg));
>>> +
>>> write_cmd(pfdev, as_nr, AS_COMMAND_UPDATE);
>>> +
>>> + dev_dbg(pfdev->dev, "mmu_as_control: as=%d, transtab=0x%016llx,
>>> memattr=0x%016llx, transcfg=0x%016llx",
>>> + as_nr, transtab, memattr, transcfg);
>>> }
>>> static void panfrost_mmu_enable(struct panfrost_device *pfdev, struct
>>> panfrost_mmu *mmu)
>>> {
>>> - int as_nr = mmu->as;
>>> - struct io_pgtable_cfg *cfg = &mmu->pgtbl_cfg;
>>> - u64 transtab = cfg->arm_mali_lpae_cfg.transtab;
>>> - u64 memattr = cfg->arm_mali_lpae_cfg.memattr;
>>> -
>>> /* Need to revisit mem attrs.
>>> * NC is the default, Mali driver is inner WT.
>>> */
>>> - _panfrost_mmu_as_control_write(pfdev, as_nr, transtab, memattr);
>>> + _panfrost_mmu_as_control_write(pfdev, mmu->as, mmu->cfg.transtab,
>>> + mmu->cfg.memattr, mmu->cfg.transcfg);
>>> }
>>> static void panfrost_mmu_disable(struct panfrost_device *pfdev, u32 as_nr)
>>> {
>>> - _panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0);
>>> + _panfrost_mmu_as_control_write(pfdev, as_nr, 0, 0,
>>> + AS_TRANSCFG_ADRMODE_UNMAPPED);
>>> }
>>> u32 panfrost_mmu_as_get(struct panfrost_device *pfdev, struct panfrost_mmu *mmu)
>>> @@ -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;
>>
>> Double initialization! :-)
>
> Will fix in v2.
>
>>> 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)) {
>>
>> Why aren't you performing this check before kzalloc?
>>
>> If you do so, you will be able to avoid having a goto, because this check will
>> simply return an error (struct not allocated yet, nothing to kfree).
>> This also means that you won't have to modify anything about the error handling
>> of the alloc_io_pgtable_ops below....
>
> Yup, definitely agreed. Thanks!
>
>>> + 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;
>>> }
>>
>> } else {
>> fmt = ARM_MALI_LPAE;
>> }
>
> Ack. Will fix in v2.
>
>>
>>> + mmu->pgtbl_ops = alloc_io_pgtable_ops(fmt, &mmu->pgtbl_cfg, mmu);
>>> + if (!mmu->pgtbl_ops)
>>> + goto err_free_mmu;
>>> +
>>> + panfrost_mmu_cfg_init(mmu, fmt);
>>> +
>>> kref_init(&mmu->refcount);
>>> return mmu;
>>> +
>>> +err_free_mmu:
>>> + kfree(mmu);
>>> + return ERR_PTR(-EINVAL);
>>> }
>>> static const char *access_type_name(struct panfrost_device *pfdev,
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/ drm/
>>> panfrost/panfrost_regs.h
>>> index b5f279a19a084..2b8f1617b8369 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_regs.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
>>> @@ -301,6 +301,17 @@
>>> #define AS_TRANSTAB_HI(as) (MMU_AS(as) + 0x04) /* (RW) Translation
>>> Table Base Address for address space n, high word */
>>> #define AS_MEMATTR_LO(as) (MMU_AS(as) + 0x08) /* (RW) Memory attributes
>>> for address space n, low word. */
>>> #define AS_MEMATTR_HI(as) (MMU_AS(as) + 0x0C) /* (RW) Memory attributes
>>> for address space n, high word. */
>>> +#define AS_MEMATTR_AARCH64_INNER_ALLOC_IMPL (2 << 2)
>>> +#define AS_MEMATTR_AARCH64_INNER_ALLOC_EXPL(w, r) ((3 << 2) | \
>>> + ((w) ? BIT(0) : 0) | \
>>> + ((r) ? BIT(1) : 0))
>>> +#define AS_MEMATTR_AARCH64_SH_MIDGARD_INNER (0 << 4)
>>> +#define AS_MEMATTR_AARCH64_SH_CPU_INNER (1 << 4)
>>> +#define AS_MEMATTR_AARCH64_SH_CPU_INNER_SHADER_COH (2 << 4)
>>> +#define AS_MEMATTR_AARCH64_SHARED (0 << 6)
>>> +#define AS_MEMATTR_AARCH64_INNER_OUTER_NC (1 << 6)
>>> +#define AS_MEMATTR_AARCH64_INNER_OUTER_WB (2 << 6)
>>> +#define AS_MEMATTR_AARCH64_FAULT (3 << 6)
>>> #define AS_LOCKADDR_LO(as) (MMU_AS(as) + 0x10) /* (RW) Lock region
>>> address for address space n, low word */
>>> #define AS_LOCKADDR_HI(as) (MMU_AS(as) + 0x14) /* (RW) Lock region
>>> address for address space n, high word */
>>> #define AS_COMMAND(as) (MMU_AS(as) + 0x18) /* (WO) MMU command
>>> register for address space n */
>>> @@ -311,6 +322,24 @@
>>> /* Additional Bifrost AS registers */
>>> #define AS_TRANSCFG_LO(as) (MMU_AS(as) + 0x30) /* (RW) Translation
>>> table configuration for address space n, low word */
>>> #define AS_TRANSCFG_HI(as) (MMU_AS(as) + 0x34) /* (RW) Translation
>>> table configuration for address space n, high word */
>>> +#define AS_TRANSCFG_ADRMODE_LEGACY (0 << 0)
>>> +#define AS_TRANSCFG_ADRMODE_UNMAPPED (1 << 0)
>>> +#define AS_TRANSCFG_ADRMODE_IDENTITY (2 << 0)
>>> +#define AS_TRANSCFG_ADRMODE_AARCH64_4K (6 << 0)
>>> +#define AS_TRANSCFG_ADRMODE_AARCH64_64K (8 << 0)
>>
>> "Anything" shifted in any direction by 0 is equal to the same "anything" :-)
>>
>> Those are just 0,1,2,6,8
>
> Well, I agree... However, I'm copying this from panthor as-is. As these are similar
> drivers, I just kept all the macro definitions.
>
>>
>>> +#define AS_TRANSCFG_INA_BITS(x) ((x) << 6)
>>> +#define AS_TRANSCFG_OUTA_BITS(x) ((x) << 14)
>>> +#define AS_TRANSCFG_SL_CONCAT BIT(22)
>>> +#define AS_TRANSCFG_PTW_MEMATTR_NC (1 << 24)
>>
>> BIT(24)
>>
>>> +#define AS_TRANSCFG_PTW_MEMATTR_WB (2 << 24)
>>
>> BIT(25)
>>
>>> +#define AS_TRANSCFG_PTW_SH_NS (0 << 28)
>>
>> 0
>>
>>> +#define AS_TRANSCFG_PTW_SH_OS (2 << 28)
>>
>> BIT(29)
>>
>>> +#define AS_TRANSCFG_PTW_SH_IS (3 << 28)
>>
>> GENMASK(29, 28) or BIT(28) | BIT(29)
>>
>> (btw, rinse and repeat for the memattrs)
>
> I think the criteria used in panfrost/panthor for these definitions is:
> * if the register field is 1 bit, use BIT()
> * if the register field is >1 bit, use the value (as defined in the datasheet) an
> shift it.
> * and -be super explicit- do this even if the value is 0.
>
> I don't really have a strong opinion, but I'd attach to the subsystem/driver
> criteria used as much as possible :)
Okay, Fair enough!
Cheers,
Angelo
>
> Thanks!
>
Powered by blists - more mailing lists