On 05/09/2024 04:27, Barry Song wrote:
> On Thu, Sep 5, 2024 at 3:50 AM Ryan Roberts <ryan.roberts@xxxxxxx> wrote:
>>
>> On 04/09/2024 16:13, Will Deacon wrote:
>>> (Adding Ryan, since you're asking him a question!)
>>
>> Thanks, Will!
>>
>> Afraid I don't do a good job of monitoring the list; I'm guessing there are
>> automated ways to filter for mentions of my name so I catch this sort of thing
>> in future?
>
> It's not your fault. I just realized that, for some unknown reason, I forgot to
> CC you.
No worries. I was just asking if there is a general approach that people take to
monitor mail that they are not explicitly cc'ed on, but I guess that's a bit off
topic.
>
>>
>>>
>>> On Sat, Aug 31, 2024 at 10:06:40PM +1200, Barry Song wrote:
>>>> On Sat, Aug 31, 2024 at 9:54 PM David Hildenbrand <david@xxxxxxxxxx> wrote:
>>>>>
>>>>> On 31.08.24 10:35, Barry Song wrote:
>>>>>> From: Barry Song <v-songbaohua@xxxxxxxx>
>>>>>>
>>>>>> Hi Ryan, David,
>>>>>> it seems contpte_ptep_set_access_flags() has never advanced
>>>>>> pte pfn, and it is setting all entries' pfn to the first
>>>>>> subpage. But I feel quite strange we never have a bug reported.
>>>>>> Am I missing something?
>>>>>>
>>>>>> Fixes: 4602e5757bcc ("arm64/mm: wire up PTE_CONT for user mappings")
>>>>>> Cc: Ard Biesheuvel <ardb@xxxxxxxxxx>
>>>>>> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
>>>>>> Cc: Mark Rutland <mark.rutland@xxxxxxx>
>>>>>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>>>>>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>>>>>> Cc: Will Deacon <will@xxxxxxxxxx>
>>>>>> Signed-off-by: Barry Song <v-songbaohua@xxxxxxxx>
>>>>>> ---
>>>>>> arch/arm64/mm/contpte.c | 4 +++-
>>>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
>>>>>> index a3edced29ac1..10dcd2641184 100644
>>>>>> --- a/arch/arm64/mm/contpte.c
>>>>>> +++ b/arch/arm64/mm/contpte.c
>>>>>> @@ -421,8 +421,10 @@ int contpte_ptep_set_access_flags(struct vm_area_struct *vma,
>>>>>> ptep = contpte_align_down(ptep);
>>>>>> start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
>>>>>>
>>>>>> - for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
>>>>>> + for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE) {
>>>>>> __ptep_set_access_flags(vma, addr, ptep, entry, 0);
>>>>>> + entry = pte_advance_pfn(entry, 1);
>>>>>> + }
>>>>>>
>>>>>> if (dirty)
>>>>>> __flush_tlb_range(vma, start_addr, addr,
>>>>>
>>>>> Taking a closer look at __ptep_set_access_flags(), there is:
>>>>>
>>>>> /* only preserve the access flags and write permission *
>>>>> pte_val(entry) &= PTE_RDONLY | PTE_AF | PTE_WRITE | PTE_DIRTY;
>>>>>
>>>>> So it looks like it doesn't need the PFN?
>>
>> Correct, I don't believe there is a bug here; __ptep_set_access_flags() only
>> consumes the access flags from entry.
>>
>>>>
>>>> right.
>>>>
>>>>>
>>>>>
>>>>> OTOH, there is the initial:
>>>>>
>>>>>
>>>>> if (pte_same(pte, entry))
>>>>> return 0;
>>>>>
>>>>> check that might accelerate things.
>>
>> There is an equivalent check in contpte_ptep_set_access_flags() which is
>> checking for the whole contpte block and returning early if so. So I don't think
>> there is a problem here either.
>>
>>>>>
>>>>> So unless I am missing something, this works as expected? (and if the
>>>>> pte_same() would frequently be taken with your change would be worthwile
>>>>> to optimize)
>>>>
>>>>
>>>> Right. From page 1 to page (nr_pages - 1), we consistently get FALSE
>>>> for pte_same().
>>>> This seems quite strange. I think we might need to "fix" it, at least
>>>> for the sake of code
>>>> semantics. on the other hand, if pte_same() is not important, it
>>>> should be dropped.
>>>>
>>>> Hi Ryan,
>>>> what is your take on this?
>>
>> The code is correct and working as intended, AFAICT. But I accept that this is
>> not exactly obvious. I'd be happy to Rb your proposed change if you feel it
>> clarifies things.
>
> If this is the case, I'd rather add some comments instead in v2?
>
> diff --git a/arch/arm64/mm/contpte.c b/arch/arm64/mm/contpte.c
> index a3edced29ac1..55107d27d3f8 100644
> --- a/arch/arm64/mm/contpte.c
> +++ b/arch/arm64/mm/contpte.c
> @@ -421,6 +421,12 @@ int contpte_ptep_set_access_flags(struct
> vm_area_struct *vma,
> ptep = contpte_align_down(ptep);
> start_addr = addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
>
> + /*
> + * We are not advancing entry because __ptep_set_access_flags()
> + * only consumes access flags from entry. And since we
> have checked
> + * for the whole contpte block and returned early, pte_same()
> + * within __ptep_set_access_flags() is likely false.
> + */
> for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
> __ptep_set_access_flags(vma, addr, ptep, entry, 0);
LGTM:
Reviewed-by: Ryan Roberts <ryan.roberts@xxxxxxx>
