Re: [PATCH] x86/altp2m: p2m_altp2m_get_or_propagate() should honor ap2m->default_access

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, Feb 7, 2024 at 5:21 PM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 07/02/2024 1:18 am, George Dunlap wrote:
> > On Tue, Feb 6, 2024 at 6:08 PM Petr Beneš <w1benny@xxxxxxxxx> wrote:
> >> From: Petr Beneš <w1benny@xxxxxxxxx>
> >>
> >> This patch addresses a behavior discrepancy in the handling of altp2m 
> >> views,
> >> where upon the creation and subsequent EPT violation, the page access
> >> permissions were incorrectly inherited from the hostp2m instead of 
> >> respecting
> >> the altp2m default_access.
> >>
> >> Previously, when a new altp2m view was established with restrictive
> >> default_access permissions and activated via xc_altp2m_switch_to_view(),
> >> it failed to trigger an event on the first access violation.  This behavior
> >> diverged from the intended mechanism, where the altp2m's default_access
> >> should dictate the initial permissions, ensuring proper event triggering on
> >> access violations.
> >>
> >> The correction involves modifying the handling mechanism to respect the
> >> altp2m view's default_access upon its activation, eliminating the need for
> >> setting memory access permissions for the entire altp2m range (e.g. within
> >> xen-access.c).  This change not only aligns the behavior with the expected
> >> access control logic but also results in a significant performance 
> >> improvement
> >> by reducing the overhead associated with setting memory access permissions
> >> across the altp2m range.
> >>
> >> Signed-off-by: Petr Beneš <w1benny@xxxxxxxxx>
> > Thanks Petr, this looks like a great change.
> >
> > Two things:
> >
> > - Probably worth adjusting the comment at the top of
> > p2m_altp2m_get_or_propagate to mention that you use the altp2m
> > default_access when propagating from the host p2m
> >
> > - This represents a change in behavior, so probably at least worth a
> > mention in CHANGELOG.md?
>
> This is a bugfix to an tech preview feature.  It's not remotely close to
> CHANGELOG material.
>
> Tangent.  SUPPORT.md says tech preview on ARM, despite there being no
> support in the slightest.  Patches were posted and never taken.
>
> > Tamas, I guess this is OK from an interface compatibility point of
> > view?  In theory it should always have been behaving this way.
>
> Given the already-provided Ack, I expect that has been considered and
> deemed ok.

Correct, this is just a bugfix.

Tamas