| Message ID | 94c76a8be13a592ef98c80525b6c016cee583e73.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 makes the code much easier to understand. > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> > --- > xen/arch/x86/mm/p2m-pt.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c > index eaba2b0fb4e6830f52b7d112fba8175dfe6d2770..cd1af33b6772ab1016e8d4c3284a6bc5d282869d 100644 > --- a/xen/arch/x86/mm/p2m-pt.c > +++ b/xen/arch/x86/mm/p2m-pt.c > @@ -99,13 +99,13 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m, > return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT; > case p2m_mmio_direct: > if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) ) > - flags |= _PAGE_RW; > + flags |= _PAGE_RW | _PAGE_UCM; > else > { > - flags |= _PAGE_PWT; > + flags |= _PAGE_UC; > ASSERT(!level); > } > - return flags | P2M_BASE_FLAGS | _PAGE_PCD; > + return flags | P2M_BASE_FLAGS; I agree that this is a correct transformation of the logic, but the logic cannot possibly be correct in the first place. The read-only-ness of the MMIO range has no legitimate bearing on UC vs UC-. I have a feeling this is another attempt to control mixed cacheability - the HVM side cleanup from XSA-402 is still very much pending. I'm tempted to R-by and commit the patch, with a note in the commit message saying that the transformation is correct but that it highlights that the pre-existing logic is suspect. ~Andrew
On 06.12.2022 11:59, Andrew Cooper wrote: > On 06/12/2022 04:33, Demi Marie Obenour wrote: >> This makes the code much easier to understand. >> >> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> >> --- >> xen/arch/x86/mm/p2m-pt.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c >> index eaba2b0fb4e6830f52b7d112fba8175dfe6d2770..cd1af33b6772ab1016e8d4c3284a6bc5d282869d 100644 >> --- a/xen/arch/x86/mm/p2m-pt.c >> +++ b/xen/arch/x86/mm/p2m-pt.c >> @@ -99,13 +99,13 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m, >> return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT; >> case p2m_mmio_direct: >> if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) ) >> - flags |= _PAGE_RW; >> + flags |= _PAGE_RW | _PAGE_UCM; >> else >> { >> - flags |= _PAGE_PWT; >> + flags |= _PAGE_UC; >> ASSERT(!level); >> } >> - return flags | P2M_BASE_FLAGS | _PAGE_PCD; >> + return flags | P2M_BASE_FLAGS; > > I agree that this is a correct transformation of the logic, but the > logic cannot possibly be correct in the first place. > > The read-only-ness of the MMIO range has no legitimate bearing on UC vs > UC-. I have a feeling this is another attempt to control mixed > cacheability I think so, yes - at the time the goal was to disallow cachability other than the one used by Xen for those pages where we know Xen has a mapping. Jan > - the HVM side cleanup from XSA-402 is still very much pending. > > I'm tempted to R-by and commit the patch, with a note in the commit > message saying that the transformation is correct but that it highlights > that the pre-existing logic is suspect. > > ~Andrew
diff --git a/xen/arch/x86/mm/p2m-pt.c b/xen/arch/x86/mm/p2m-pt.c index eaba2b0fb4e6830f52b7d112fba8175dfe6d2770..cd1af33b6772ab1016e8d4c3284a6bc5d282869d 100644 --- a/xen/arch/x86/mm/p2m-pt.c +++ b/xen/arch/x86/mm/p2m-pt.c @@ -99,13 +99,13 @@ static unsigned long p2m_type_to_flags(const struct p2m_domain *p2m, return flags | P2M_BASE_FLAGS | _PAGE_RW | _PAGE_NX_BIT; case p2m_mmio_direct: if ( !rangeset_contains_singleton(mmio_ro_ranges, mfn_x(mfn)) ) - flags |= _PAGE_RW; + flags |= _PAGE_RW | _PAGE_UCM; else { - flags |= _PAGE_PWT; + flags |= _PAGE_UC; ASSERT(!level); } - return flags | P2M_BASE_FLAGS | _PAGE_PCD; + return flags | P2M_BASE_FLAGS; } }
This makes the code much easier to understand. Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com> --- xen/arch/x86/mm/p2m-pt.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)