From: Petr Beneš <w1benny@gmail.com>

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@gmail.com>
---
 tools/misc/xen-access.c | 14 --------------
 xen/arch/x86/mm/p2m.c   |  3 +++
 2 files changed, 3 insertions(+), 14 deletions(-)

diff --git a/tools/misc/xen-access.c b/tools/misc/xen-access.c
index 4097eebe6f..6cf3b6a42c 100644
--- a/tools/misc/xen-access.c
+++ b/tools/misc/xen-access.c
@@ -517,9 +517,6 @@ int main(int argc, char *argv[])
     /* With altp2m we just create a new, restricted view of the memory */
     if ( memaccess && altp2m )
     {
-        xen_pfn_t gfn = 0;
-        unsigned long perm_set = 0;
-
         if( altp2m_write_no_gpt )
         {
             rc = xc_monitor_inguest_pagefault(xch, domain_id, 1);
@@ -551,17 +548,6 @@ int main(int argc, char *argv[])
         }
 
         DPRINTF("altp2m view created with id %u\n", altp2m_view_id);
-        DPRINTF("Setting altp2m mem_access permissions.. ");
-
-        for(; gfn < xenaccess->max_gpfn; ++gfn)
-        {
-            rc = xc_altp2m_set_mem_access( xch, domain_id, altp2m_view_id, gfn,
-                                           default_access);
-            if ( !rc )
-                perm_set++;
-        }
-
-        DPRINTF("done! Permissions set on %lu pages.\n", perm_set);
 
         rc = xc_altp2m_switch_to_view( xch, domain_id, altp2m_view_id );
         if ( rc < 0 )
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 0983bd71d9..4251144704 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1849,6 +1849,9 @@ bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l,
     amfn = _mfn(mfn_x(*mfn) & mask);
     gfn = _gfn(gfn_l & mask);
 
+    /* Override the altp2m entry with its default access. */
+    *p2ma = ap2m->default_access;
+
     rc = p2m_set_entry(ap2m, gfn, amfn, cur_order, *p2mt, *p2ma);
     p2m_unlock(ap2m);
 
-- 
2.34.1

On 06/02/2024 10:08 am, Petr Beneš wrote:
> From: Petr Beneš <w1benny@gmail.com>
>
> 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@gmail.com>

It appears that all discussion has completed.

So unless anyone screams, I'm going to commit this with Tamas' ack as
he's the major user of this functionality.

~Andrew

On Tue, Feb 6, 2024 at 6:08 PM Petr Beneš <w1benny@gmail.com> wrote:
>
> From: Petr Beneš <w1benny@gmail.com>
>
> 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@gmail.com>

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?

Tamas, I guess this is OK from an interface compatibility point of
view?  In theory it should always have been behaving this way.

 -George

On 07/02/2024 1:18 am, George Dunlap wrote:
> On Tue, Feb 6, 2024 at 6:08 PM Petr Beneš <w1benny@gmail.com> wrote:
>> From: Petr Beneš <w1benny@gmail.com>
>>
>> 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@gmail.com>
> 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.

~Andrew

On Wed, Feb 7, 2024 at 5:21 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 07/02/2024 1:18 am, George Dunlap wrote:
> > On Tue, Feb 6, 2024 at 6:08 PM Petr Beneš <w1benny@gmail.com> wrote:
> >> From: Petr Beneš <w1benny@gmail.com>
> >>
> >> 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@gmail.com>
> > 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

On Thu, Feb 8, 2024 at 9:21 AM Tamas K Lengyel <tamas@tklengyel.com> wrote:
>
> On Wed, Feb 7, 2024 at 5:21 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> >
> > On 07/02/2024 1:18 am, George Dunlap wrote:
> > > On Tue, Feb 6, 2024 at 6:08 PM Petr Beneš <w1benny@gmail.com> wrote:
> > >> From: Petr Beneš <w1benny@gmail.com>
> > >>
> > >> 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@gmail.com>
> > > 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.

Er, ok, just one more comment: this could allow an altp2m to have more
permissions than the host; for example, the host p2m entry could be
p2m_access_r, but if the altp2m's default_access were p2m_access_rw,
it would override that.  Is that the behavior we want?  Or do we want
to do some sort of intersection of permissions?

If the former, I'd propose the comment be adjusted thus:

 * If the entry is invalid, and the host entry was valid, propagate
 * the host's entry to the altp2m, retaining page order but using the
 * altp2m's default_access, and indicate that the caller should re-try
 * the faulting instruction.

I could do that on check-in.

 -George

On 08.02.2024 05:32, George Dunlap wrote:
> Er, ok, just one more comment: this could allow an altp2m to have more
> permissions than the host; for example, the host p2m entry could be
> p2m_access_r, but if the altp2m's default_access were p2m_access_rw,
> it would override that.  Is that the behavior we want?  Or do we want
> to do some sort of intersection of permissions?
> 
> If the former, I'd propose the comment be adjusted thus:
> 
>  * If the entry is invalid, and the host entry was valid, propagate
>  * the host's entry to the altp2m, retaining page order but using the
>  * altp2m's default_access, and indicate that the caller should re-try
>  * the faulting instruction.

I find it highly questionable that such blind overriding should be taking
place.

Jan

On Thu, Feb 8, 2024 at 2:46 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.02.2024 05:32, George Dunlap wrote:
> > Er, ok, just one more comment: this could allow an altp2m to have more
> > permissions than the host; for example, the host p2m entry could be
> > p2m_access_r, but if the altp2m's default_access were p2m_access_rw,
> > it would override that.  Is that the behavior we want?  Or do we want
> > to do some sort of intersection of permissions?
> >
> > If the former, I'd propose the comment be adjusted thus:

No intersection of permissions please, that needlessly complicates
things and makes it hard to reason about the state of a view where
default permissions are used. No need to force a specific type of
usecase here where the hostp2m's permissions are special just cause we
say so. No, the permissions in the hostp2m should not have more weight
then the specifically requested default permission.

> >
> >  * If the entry is invalid, and the host entry was valid, propagate
> >  * the host's entry to the altp2m, retaining page order but using the
> >  * altp2m's default_access, and indicate that the caller should re-try
> >  * the faulting instruction.
>
> I find it highly questionable that such blind overriding should be taking
> place.

It's not blind overriding, it's the requested default permission set
for a view where no entry was present before. It is the expected
behavior. It would be way harder to design applications with this
feature if it was special cased and it would take different
permissions based on what permission is set in another view.

Tamas

On 08.02.2024 14:45, Tamas K Lengyel wrote:
> On Thu, Feb 8, 2024 at 2:46 AM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 08.02.2024 05:32, George Dunlap wrote:
>>> Er, ok, just one more comment: this could allow an altp2m to have more
>>> permissions than the host; for example, the host p2m entry could be
>>> p2m_access_r, but if the altp2m's default_access were p2m_access_rw,
>>> it would override that.  Is that the behavior we want?  Or do we want
>>> to do some sort of intersection of permissions?
>>>
>>> If the former, I'd propose the comment be adjusted thus:
> 
> No intersection of permissions please, that needlessly complicates
> things and makes it hard to reason about the state of a view where
> default permissions are used. No need to force a specific type of
> usecase here where the hostp2m's permissions are special just cause we
> say so. No, the permissions in the hostp2m should not have more weight
> then the specifically requested default permission.
> 
>>>
>>>  * If the entry is invalid, and the host entry was valid, propagate
>>>  * the host's entry to the altp2m, retaining page order but using the
>>>  * altp2m's default_access, and indicate that the caller should re-try
>>>  * the faulting instruction.
>>
>> I find it highly questionable that such blind overriding should be taking
>> place.
> 
> It's not blind overriding, it's the requested default permission set
> for a view where no entry was present before. It is the expected
> behavior. It would be way harder to design applications with this
> feature if it was special cased and it would take different
> permissions based on what permission is set in another view.

But the default can be only one specific value: How do you make sure that
R/O, R/X, and R/W mappings all retain their respective restrictions in the
alternative view? To not over-restrict permissions, the default would then
need to be RWX, yet then all mappings will have full permission. What am I
missing?

Jan

On Thu, Feb 8, 2024 at 9:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 08.02.2024 14:45, Tamas K Lengyel wrote:
> > On Thu, Feb 8, 2024 at 2:46 AM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 08.02.2024 05:32, George Dunlap wrote:
> >>> Er, ok, just one more comment: this could allow an altp2m to have more
> >>> permissions than the host; for example, the host p2m entry could be
> >>> p2m_access_r, but if the altp2m's default_access were p2m_access_rw,
> >>> it would override that.  Is that the behavior we want?  Or do we want
> >>> to do some sort of intersection of permissions?
> >>>
> >>> If the former, I'd propose the comment be adjusted thus:
> >
> > No intersection of permissions please, that needlessly complicates
> > things and makes it hard to reason about the state of a view where
> > default permissions are used. No need to force a specific type of
> > usecase here where the hostp2m's permissions are special just cause we
> > say so. No, the permissions in the hostp2m should not have more weight
> > then the specifically requested default permission.
> >
> >>>
> >>>  * If the entry is invalid, and the host entry was valid, propagate
> >>>  * the host's entry to the altp2m, retaining page order but using the
> >>>  * altp2m's default_access, and indicate that the caller should re-try
> >>>  * the faulting instruction.
> >>
> >> I find it highly questionable that such blind overriding should be taking
> >> place.
> >
> > It's not blind overriding, it's the requested default permission set
> > for a view where no entry was present before. It is the expected
> > behavior. It would be way harder to design applications with this
> > feature if it was special cased and it would take different
> > permissions based on what permission is set in another view.
>
> But the default can be only one specific value: How do you make sure that
> R/O, R/X, and R/W mappings all retain their respective restrictions in the
> alternative view? To not over-restrict permissions, the default would then
> need to be RWX, yet then all mappings will have full permission. What am I
> missing?

Why do you assume you need to retain the permissions that were set in
the hostp2m across all altp2ms? It is entirely reasonable to set
permissions for a couple entries manually in an altp2m and set the
default to _n, which may be totally different then what the hostp2m
has. When you get the event for entries you didn't manually specify
you can decide which view to switch to, which may be the hostp2m, but
may be some other view.

If you wanted to have a view with a default permission that inherits
permissions that are always at least as restrictive as the hostp2m,
you could do that, but the way to do that would be to introduce a
special mem_access permission to signify that that's the expected
behavior (similar to permissions like n2rwx). I don't think anyone is
asking for that.

Tamas

On Tue, Feb 6, 2024 at 5:08 AM Petr Beneš <w1benny@gmail.com> wrote:
>
> From: Petr Beneš <w1benny@gmail.com>
>
> 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@gmail.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>