| Message ID | 1b854c6b38787675294c58eea25556ce241b2b4f.1707213023.git.w1benny@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/altp2m: p2m_altp2m_get_or_propagate() should honor ap2m->default_access | expand |
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>
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 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
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);