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