Re: [PATCH 11/16] x86/shadow: drop is_hvm_...() where easily possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2023 19:18, Andrew Cooper wrote:
> On 22/03/2023 9:35 am, Jan Beulich wrote:
>> Emulation related functions are involved in HVM handling only, and in
>> some cases they even invoke such checks after having already done things
>> which are valid for HVM domains only. OOS active also implies HVM. In
>> sh_remove_all_mappings() one of the two checks is redundant with an
>> earlier paging_mode_external() one (the other, however, needs to stay).
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -1522,7 +1522,7 @@ int sh_remove_all_mappings(struct domain
>>                 && (page->count_info & PGC_count_mask) <= 3
>>                 && ((page->u.inuse.type_info & PGT_count_mask)
>>                     == (is_special_page(page) ||
>> -                       (is_hvm_domain(d) && is_ioreq_server_page(d, 
>> page))))) )
>> +                       is_ioreq_server_page(d, page)))) )
>>              printk(XENLOG_G_ERR "can't find all mappings of mfn %"PRI_mfn
>>                     " (gfn %"PRI_gfn"): c=%lx t=%lx s=%d i=%d\n",
>>                     mfn_x(gmfn), gfn_x(gfn),
> 
> Out of context here needs an equivalent adjustment.

I'm afraid I don't seen any further is_hvm_*() in this function.

> But in this case, I'm not sure the commit message covers the relevant
> details.  ioreq servers have been made fully common since this code was
> written, and *that* is a better reason for dropping the predicates IMO
> than the redundancy with paging_mode_external().

How does "fully common" matter? It's still a HVM-only thing, hence the
paging_mode_external() check just out of context. Also note that the
ioreq-server-page check is only one side of || (and I realize that by
correcting indentation here at the same time this might be better
visible).

> That said...  I'm not sure the logic here is correct any more.  It used
> to be the case that ioreq pages were in the p2m, but they're outside of
> the p2m these days, so don't see how there can be any interaction with
> unexpected refcounts any more.
> 
> I suspect that one way or another, this change wants to be in a separate
> patch.

I think that if there are further adjustments to make (like dropping
is_ioreq_server_page() altogether, as you appear to suggest), that would
want to be in a separate patch, but the change as done fully fits the
given justification. (Of course in such a patch both _could_ also be
dropped at the same time.)

>> --- a/xen/arch/x86/mm/shadow/multi.c
>> +++ b/xen/arch/x86/mm/shadow/multi.c
>> @@ -3441,7 +3441,7 @@ int sh_rm_write_access_from_sl1p(struct
>>  
>>  #ifdef CONFIG_HVM
>>      /* Remember if we've been told that this process is being torn down */
>> -    if ( curr->domain == d && is_hvm_domain(d) )
>> +    if ( curr->domain == d )
>>          curr->arch.paging.shadow.pagetable_dying
>>              = mfn_to_page(gmfn)->pagetable_dying;
>>  #endif
> 
> This one is dangerous.
> 
> After tracing, I can see that sh_rm_write_access_from_sl1p() is only
> called from OOS functions, but this function itself does its very best
> to look like it has mixed PV + HVM usage, and dropping this conditional
> means that pagetable_dying can, in principle at least, become non-NULL
> for a PV guest.
> 
> I think this function needs to be made far more obviously HVM-only first.

Oh, sure - the #ifdef inside the functions can be replaced collectively
by one around it, now that OOS code is built separately and for HVM only.

>> --- a/xen/arch/x86/mm/shadow/oos.c
>> +++ b/xen/arch/x86/mm/shadow/oos.c
>> @@ -577,7 +577,6 @@ int sh_unsync(struct vcpu *v, mfn_t gmfn
>>      if ( pg->shadow_flags &
>>           ((SHF_page_type_mask & ~SHF_L1_ANY) | SHF_out_of_sync)
>>           || sh_page_has_multiple_shadows(pg)
>> -         || !is_hvm_vcpu(v)
>>           || !v->domain->arch.paging.shadow.oos_active )
> 
> This is reachable for PV guests as far as I can see.  What am I missing ?

Well, the footnote in patch 1 ("x86/shadow: fix and improve
sh_page_has_multiple_shadows()") kind of explains this wrt the safety
of the sh_page_has_multiple_shadows() use here: Since PV guests can't
have OOS pages, there's no way SHF_out_of_sync could be set.

> The changes in hvm.c are all fine, and for those alone, consider it R-by
> if you end up splitting the patch.

Thanks, but for now I'm not meaning to split the patch, as per above.
There will be a new prereq patch as per your request.

Jan