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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/02/2023 7:11 am, Jan Beulich wrote:
> On 28.02.2023 08:09, Xenia Ragiadakou wrote:
>> On 2/27/23 14:17, Jan Beulich wrote:
>>> On 27.02.2023 13:06, Andrew Cooper wrote:
>>>> 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.
>>> Which keeps growing, and sooner or later we'll need to find something
>>> again to split off, so we won't exceed a page in size. The nested
>>> structures would, to me, look to be prime candidates for such.
>>>
>>>>> 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.
>>> That would make it a build-time decision, but then on NESTED_VIRT=y
>>> hypervisors there might still be guests not meaning to use that
>>> functionality, and for quite some time that may actually be a majority.
>>>
>>>> Everything you've posed here is way out of scope for Xenia's series.
>>> There was never an intention to extend the scope of the work she's doing.
>>> Instead I was trying to limit the scope by suggesting to avoid a piece
>>> of rework which later may want undoing.
>> Can I suggest to leave hvm/svm/svm.h and hvm/svm/nestedsvm.h separate 
>> for now?
> As per before - that's my preference. It'll be Andrew who you would need
> to convince, as he did suggest the folding.

Please fold them.

I have strong doubts that they would actually be unfolded, even if we
did want to make nested virt a build time choice.

(I'm not actually convinced that the existing nestedvcpu structure will
survive first-pass design of a working nested virt solution.)

~Andrew