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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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?

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?

~Andrew