Message ID | 20231121212016.1154303-3-mhklinux@outlook.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | x86/coco: Mark CoCo VM pages not present when changing encrypted state | expand |
On Tue, 2023-11-21 at 13:20 -0800, mhkelley58@gmail.com wrote: > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -1636,7 +1636,10 @@ static int __change_page_attr(struct cpa_data > *cpa, int primary) > */ > if (pte_val(old_pte) != pte_val(new_pte)) { > set_pte_atomic(kpte, new_pte); > - cpa->flags |= CPA_FLUSHTLB; > + > + /* If old_pte isn't present, it's not in the > TLB */ > + if (pte_present(old_pte)) > + cpa->flags |= CPA_FLUSHTLB; > } > cpa->numpages = 1; > return 0; > Makes sense to me. The PMD case can be handled similarly in __should_split_large_page(). I also think it should be more robust in regards to the cache flushing changes. If callers did: set_memory_np() set_memory_uc() set_memory_p() Then the cache flush would be missed. I don't think anyone is, but we shouldn't introduce hidden things like that. Maybe fix it like this: diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index f519e5ca543b..28ff53a4447a 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -1856,11 +1856,6 @@ static int change_page_attr_set_clr(unsigned long *addr, int numpages, ret = __change_page_attr_set_clr(&cpa, 1); - /* - * Check whether we really changed something: - */ - if (!(cpa.flags & CPA_FLUSHTLB)) - goto out; /* * No need to flush, when we did not set any of the caching @@ -1868,6 +1863,12 @@ static int change_page_attr_set_clr(unsigned long *addr, int numpages, */ cache = !!pgprot2cachemode(mask_set); + /* + * Check whether we really changed something: + */ + if (!(cpa.flags & CPA_FLUSHTLB) && !cache) + goto out; + /* * On error; flush everything to be sure. */ Hmm, might want to maintain the "On error; flush everything to be sure" logic in the NP->P case as well.
From: Edgecombe, Rick P <rick.p.edgecombe@intel.com> Sent: Monday, November 27, 2023 2:21 PM > > On Tue, 2023-11-21 at 13:20 -0800, mhkelley58@gmail.com wrote: > > --- a/arch/x86/mm/pat/set_memory.c > > +++ b/arch/x86/mm/pat/set_memory.c > > @@ -1636,7 +1636,10 @@ static int __change_page_attr(struct cpa_data > > *cpa, int primary) > > */ > > if (pte_val(old_pte) != pte_val(new_pte)) { > > set_pte_atomic(kpte, new_pte); > > - cpa->flags |= CPA_FLUSHTLB; > > + > > + /* If old_pte isn't present, it's not in the TLB */ > > + if (pte_present(old_pte)) > > + cpa->flags |= CPA_FLUSHTLB; > > } > > cpa->numpages = 1; > > return 0; > > > > Makes sense to me. The PMD case can be handled similarly in > __should_split_large_page(). OK, I'll look at that case. > > I also think it should be more robust in regards to the cache flushing > changes. > > If callers did: > set_memory_np() > set_memory_uc() > set_memory_p() > > Then the cache flush would be missed. I don't think anyone is, but we > shouldn't introduce hidden things like that. Maybe fix it like this: > > diff --git a/arch/x86/mm/pat/set_memory.c > b/arch/x86/mm/pat/set_memory.c > index f519e5ca543b..28ff53a4447a 100644 > --- a/arch/x86/mm/pat/set_memory.c > +++ b/arch/x86/mm/pat/set_memory.c > @@ -1856,11 +1856,6 @@ static int change_page_attr_set_clr(unsigned > long *addr, int numpages, > > ret = __change_page_attr_set_clr(&cpa, 1); > > - /* > - * Check whether we really changed something: > - */ > - if (!(cpa.flags & CPA_FLUSHTLB)) > - goto out; > > /* > * No need to flush, when we did not set any of the caching > @@ -1868,6 +1863,12 @@ static int change_page_attr_set_clr(unsigned > long *addr, int numpages, > */ > cache = !!pgprot2cachemode(mask_set); > > + /* > + * Check whether we really changed something: > + */ > + if (!(cpa.flags & CPA_FLUSHTLB) && !cache) > + goto out; > + > /* > * On error; flush everything to be sure. > */ > > Hmm, might want to maintain the "On error; flush everything to be sure" > logic in the NP->P case as well. OK, I see your point. I had not realized that CPA_FLUSHTLB really has a meaning beyond just indicating that the TLB needs to be flushed. It really means "something has changed" in a PTE. I'll incorporate your suggestion. Michael
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c index 8e19796e7ce5..d7ef8d312a47 100644 --- a/arch/x86/mm/pat/set_memory.c +++ b/arch/x86/mm/pat/set_memory.c @@ -1636,7 +1636,10 @@ static int __change_page_attr(struct cpa_data *cpa, int primary) */ if (pte_val(old_pte) != pte_val(new_pte)) { set_pte_atomic(kpte, new_pte); - cpa->flags |= CPA_FLUSHTLB; + + /* If old_pte isn't present, it's not in the TLB */ + if (pte_present(old_pte)) + cpa->flags |= CPA_FLUSHTLB; } cpa->numpages = 1; return 0;