Re: [Intel-gfx] [PATCH v2] x86/mm: Fix PAT bit missing from page protection modify mask

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Wed, 2023-06-07 at 23:33 +0200, Janusz Krzysztofik wrote:
> > So since _PAGE_PSE is actually the same value as _PAGE_PAT, you
> > don't
> > actually need to have _PAGE_PSE in _HPAGE_CHG_MASK in order to get
> > functional correctness. Is that right?
> 
> As soon as we add _PAGE_PAT to _PAGE_CHG_MASK -- yes, that's right. 
> But we 
> may still want to add _PAGE_PSE to _HPAGE_CHG_MASK to have the need
> for that 
> bit explicitly documented.

_PAGE_PSE is already in _HPAGE_CHG_MASK though, right? I'm confused.

> 
> > 
> > I think it is still a little hidden (even before this) and I wonder
> > about separating out the common bits into, like,
> > _COMMON_PAGE_CHG_MASK
> > or something. Then setting specific PAGE and HPAGE bits (like
> > _PAGE_PAT, _PAGE_PSE and _PAGE_PAT_LARGE) in their specific define.
> > Would it be more readable that way?
> 
> Yes, I think that's a good idea, and I can use it in my patch.
> 
> The question if _PAGE_PAT vel _PAGE_PSE added to _PAGE_CHG_MASK is
> really 
> harmless for pte_modify() and its users is still open for me though.

When you say "vel", this is similar to the english acronym "AKA" I
think?

So I think you mean, when you add _PAGE_PAT to _PAGE_CHG_MASK, you are
also adding _PAGE_PSE to it. So does that cause any problems? I see,
good question... 

vm_page_prot is used when creating PTEs and huge PMDs, and the setter
only uses _PAGE_CHG_MASK, even though it won't actually know where that
prot is going to end up.

Having _PAGE_PAT/PSE in _PAGE_CHG_MASK certainly doesn't make it easier
to think about. One thing it's favor though is vm_page_prot is not
applied to page table entries that are pointing to other page table
entries (PSE = 0). So you shouldn't accidentally set PSE=1. And
_PAGE_PSE shouldn't be being set in there, so you also shouldn't
accidentally be setting PAT=1.

But yea, I see why you are concerned. I would /guess/ it would be ok
functionally. That probably doesn't help much...