[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <amc96@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 2 Dec 2021 09:06:47 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Jg/nohhn9niZ11G4oPwBwbPMDTh20ap9Lt0wxAEh1x4=; b=GycWv2Me/1HFSniPd7H+oiMFGkaLYErjjSduvMJGS2vnN7C9nDw0g/t28y9epnIeA/1tVOv6gcWLcr3eoIs4HlbFhhi/aNqxz9oLDZD+5XcjvSZd+MbGaQegnFuWh6NfQqL+a+OpawOgSlFTv5Be/IS71Y+HL7oGCxBgGjdhthptC9YbOn2alDI2CnTZ8jn3xl0ksZJTOgLayBfwTMhB+F8nYcpFGRU3y+O4dTyFfYF0s5fQUSZcyQhzyB3+iZdaxFoSaXtRE9LciFJpsiJsXR6dpbnppjJBf8MVknYHZ8nXfyb963IM7XFFeg4wodwRxdsdpbX7S6AA70A/FRw7IA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Fk7jWQiE/jktbYd/5t39+JfXWw4aE0Al7o3iOpvWXVKWFAfv2aqrTXTtjVjxD+ityQTSBu9QLhY6O9OYxaDSz95tzo0EnHv+BIZeaYd0REJNRuIp/JUEW13Wee0FYv0BDb3jZ4dE3blWgprEXwdjngpkIaf20FClWbtRBZjDZfWihtGhPqjE5u6oezb4Mis8w/MNij1SMjhAUqKnGXdQcuFeB0XvyP1UEQPSlb/dDTy7I3+fKPkyUzdTlkiXXFDRLFhDYqmEFivKKGOTdMHFfNpME67OIGVUj3feMo2IK5KrQ89hP8XX7jB65bDJzg3wxgA1St7LKYnKBE4+quRNBQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Tamas K Lengyel <tamas@xxxxxxxxxxxxx>, Alexandru Isaila <aisaila@xxxxxxxxxxxxxxx>, Petre Pircalabu <ppircalabu@xxxxxxxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Thu, 02 Dec 2021 08:07:03 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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

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 <jbeulich@xxxxxxxx>

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.