Re: [PATCH 2/2] x86/hvm: Rework nested hap functions to reduce parameters

[Andrew Cooper <amc96@xxxxxxxx>]

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 <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Tamas K Lengyel <tamas@xxxxxxxxxxxxx>
>> CC: Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>
>> CC: Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>
>>
>> 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.

>> --- 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.

~Andrew