Re: [PATCH 2/4] x86/svm: cleanup svm.h

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 2/21/23 01:08, Andrew Cooper wrote:
> On 17/02/2023 6:48 pm, Xenia Ragiadakou wrote:
> > Remove the forward declaration of struct vcpu because it is not used.
> Huh, turns out that was my fault in c/s b158e72abe, shortly after I
> introduced them in the first place.
>
> Also, looking into that, there's one legitimate use of svm.h from
> outside, which is svm_load_segs*() which means we can't make all the
> headers be local.
>
> But still, most of svm.h shouldn't be includable in the rest of Xen.
> Perhaps we can make a separate dedicated header for just this.
>
> [edit]  And svm_{doman,vcpu}.  Perhaps we want a brand new
> include/asm/hvm/svm.h with only the things needed elsewhere.
This can be done as part of the series aiming to make svm/vmx support 
configurable.
> > Move the forward declaration of struct cpu_user_regs just above the
> > function that needs it (to remind that it will need to be removed
> > along with the function).
> I'm not sure.  This feels like churn.
Ok I will leave it where it is in v2.

> > Move the definitions of NPT_PFEC_with_gla and NPT_PFEC_in_gpt in svm.c
> > because they are used only in this file.
> IMO, these would better live in vmcb.h because that's where all the
> other decode information lives, not that there is much.  I previously
> started trying to convert all the exit_into fields into a typed union,
> but I didn't get as far the NPT info.
Ok I will fix it in v2.

> > Move the definitions of SVM_PAUSE{FILTER,THRESH}_INIT in vmcb.c because
> > they are used only in this file.
> Honestly, at this point you might as well just delete the defines, and
> opencode at their single usage site.  They're pure obfuscation like
> this, given no statement of units / behaviour, etc.
Ok I will do in v2.

> That said, we do need to stea^W borrow adaptive PLE, and make the
I cannot understand what do you mean by "stea^W borrow adaptive PLE".

> settings hvm-common because right now we've got two different ways of
> configuring the same thing for VT-x and SVM.  (This is definitely not
> cleanup work.  Just an observation for anyone feeling at a loose end.)
I will create hvm_function_table stubs for the functions that are used 
in common code, do the same thing but are implemented differently by svm 
and vtx, as part of the series aiming to make svm/vmx support configurable.
> ~Andrew
--
Xenia