| Message ID | c70f4c4584cf28ff1da8f56e08d61ad0c406a4fd.1670300446.git.demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make PAT handling less brittle | expand |
On 06/12/2022 04:33, Demi Marie Obenour wrote: > This still hard-codes the assumption that the two spare values are > mapped to UC. Removing this assumption would require a more complex > patch. > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > xen/arch/x86/mm.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c > index 78b1972e4170cacccc9c37c6e64e76e66a7da87f..5d05399c3a841bf03991a3bed63df9a815c1e891 100644 > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -961,13 +961,10 @@ get_page_from_l1e( > As you noted that this desperately needs a comment, how about /* Force cacheable memtypes to UC, */ here? Can fix up on commit. ~Andrew > switch ( l1f & PAGE_CACHE_ATTRS ) > { > - case 0: /* WB */ > - flip |= _PAGE_PWT | _PAGE_PCD; > - break; > - case _PAGE_PWT: /* WT */ > - case _PAGE_PWT | _PAGE_PAT: /* WP */ > - flip |= _PAGE_PCD | (l1f & _PAGE_PAT); > - break; > + case _PAGE_WB: > + case _PAGE_WT: > + case _PAGE_WP: > + flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC; > } > > return flip;
On 06.12.2022 05:33, Demi Marie Obenour wrote: > This still hard-codes the assumption that the two spare values are > mapped to UC. Removing this assumption would require a more complex > patch. > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> You eliminate some assumptions at the price of introducing new ones. With that I consider the description insufficient; really there isn't any description here, just a statement on something you do _not_ do. > --- a/xen/arch/x86/mm.c > +++ b/xen/arch/x86/mm.c > @@ -961,13 +961,10 @@ get_page_from_l1e( > > switch ( l1f & PAGE_CACHE_ATTRS ) > { > - case 0: /* WB */ > - flip |= _PAGE_PWT | _PAGE_PCD; > - break; > - case _PAGE_PWT: /* WT */ > - case _PAGE_PWT | _PAGE_PAT: /* WP */ > - flip |= _PAGE_PCD | (l1f & _PAGE_PAT); > - break; > + case _PAGE_WB: > + case _PAGE_WT: > + case _PAGE_WP: > + flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC; > } Please never drop "break" from case blocks, even if it's (at a given point in time) the last in its switch(). That's a recipe for someone else then forgetting to add it when another case block is added. Jan
On 06.12.2022 11:42, Andrew Cooper wrote: >> --- a/xen/arch/x86/mm.c >> +++ b/xen/arch/x86/mm.c >> @@ -961,13 +961,10 @@ get_page_from_l1e( >> > > As you noted that this desperately needs a comment, how about > > /* Force cacheable memtypes to UC, */ > > here? The comment you appear to be missing is actually there, just perhaps a bit too far ahead: /* MMIO pages must not be mapped cachable unless requested so. */ In any event the comment you suggest to add merely re-states in different words what this earlier one already says. Jan
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index 78b1972e4170cacccc9c37c6e64e76e66a7da87f..5d05399c3a841bf03991a3bed63df9a815c1e891 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -961,13 +961,10 @@ get_page_from_l1e( switch ( l1f & PAGE_CACHE_ATTRS ) { - case 0: /* WB */ - flip |= _PAGE_PWT | _PAGE_PCD; - break; - case _PAGE_PWT: /* WT */ - case _PAGE_PWT | _PAGE_PAT: /* WP */ - flip |= _PAGE_PCD | (l1f & _PAGE_PAT); - break; + case _PAGE_WB: + case _PAGE_WT: + case _PAGE_WP: + flip |= (l1f & PAGE_CACHE_ATTRS) ^ _PAGE_UC; } return flip;
This still hard-codes the assumption that the two spare values are mapped to UC. Removing this assumption would require a more complex patch. Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> --- xen/arch/x86/mm.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)