Re: [PATCH V2 0/8] arm64/mm: Drop PXD_TABLE_BIT

[Ryan Roberts <ryan.roberts@arm.com>]

On 03/03/2025 05:02, Anshuman Khandual wrote:
> 
> 
> On 2/28/25 21:02, Ryan Roberts wrote:
>> On 21/02/2025 04:42, Anshuman Khandual wrote:
>>> Remove the PXX_TABLE_BIT definitions and instead rely on PXX_TYPE_MASK,
>>> PXX_TYPE_SECT and PXX_TYPE_TABLE. The latter versions are more abstract
>>> and also include the PTE_VALID bit.
>>>
>>> This abstraction is valuable for the impending D128 page table support,
>>> which doesn't have a single page table bit to determine table vs block.
>>> Instead it has the skip level (SKL) field, where it will consider 0 to
>>> mean table and any other value to mean a block entry. So PXX_TABLE_BIT
>>> therefore doesn't fit into the D128 model well, but the type fields do.
>>
>> All the patches look logically correct to me and I agree with the intention of
>> removing PXX_TABLE_BIT. But personally I'd prefer to see a single patch that
>> just does everything that's required to remove PXX_TABLE_BIT. And then a second
>> patch for the pud_bad() fix/improvement (currently patch 6) which is orthogonal
>> to the removal of PXX_TABLE_BIT.
>>
>> That would make it much easier to review IMHO, and would also allow for writing
>> a single commit log which provides the justification for the change. I find the
>> current set of 7 commit logs to not be hugely helpful.
> 
> Dropping PXX_TABLE_BIT from individual functional components which stand on their
> own progressively leads to its complete removal from the tree. Even though goal
> is PXX_TABLE_BIT mask's complete removal, each patch here could be justified on
> its own improving consistent reasoning around various section mapping creation
> and identification while keeping the functionality unchanged and also improving
> code readability as well.
> 
>>
>> But I wrote the original patches and wrote them as I'm suggesting, so I would
>> say that :)
> 
> I can understand :) Although it also follows and expands on the previous attempt
> in removing this mask that formed a patch series instead.
> 
> https://lore.kernel.org/all/20241005123824.1366397-1-anshuman.khandual@arm.com/
> 
> TBH this is not a big deal. I can merge all but last one into a single patch as
> you have suggested if that's a general consensus. Although I would prefer the
> current logically progressive series based approach but that's just me.

I guess leave as is for now and see what others say.

> 
>>
>> I'm guessing I shouldn't provide a Reviewed-By here, given I wrote the code
>> originally...
>>
>> Thanks,
>> Ryan
>>
>>
>>>
>>> This series applies on v6.14-rc3.
>>>
>>> Changes in V2:
>>>
>>> - Changed pmd_mkhuge() and pud_mkhuge() implementation
>>> - Changed pud_bad() implementation with an additional patch
>>>
>>> Changes in V1:
>>>
>>> https://lore.kernel.org/all/20241005123824.1366397-1-anshuman.khandual@arm.com/
>>>
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: Oliver Upton <oliver.upton@linux.dev>
>>> Cc: James Morse <james.morse@arm.com>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Will Deacon <will@kernel.org>
>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>> Cc: Ryan Roberts <ryan.roberts@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: kvmarm@lists.linux.dev
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-kernel@vger.kernel.org
>>>
>>> Anshuman Khandual (6):
>>>   KVM: arm64: ptdump: Test PMD_TYPE_MASK for block mapping
>>>   arm64/ptdump: Test PMD_TYPE_MASK for block mapping
>>>   arm64/mm: Clear PXX_TYPE_MASK in mk_[pmd|pud]_sect_prot()
>>>   arm64/mm: Clear PXX_TYPE_MASK and set PXD_TYPE_SECT in [pmd|pud]_mkhuge()
>>>   arm64/mm: Check PXD_TYPE_TABLE in [p4d|pgd]_bad()
>>>   arm64/mm: Drop PXD_TABLE_BIT
>>>
>>> Ryan Roberts (2):
>>>   arm64/mm: Check PUD_TYPE_TABLE in pud_bad()
>>>   arm64/mm: Check pmd_table() in pmd_trans_huge()
>>>
>>>  arch/arm64/include/asm/pgtable-hwdef.h |  5 --
>>>  arch/arm64/include/asm/pgtable.h       | 65 ++++++++++++++++++--------
>>>  arch/arm64/kvm/ptdump.c                |  4 +-
>>>  arch/arm64/mm/ptdump.c                 |  4 +-
>>>  4 files changed, 50 insertions(+), 28 deletions(-)
>>>
>>