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

[Andrew Cooper <amc96@xxxxxxxx>]

On 30/11/2021 14:03, Jan Beulich wrote:
> On 30.11.2021 14:48, Andrew Cooper wrote:
>> 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().
> Right, but that's not going to be sufficient: The data members then also
> need to move elsewhere, aiui.

Nope.  It is safe for data members to stay.

>> 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.
> Why's that an open question? The requirement is that the pointers be
> set before the 2nd pass of alternatives patching (it's really just
> one: setup()). That's already the case, or else the hook couldn't be
> invoked via altcall. And that's also not the only hook getting set
> dynamically.

This was in reference to cf_clobber, not altcall().

If the conditional hooks aren't added into {vmx,svm}_hvm_funcs, then the
clobbering loop can't find them.

>
>>   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() ?
> Sure. I don't think cleaning up enable_msr_interception() is a prereq
> though. The potential for doing so was merely an observation while
> going through the hook uses.

Yeah, I suppose that one can be a followup.

> With it not being sufficient to convert the remaining hook invocations
> and with the patch already being quite large, I wonder though whether
> it wouldn't make sense to make further conversions the subject of a
> fresh patch. I could commit this one then with your R-b (and further
> acks, once they have trickled in) once the tree is fully open again.

Honestly, this is legitimately "tree-wide".  While the patch is already
large, 3 extra hooks (on top of a fix for nhvm_hap_walk_L1_p2m()) is
mechanical, and probably easier than two patches.

~Andrew