| Message ID | 20230224185010.3692754-1-burzalodowa@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86/hvm: {svm,vmx} {c,h} cleanup | expand |
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
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 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
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
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
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