Re: [PATCH v3 00/14] x86/hvm: {svm,vmx} {c,h} cleanup

[Xenia Ragiadakou <burzalodowa@xxxxxxxxx>]

On 2/24/23 23:33, Andrew Cooper wrote:
> On 24/02/2023 8:08 pm, Xenia Ragiadakou wrote:
> > On 2/24/23 21:29, Andrew Cooper wrote:
> > > On 24/02/2023 6:49 pm, Xenia Ragiadakou wrote:
> > > > There are more detailed per-patch changesets.
> > > >
> > > > Xenia Ragiadakou (14):
> > > >     x86/svm: move declarations used only by svm code from svm.h to
> > > > private
> > > >       header
> > > >     x86/svm: make asid.h private
> > > >     x86/svm: delete header asm/hvm/svm/intr.h
> > > Thankyou for this work.  I've acked and committed patches 1 and 3.
> > >
> > > Note that you had one hunk in patch 5 (whitespace correction in
> > > svm_invlpga) that obviously should have been in patch 1, so I've
> > > moved it.
> > Thanks, I missed that  ...
> >
> > > Looking at asid.h, I still can't bare to keep it even in that state, so
> > > I've constructed an alternative which I'll email out in a moment.
> > I 'm also in favor of less headers.
> I've committed as far as the nestedhvm move.  At that point, I've sent
> out a small patch to try and help decouple later changes.
>
> But I think we want to change tact slightly at this point, so I'm not
> going to do any further tweaking on commit.
>
> Next, I think we want to rename asm/hvm/svm/svm.h to asm/hvm/svm.h,
> updating the non-SVM include paths as we go.  Probably best to
> chain-include the other svm/hvm/svm/*.h headers temporarily, so we've
> only got one tree-wide cleanup of the external include paths.
>
> Quick tangent - I will be making all of that cpu_has_svm_*
> infrastructure disappear by moving it into the normal CPUID handling,
> but I've not had sufficient time to finish that yet.
>
> Next, hvm/svm/nestedsvm.h can merge straight into hvm/svm.h, and
> disappear (after my decoupling patch has gone in).
>
> After that, hvm/svm/vmcb.h can be cleanly split in half.  struct
> svm_{domain,vcpu} can move sideways into hvm/svm.h (with a forward
> declaration of vmcb_struct), as can the svm msr intercept declarations.
> Everything else can move to being a private vmcb.h
>
> Finally your svmdebug.h can come at the end, pretty much in the same
> shape it is now.  One thing to say is that right now, you've left enum
> vmcb_sync_state public, but it's type is already decoupled by virtue of
> struct svm_vcpu having a uint8_t field rather than an enum field.
>
> And I think that cleanly gets rid of the entire asm/hvm/svm/* dir, which
> is a great position to get to.
>
>
> Beyond that, you will want to clean up the hvm msr intercept handling as
> hvm_funcs, which I think will decouple the vpmu files from svm/vmx
> specifically, but that will want to be a separate series.
>
> How does this sound?
Thanks for the review. I think it sounds good.

The last part i.e the hvm msr intercept handling as hvm_funcs, I have it 
already I just didn't have the chance to send it yet (because the 
changes affect {svm,vmx}.h). I will rebase and send it on Monday for 
review to verify that we are on the same page.
> Thanks,
>
> ~Andrew
--
Xenia