This patch series attempts a cleanup of files {svm,vmx} files and headers
by removing redundant headers and sorting the rest, reducing the scope of
declarations and definitions, moving functions used only by internal {svm,vmx}
code to private headers, fix coding style, replace u* with uint*_t types etc.

This series aims to address the comments made on v2.

Main changes from the v2 series:
  - move all internal svm/vmx declarations found in external headers into
    private headers
  - add SPDX tags and guards to the new headers
  - take the opportunity to fix coding style issues and rearrange the code
    per Jan's suggestion
  - replace u* with uint*_t
  - rebased to the latest staging

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
  x86/svm: make emulate.h private
  x86/svm: move nestedsvm declarations used only by svm code to private
    header
  x86/svm: move vmcb declarations used only by svm code to private
    header
  x86/svm: move svmdebug.h declarations to private vmcb.h and delete it
  x86/vmx: move vmx_update_debug_state() in vmcs.c and declare it static
  x86/vmx: remove unused included headers from vmx.h
  x86/vmx: move declarations used only by vmx code from vmx.h to private
    headers
  x86/vmx: remove unused included headers from vmx.c
  x86/vmx: declare nvmx_enqueue_n2_exceptions() static
  x86/vmx: move vvmx declarations used only by vmx code to private
    header
  x86/vmx: move vmcs declarations used only by vmx code to private
    header

 xen/arch/x86/hvm/svm/asid.c                   |   4 +-
 xen/arch/x86/hvm/svm/asid.h                   |  38 ++
 xen/arch/x86/hvm/svm/emulate.c                |   4 +-
 .../x86/{include/asm => }/hvm/svm/emulate.h   |  20 +-
 xen/arch/x86/hvm/svm/intr.c                   |   4 +-
 xen/arch/x86/hvm/svm/nestedhvm.h              |  77 +++
 xen/arch/x86/hvm/svm/nestedsvm.c              |   7 +-
 xen/arch/x86/hvm/svm/svm.c                    |  10 +-
 xen/arch/x86/hvm/svm/svm.h                    |  62 ++
 xen/arch/x86/hvm/svm/svmdebug.c               |   3 +-
 xen/arch/x86/hvm/svm/vmcb.c                   |   3 +-
 xen/arch/x86/hvm/svm/vmcb.h                   | 597 ++++++++++++++++++
 xen/arch/x86/hvm/vmx/intr.c                   |   4 +
 xen/arch/x86/hvm/vmx/pi.h                     |  78 +++
 xen/arch/x86/hvm/vmx/realmode.c               |   2 +
 xen/arch/x86/hvm/vmx/vmcs.c                   |  17 +-
 xen/arch/x86/hvm/vmx/vmcs.h                   | 100 +++
 xen/arch/x86/hvm/vmx/vmx.c                    |  76 +--
 xen/arch/x86/hvm/vmx/vmx.h                    | 416 ++++++++++++
 xen/arch/x86/hvm/vmx/vvmx.c                   |   4 +
 xen/arch/x86/hvm/vmx/vvmx.h                   | 187 ++++++
 xen/arch/x86/include/asm/hvm/svm/asid.h       |  49 --
 xen/arch/x86/include/asm/hvm/svm/intr.h       |  25 -
 xen/arch/x86/include/asm/hvm/svm/nestedsvm.h  |  53 +-
 xen/arch/x86/include/asm/hvm/svm/svm.h        |  41 --
 xen/arch/x86/include/asm/hvm/svm/svmdebug.h   |  30 -
 xen/arch/x86/include/asm/hvm/svm/vmcb.h       | 575 +----------------
 xen/arch/x86/include/asm/hvm/vmx/vmcs.h       | 118 +---
 xen/arch/x86/include/asm/hvm/vmx/vmx.h        | 453 +------------
 xen/arch/x86/include/asm/hvm/vmx/vvmx.h       | 165 +----
 30 files changed, 1685 insertions(+), 1537 deletions(-)
 create mode 100644 xen/arch/x86/hvm/svm/asid.h
 rename xen/arch/x86/{include/asm => }/hvm/svm/emulate.h (73%)
 create mode 100644 xen/arch/x86/hvm/svm/nestedhvm.h
 create mode 100644 xen/arch/x86/hvm/svm/svm.h
 create mode 100644 xen/arch/x86/hvm/svm/vmcb.h
 create mode 100644 xen/arch/x86/hvm/vmx/pi.h
 create mode 100644 xen/arch/x86/hvm/vmx/vmcs.h
 create mode 100644 xen/arch/x86/hvm/vmx/vmx.h
 create mode 100644 xen/arch/x86/hvm/vmx/vvmx.h
 delete mode 100644 xen/arch/x86/include/asm/hvm/svm/asid.h
 delete mode 100644 xen/arch/x86/include/asm/hvm/svm/intr.h
 delete mode 100644 xen/arch/x86/include/asm/hvm/svm/svmdebug.h

-- 
2.37.2

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.

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.

~Andrew

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.

> 
> ~Andrew

-- 
Xenia

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,

~Andrew

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? And it
also doesn't include it right now. Since nested is still far from
being properly functional, and since most guests don't use it, I'd
rather see struct nestedvcpu's union to become a union of two
pointers, which get space allocated only for nested-enabled guests.
Yet it's only the use there which makes it necessary to globally
expose the struct right now.

Jan

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.

nestedsvm is literally just one struct now, and no subsystem ought to
have multiple headers when one will do.

The resulting "unified" svm.h will have very little in it, but
everything in it (including nestedsvm) should be there.

~Andrew

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

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

Jan

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

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.

Jan

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?

> 
> Jan

-- 
Xenia

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.

Jan

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

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