Hi Andre,
On Tue, 11 Mar 2025 at 20:31, Andre Przywara <andre.przyw...@arm.com> wrote:
>
> On Sat, 1 Mar 2025 18:49:02 +0200
> Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote:
>
> Hi Ilias,
>
> > The ARM ARM on section 8.17.1 describes the cases where
>
> Thanks for referencing the ARM ARM! The section name would be D8.17.1, and
> please mention the version of the document, since the numbering is not
> stable across the different releases. So something like:
>
> The ARM ARM (Rev L.a) in section D8.17.1 ("Using break-before-make when
> updating translation table entries") ...
Sure
>
> > break-before-make is required when changing live page tables.
> > Since we can use this function to tweak block and page permssions,
> > where BBM is not required add an extra argument to the function.
>
> It looks like one of the two existing users of this function (the
> snapdragon code) just calls this function with attr=PTE_TYPE_FAULT, so
> that would not need to call the full BBM version.
> The Layerscape code indeed requires it, though.
Yep, I'll leave it up the qualcomm maintainers to pick that
>
> > While at it add a function description.
>
> I think these sentences describe the v2 and older, but don't fit the
> current patch anymore.
>
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> > ---
> > arch/arm/cpu/armv8/cache_v8.c | 48 ++++++++++++++++++++---------------
> > arch/arm/include/asm/system.h | 18 +++++++++++++
> > 2 files changed, 46 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/cache_v8.c b/arch/arm/cpu/armv8/cache_v8.c
> > index c4b3da4a8da7..29100913bc82 100644
> > --- a/arch/arm/cpu/armv8/cache_v8.c
> > +++ b/arch/arm/cpu/armv8/cache_v8.c
> > @@ -967,61 +967,69 @@ void mmu_set_region_dcache_behaviour(phys_addr_t
> > start, size_t size,
> > flush_dcache_range(real_start, real_start + real_size);
> > }
> >
> > -/*
> > - * Modify MMU table for a region with updated PXN/UXN/Memory type/valid
> > bits.
> > - * The procecess is break-before-make. The target region will be marked as
> > - * invalid during the process of changing.
> > - */
> > -void mmu_change_region_attr(phys_addr_t addr, size_t siz, u64 attrs)
> > +void mmu_change_region_attr_nobreak(phys_addr_t addr, size_t siz, u64
> > attrs)
> > {
> > int level;
> > u64 r, size, start;
> >
> > - start = addr;
> > - size = siz;
> > /*
> > * Loop through the address range until we find a page granule that
> > fits
> > - * our alignment constraints, then set it to "invalid".
> > + * our alignment constraints, then set it to the new cache attributes
>
> It's not cache attributes (those would require BBM), but "permissions" or
> just attributes.
Yep correct
>
> Otherwise this looks correct, just changing the permission indeed does not
> require BBM, so it's just the comment and the commit message that might
> need changing. And clever function split up!
That was what Richard requested, the initial one was just taking a
bool argument, but this is indeed better
Since it's only commit message changes, if n one else objects, I'll
amend it on my PR to Tom
Thanks for taking the time
/Ilias
>
> Cheers,
> Andre
>
> > */
> > + start = addr;
> > + size = siz;
> > while (size > 0) {
> > for (level = 1; level < 4; level++) {
> > - /* Set PTE to fault */
> > - r = set_one_region(start, size, PTE_TYPE_FAULT, true,
> > - level);
> > + /* Set PTE to new attributes */
> > + r = set_one_region(start, size, attrs, true, level);
> > if (r) {
> > - /* PTE successfully invalidated */
> > + /* PTE successfully updated */
> > size -= r;
> > start += r;
> > break;
> > }
> > }
> > }
> > -
> > flush_dcache_range(gd->arch.tlb_addr,
> > gd->arch.tlb_addr + gd->arch.tlb_size);
> > __asm_invalidate_tlb_all();
> > +}
> > +
> > +/*
> > + * Modify MMU table for a region with updated PXN/UXN/Memory type/valid
> > bits.
> > + * The procecess is break-before-make. The target region will be marked as
> > + * invalid during the process of changing.
> > + */
> > +void mmu_change_region_attr(phys_addr_t addr, size_t siz, u64 attrs)
> > +{
> > + int level;
> > + u64 r, size, start;
> >
> > + start = addr;
> > + size = siz;
> > /*
> > * Loop through the address range until we find a page granule that
> > fits
> > - * our alignment constraints, then set it to the new cache attributes
> > + * our alignment constraints, then set it to "invalid".
> > */
> > - start = addr;
> > - size = siz;
> > while (size > 0) {
> > for (level = 1; level < 4; level++) {
> > - /* Set PTE to new attributes */
> > - r = set_one_region(start, size, attrs, true, level);
> > + /* Set PTE to fault */
> > + r = set_one_region(start, size, PTE_TYPE_FAULT, true,
> > + level);
> > if (r) {
> > - /* PTE successfully updated */
> > + /* PTE successfully invalidated */
> > size -= r;
> > start += r;
> > break;
> > }
> > }
> > }
> > +
> > flush_dcache_range(gd->arch.tlb_addr,
> > gd->arch.tlb_addr + gd->arch.tlb_size);
> > __asm_invalidate_tlb_all();
> > +
> > + mmu_change_region_attr_nobreak(addr, siz, attrs);
> > }
> >
> > #else /* !CONFIG_IS_ENABLED(SYS_DCACHE_OFF) */
> > diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
> > index 091082281c73..849b3d0efb7a 100644
> > --- a/arch/arm/include/asm/system.h
> > +++ b/arch/arm/include/asm/system.h
> > @@ -303,8 +303,26 @@ void flush_l3_cache(void);
> > * @emerg: Also map the region in the emergency table
> > */
> > void mmu_map_region(phys_addr_t start, u64 size, bool emerg);
> > +
> > +/**
> > + * mmu_change_region_attr() - change a mapped region attributes
> > + *
> > + * @start: Start address of the region
> > + * @size: Size of the region
> > + * @aatrs: New attributes
> > + */
> > void mmu_change_region_attr(phys_addr_t start, size_t size, u64 attrs);
> >
> > +/**
> > + * mmu_change_region_attr_nobreak() - change a mapped region attributes
> > without doing
> > + * break-before-make
> > + *
> > + * @start: Start address of the region
> > + * @size: Size of the region
> > + * @aatrs: New attributes
> > + */
> > +void mmu_change_region_attr_nobreak(phys_addr_t addr, size_t size, u64
> > attrs);
> > +
> > /*
> > * smc_call() - issue a secure monitor call
> > *
>
