Re: [PATCH 12/16] x86/shadow: make monitor table create/destroy more consistent

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.03.2023 19:28, Andrew Cooper wrote:
> On 22/03/2023 9:35 am, Jan Beulich wrote:
>> While benign at present, it is still a little fragile to operate on a
>> wrong "old_mode" value in sh_update_paging_modes(). This can happen when
>> no monitor table was present initially - we'd create one for the new
>> mode without updating old_mode. Correct this two ways, each of which
> 
> I think you mean "Correct this in two ways" ?
> 
>> would be sufficient on its own: Once by adding "else" to the second of
>> the involved if()s in the function, and then by setting the correct
>> initial mode for HVM domains in shadow_vcpu_init().
>>
>> Further use the same predicate (paging_mode_external()) consistently
>> when dealing with shadow mode init/update/cleanup, rather than a mix of
>> is_hvm_vcpu() (init), is_hvm_domain() (update), and
>> paging_mode_external() (cleanup).
>>
>> Finally drop a redundant is_hvm_domain() from inside the bigger if()
>> (which is being converted to paging_mode_external()) in
>> sh_update_paging_modes().
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>
>> --- a/xen/arch/x86/mm/shadow/common.c
>> +++ b/xen/arch/x86/mm/shadow/common.c
>> @@ -129,8 +129,8 @@ void shadow_vcpu_init(struct vcpu *v)
>>      }
>>  #endif
>>  
>> -    v->arch.paging.mode = is_hvm_vcpu(v) ?
>> -                          &SHADOW_INTERNAL_NAME(sh_paging_mode, 3) :
>> +    v->arch.paging.mode = paging_mode_external(v->domain) ?
>> +                          &SHADOW_INTERNAL_NAME(sh_paging_mode, 2) :
>>                            &SHADOW_INTERNAL_NAME(sh_paging_mode, 4);
> 
> As you're changing this, reposition the ? and : to the start of the
> following lines?

Sure.

> But, is 2-level mode actually right?  It's better than 3 certainly, and
> is what sh_update_paging_modes() selects, but isn't that only right for
> the IDENT_PT case?

Which is how HVM vCPU-s start, isn't it? For PVH there clearly needs to
be a separate (later) paging mode update anyway (or else what - as you
say - sh_update_paging_modes() selects would also be wrong), which is
going to be whenever we see CR0.PG becoming set by the toolstack.

Jan