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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/02/2023 11:33 am, Jan Beulich wrote:
> On 27.02.2023 12:15, Andrew Cooper wrote:
>> On 27/02/2023 10:46 am, Jan Beulich wrote:
>>> On 24.02.2023 22:33, Andrew Cooper wrote:
>>>> 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).
>>> Why would you want to fold hvm/svm/nestedsvm.h into hvm/svm/svm.h?
>>> The latter doesn't use anything from the former, does it?
>> It's about what else uses them.
>>
>> hvm_vcpu needs both svm_vcpu and nestedsvm, so both headers are always
>> included in tandem.
> Well, yes, that's how things are today. But can you explain to me why
> hvm_vcpu has to know nestedsvm's layout?

Because it's part of the same single memory allocation.

> If the field was a pointer,
> a forward decl of that struct would suffice, and any entity in the
> rest of Xen not caring about struct nestedsvm would no longer see it
> (and hence also no longer be re-built if a change is made there).

Yes, you could hide it as a pointer.  The cost of doing so is an
unnecessary extra memory allocation, and extra pointer handling on
create/destroy, not to mention extra pointer chasing in memory during use.

>> nestedsvm is literally just one struct now, and no subsystem ought to
>> have multiple headers when one will do.
> When one will do, yes. Removing build dependencies is a good reason
> to have separate headers, though.

Its not the only only option, and an #ifdef CONFIG_NESTED_VIRT inside
the struct would be an equally acceptable way of doing this which
wouldn't involve making an extra memory allocation.


Everything you've posed here is way out of scope for Xenia's series. 
You are welcome to try and make the changes in the future if you want,
but you're going to have a uphill battle trying to justify it as a
sensible move.

~Andrew