On 01.12.2021 21:13, Andrew Cooper wrote:
> On 01/12/2021 09:14, Jan Beulich wrote:
>> On 30.11.2021 19:11, Andrew Cooper wrote:
>>> Most functions in this call chain have 8 parameters, meaning that the final
>>> two booleans are spilled to the stack for for calls.
>>>
>>> First, delete nestedhap_walk_L1_p2m and introduce nhvm_hap_walk_L1_p2m() as
>>> a
>>> thin wrapper around hvm_funcs just like all the other nhvm_*() hooks. This
>>> involves including xen/mm.h as the forward declaration of struct npfec is no
>>> longer enough.
>>>
>>> Next, replace the triple of booleans with struct npfec, which contains the
>>> same information in the bottom 3 bits.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <[email protected]>
>>> ---
>>> CC: Jan Beulich <[email protected]>
>>> CC: Roger Pau Monné <[email protected]>
>>> CC: Wei Liu <[email protected]>
>>> CC: Tamas K Lengyel <[email protected]>
>>> CC: Alexandru Isaila <[email protected]>
>>> CC: Petre Pircalabu <[email protected]>
>>>
>>> I don't much like this, but I think it's the least bad option in the short
>>> term. npfec is horribly mis-named/mis-used (at best, it should be
>>> considered
>>> npf_info, and probably inherits from the same API/ABI mistakes our regular
>>> pagewalk functions have) and is going to have to be untangled to make nested
>>> virt a maintainable option.
>> So why use struct npfec here then in the first place? It could as well
>> be "unsigned int" with constants defined for X, R, and W, couldn't it?
>
> I started prototyping that first, but substantially ups the work
> required to support XU/XS down the line, and that's far more likely to
> happen before getting around to cleaning up the API/ABI.
Well, okay then.
>>> --- a/xen/include/asm-x86/hvm/hvm.h
>>> +++ b/xen/include/asm-x86/hvm/hvm.h
>>> @@ -25,6 +25,7 @@
>>> #include <asm/current.h>
>>> #include <asm/x86_emulate.h>
>>> #include <asm/hvm/asid.h>
>>> +#include <xen/mm.h>
>> Nit: Typically we have xen/ includes ahead of asm/ ones.
>
> Fixed.
>
>>
>>> @@ -631,6 +630,14 @@ static inline enum hvm_intblk
>>> nhvm_interrupt_blocked(struct vcpu *v)
>>> return hvm_funcs.nhvm_intr_blocked(v);
>>> }
>>>
>>> +static inline int nhvm_hap_walk_L1_p2m(
>>> + struct vcpu *v, paddr_t L2_gpa, paddr_t *L1_gpa, unsigned int
>>> *page_order,
>>> + uint8_t *p2m_acc, struct npfec npfec)
>>> +{
>>> + return hvm_funcs.nhvm_hap_walk_L1_p2m(
>>> + v, L2_gpa, L1_gpa, page_order, p2m_acc, npfec);
>>> +}
>> Is there a specific reason you don't switch to altcall right in
>> this patch, making a follow-on change unnecessary?
>
> I was still hoping to keep the altcall stuff in one patch. I'm happy to
> do the rebase.
I'm not worried about the (trivial) rebase. Yet both patches will be needed
anyway once we consider possible backporting of all of this work.
Reviewed-by: Jan Beulich <[email protected]>
Jan
