Re: [PATCH] x86/HVM: convert most remaining hvm_funcs hook invocations to alt-call

[Andrew Cooper <amc96@xxxxxxxx>]

On 29/11/2021 09:04, Jan Beulich wrote:
> The aim being to have as few indirect calls as possible (see [1]),
> whereas during initial conversion performance was the main aspect and
> hence rarely used hooks didn't get converted. Apparently one use of
> get_interrupt_shadow() was missed at the time.
>
> While I've intentionally left alone the cpu_{up,down}() etc hooks for
> not being guest reachable, the nhvm_hap_walk_L1_p2m() one can't
> currently be converted as the framework supports only up to 6 arguments.
> Down the road the three booleans perhaps want folding into a single
> parameter/argument.

To use __initdata_cf_clobber, all hooks need to use altcall().

There is also an open question on how to cope with things such as the
TSC scaling hooks, which are only conditionally set in {vmx,svm}_hvm_funcs.

I was expecting to have to introduce a macro for per-function
registration in the cf_clobber section, given some of the lone function
pointers used with altcall().


As for >6 arguments, we really should discourage such functions
generally, because spilling parameters to the stack is not a efficient
thing to do in the slightest.


>
> While doing this, drop NULL checks ahead of .nhvm_*() calls when the
> hook is always present. Also convert the .nhvm_vcpu_reset() call to
> alternative_vcall(), as the return value is unused and the caller has
> currently no way of propagating it.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>.  However...

>
> [1] https://lists.xen.org/archives/html/xen-devel/2021-11/msg01822.html
> ---
> Another candidate for dropping the conditional would be
> .enable_msr_interception(), but this would then want the wrapper to also
> return void (hence perhaps better done separately).

I think that's a side effect of Intel support being added first, and
then an incomplete attempt to add AMD support.

Seeing as support isn't disappearing, I'd be in favour of reducing it to
void.  The sole caller already doesn't check the return value.


If I do a prep series sorting out nhvm_hap_walk_L1_p2m() and
enable_msr_interception(), would you be happy rebasing this patch and
adjusting every caller, including cpu_up/down() ?

> --- a/xen/arch/x86/monitor.c
> +++ b/xen/arch/x86/monitor.c
> @@ -270,7 +270,8 @@ int arch_monitor_domctl_event(struct dom
>          ad->monitor.descriptor_access_enabled = requested_status;
>  
>          for_each_vcpu ( d, v )
> -            hvm_funcs.set_descriptor_access_exiting(v, requested_status);
> +            alternative_vcall(hvm_funcs.set_descriptor_access_exiting, v,
> +                              requested_status);

(For a future change...)  It occurs to me that this wants to be:

for_each_vcpu ( d, v )
    v->arch.hvm.recalc_intercepts = true;

and leave the resume path to do the right thing.

While it's not a big deal for AMD, avoiding the line of VMCS loads on
Intel really is a big deal.

~Andrew