While PGT_pae_xen_l2 will be zapped once the type refcount of an L2 page
reaches zero, it'll be retained as long as the type refcount is non-
zero. Hence any checking against the requested type needs to either zap
the bit from the type or include it in the used mask.

Fixes: 9186e96b199e ("x86/pv: Clean up _get_page_type()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The check around the TLB flush which was moved for XSA-401 also looks to
needlessly trigger a flush when "type" has the bit set (while "x"
wouldn't). That's no different from original behavior, but still looks
inefficient.

--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2956,7 +2956,8 @@ static int _get_page_type(struct page_in
              * The page is in one of two states (depending on PGT_partial),
              * and should have exactly one reference.
              */
-            ASSERT((x & (PGT_type_mask | PGT_count_mask)) == (type | 1));
+            ASSERT((x & (PGT_type_mask | PGT_pae_xen_l2 | PGT_count_mask)) ==
+                   (type | 1));
 
             if ( !(x & PGT_partial) )
             {

On 10/06/2022 08:26, Jan Beulich wrote:
> While PGT_pae_xen_l2 will be zapped once the type refcount of an L2 page
> reaches zero, it'll be retained as long as the type refcount is non-
> zero. Hence any checking against the requested type needs to either zap
> the bit from the type or include it in the used mask.
>
> Fixes: 9186e96b199e ("x86/pv: Clean up _get_page_type()")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

pae_xen_l2 being outside of the typemask is deeply confusing to work
with.  It also renders all of the comments trying to explain the
structure of this logic wrong.

I'm a little concerned with type usage in the non-coherent path too. 
It's safe, but is (along side the IOMMU path) a misleading example to
surrounding code.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I can't think of anything better to do in the short term.

> ---
> The check around the TLB flush which was moved for XSA-401 also looks to
> needlessly trigger a flush when "type" has the bit set (while "x"
> wouldn't). That's no different from original behavior, but still looks
> inefficient.

It's not the only inefficiency here.  Still plenty of improvements to be
had in _get_page_type().

~Andrew

On 10.06.2022 10:12, Andrew Cooper wrote:
> On 10/06/2022 08:26, Jan Beulich wrote:
>> While PGT_pae_xen_l2 will be zapped once the type refcount of an L2 page
>> reaches zero, it'll be retained as long as the type refcount is non-
>> zero. Hence any checking against the requested type needs to either zap
>> the bit from the type or include it in the used mask.
>>
>> Fixes: 9186e96b199e ("x86/pv: Clean up _get_page_type()")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> pae_xen_l2 being outside of the typemask is deeply confusing to work
> with.  It also renders all of the comments trying to explain the
> structure of this logic wrong.
> 
> I'm a little concerned with type usage in the non-coherent path too. 
> It's safe, but is (along side the IOMMU path) a misleading example to
> surrounding code.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> I can't think of anything better to do in the short term.
> 
>> ---
>> The check around the TLB flush which was moved for XSA-401 also looks to
>> needlessly trigger a flush when "type" has the bit set (while "x"
>> wouldn't). That's no different from original behavior, but still looks
>> inefficient.
> 
> It's not the only inefficiency here.  Still plenty of improvements to be
> had in _get_page_type().

You did say you have some follow-up changes pending. It wasn't clear to
me whether this particular aspect was among them. If not, I can make
a(nother) patch ...

Jan

On 10/06/2022 09:17, Jan Beulich wrote:
> On 10.06.2022 10:12, Andrew Cooper wrote:
>> On 10/06/2022 08:26, Jan Beulich wrote:
>>> ---
>>> The check around the TLB flush which was moved for XSA-401 also looks to
>>> needlessly trigger a flush when "type" has the bit set (while "x"
>>> wouldn't). That's no different from original behavior, but still looks
>>> inefficient.
>> It's not the only inefficiency here.  Still plenty of improvements to be
>> had in _get_page_type().
> You did say you have some follow-up changes pending. It wasn't clear to
> me whether this particular aspect was among them. If not, I can make
> a(nother) patch ...

At this point, it's probably more accurate to say that I've got a pile
of plans about making improvements, rather than a pile of patches.

The major improvement is the early exit for PGT_validated, making the
second half of the function exclusively for the validate-locked state.

Other improvements (off the top of my head) are shuffling the TLB flush
setup logic to not even do the tlb filter calculations if we're going to
skip the flush call anyway, deduping the page_get_owner()
calls/variables, sorting out PGC_page_table naming/semantics, reducing
the number of redundant ways we've got of expressing the same logic
(there are a lot of invariants between x, nx and type), better
explanation of the iommu behaviour, and better explanation of the tlb
flushing safety requirements.

If you think some of that's easy enough to do, then feel free.

~Andrew