Hi, Attached patch implements the VMCB cleanbits SVM feature. Upcoming AMD CPUs introduce them and they are basically hints for the CPU which vmcb values can be re-used from the previous VMRUN instruction. Each bit represents a certain set of fields in the VMCB. Setting a bit tells the cpu it can re-use the cached value from the previous VMRUN. Clearing a bit tells the cpu to reload the values from the given VMCB. Signed-off-by: Wei Huang <Wei.Huang2@amd.com> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi, Thanks for the patch! At 10:52 +0000 on 15 Dec (1292410374), Christoph Egger wrote:> @@ -660,16 +671,17 @@ static void svm_ctxt_switch_to(struct vc > > static void svm_do_resume(struct vcpu *v) > { > + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > bool_t debug_state = v->domain->debugger_attached; > > if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) ) > { > - uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3); > v->arch.hvm_vcpu.debug_state_latch = debug_state; > if ( debug_state ) > - v->arch.hvm_svm.vmcb->exception_intercepts |= mask; > - else > - v->arch.hvm_svm.vmcb->exception_intercepts &= ~mask; > + { > + svm_set_exception_intercept(vmcb, TRAP_debug); > + svm_set_exception_intercept(vmcb, TRAP_int3); > + }This seems to change the logic so it doesn''t clear the intercepts if debug_state == 0. Is that OK? More generally, I''m not sure I like having all the VMCB accessor functions in files called "cleanbits" -- wouldn''t it make sense to have all that in the vmcb files so people will see them and know to use them? You could rename the actual vmcb fields as well to catch anyone writing them directly, e.g. in forward-ported patches. Cheers, Tim. -- Tim Deegan <Tim.Deegan@citrix.com> Principal Software Engineer, Xen Platform Team Citrix Systems UK Ltd. (Company #02937203, SL9 0BG) _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Wednesday 15 December 2010 12:27:51 Tim Deegan wrote:> Hi, > > Thanks for the patch! > > At 10:52 +0000 on 15 Dec (1292410374), Christoph Egger wrote: > > @@ -660,16 +671,17 @@ static void svm_ctxt_switch_to(struct vc > > > > static void svm_do_resume(struct vcpu *v) > > { > > + struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb; > > bool_t debug_state = v->domain->debugger_attached; > > > > if ( unlikely(v->arch.hvm_vcpu.debug_state_latch != debug_state) ) > > { > > - uint32_t mask = (1U << TRAP_debug) | (1U << TRAP_int3); > > v->arch.hvm_vcpu.debug_state_latch = debug_state; > > if ( debug_state ) > > - v->arch.hvm_svm.vmcb->exception_intercepts |= mask; > > - else > > - v->arch.hvm_svm.vmcb->exception_intercepts &= ~mask; > > + { > > + svm_set_exception_intercept(vmcb, TRAP_debug); > > + svm_set_exception_intercept(vmcb, TRAP_int3); > > + } > > This seems to change the logic so it doesn''t clear the intercepts if > debug_state == 0. Is that OK?No, that''s not ok. I fixed that in the new patch.> More generally, I''m not sure I like having all the VMCB accessor > functions in files called "cleanbits" -- wouldn''t it make sense to have > all that in the vmcb files so people will see them and know to use them? > You could rename the actual vmcb fields as well to catch anyone writing > them directly, e.g. in forward-ported patches.I renamed the ''svmcleanbits.[ch]'' files to ''vmc_funcs.[ch]'' Thanks for your review. Signed-off-by: Wei Huang <Wei.Huang2@amd.com> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 15/12/2010 12:36, "Christoph Egger" <Christoph.Egger@amd.com> wrote:> On Wednesday 15 December 2010 12:27:51 Tim Deegan wrote: >> This seems to change the logic so it doesn''t clear the intercepts if >> debug_state == 0. Is that OK? > > No, that''s not ok. I fixed that in the new patch. > >> More generally, I''m not sure I like having all the VMCB accessor >> functions in files called "cleanbits" -- wouldn''t it make sense to have >> all that in the vmcb files so people will see them and know to use them? >> You could rename the actual vmcb fields as well to catch anyone writing >> them directly, e.g. in forward-ported patches. > > I renamed the ''svmcleanbits.[ch]'' files to ''vmc_funcs.[ch]'' > > Thanks for your review.I went through this patch quite brutally when I applied it (c/s 22546). In particular I made the VMCB field accessor functions more consistent in name and semantics, and pulled out their implementations into a common macro to make the code clearer. There should be no significant changes compared with your patch *EXCEPT*: 1. Updates to the MSR and I/O bitmaps do not affect clear bits 2. Updates to lbr_control.enable do not affect clear bits 3. Updates to debugctlmsr *do* affect clear bits In the above I am following what is described in AMD Volume 2 Section 15.15.3 "VMCB Clean Field". I note that the MSRPM_BASE and IOPM_BASE fields are listed as cacheable, but *no* mention is made of caching the bitmap contents. Also, bit 10 (LBR) has debugctlmsr listed as cacheable, but again *no* mention is made of the lbr_control.enable bit flag. If any of the above is wrong, then: (a) the reference manual should be fixed; (b) I would accept a fixup patch, with a patch description explaininbg why behaviour is deviating from cleanbits behaviour describved in the latest version of the AMD reference manuals. -- Keir> Signed-off-by: Wei Huang <Wei.Huang2@amd.com> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> > > Christoph >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Hi Keir, Thanks for putting up this patch. I think the comments you made are correct after reading the spec again. Christoph and I misread some APM content. :-( Thanks again, -Wei -----Original Message----- From: xen-devel-bounces@lists.xensource.com [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser Sent: Wednesday, December 15, 2010 10:56 AM To: Egger, Christoph; Tim Deegan Cc: xen-devel@lists.xensource.com Subject: Re: [Xen-devel] [PATCH] svm: support VMCB cleanbits On 15/12/2010 12:36, "Christoph Egger" <Christoph.Egger@amd.com> wrote:> On Wednesday 15 December 2010 12:27:51 Tim Deegan wrote: >> This seems to change the logic so it doesn''t clear the intercepts if >> debug_state == 0. Is that OK? > > No, that''s not ok. I fixed that in the new patch. > >> More generally, I''m not sure I like having all the VMCB accessor >> functions in files called "cleanbits" -- wouldn''t it make sense to have >> all that in the vmcb files so people will see them and know to use them? >> You could rename the actual vmcb fields as well to catch anyone writing >> them directly, e.g. in forward-ported patches. > > I renamed the ''svmcleanbits.[ch]'' files to ''vmc_funcs.[ch]'' > > Thanks for your review.I went through this patch quite brutally when I applied it (c/s 22546). In particular I made the VMCB field accessor functions more consistent in name and semantics, and pulled out their implementations into a common macro to make the code clearer. There should be no significant changes compared with your patch *EXCEPT*: 1. Updates to the MSR and I/O bitmaps do not affect clear bits 2. Updates to lbr_control.enable do not affect clear bits 3. Updates to debugctlmsr *do* affect clear bits In the above I am following what is described in AMD Volume 2 Section 15.15.3 "VMCB Clean Field". I note that the MSRPM_BASE and IOPM_BASE fields are listed as cacheable, but *no* mention is made of caching the bitmap contents. Also, bit 10 (LBR) has debugctlmsr listed as cacheable, but again *no* mention is made of the lbr_control.enable bit flag. If any of the above is wrong, then: (a) the reference manual should be fixed; (b) I would accept a fixup patch, with a patch description explaininbg why behaviour is deviating from cleanbits behaviour describved in the latest version of the AMD reference manuals. -- Keir> Signed-off-by: Wei Huang <Wei.Huang2@amd.com> > Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> > > Christoph >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 15/12/2010 23:04, "Huang2, Wei" <Wei.Huang2@amd.com> wrote:> Hi Keir, > > Thanks for putting up this patch. I think the comments you made are correct > after reading the spec again. Christoph and I misread some APM content. :-(No problem then. It would be good to know that the applied patch works and with the same performance win as you saw with your original patch. -- Keir> Thanks again, > -Wei > > -----Original Message----- > From: xen-devel-bounces@lists.xensource.com > [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of Keir Fraser > Sent: Wednesday, December 15, 2010 10:56 AM > To: Egger, Christoph; Tim Deegan > Cc: xen-devel@lists.xensource.com > Subject: Re: [Xen-devel] [PATCH] svm: support VMCB cleanbits > > On 15/12/2010 12:36, "Christoph Egger" <Christoph.Egger@amd.com> wrote: > >> On Wednesday 15 December 2010 12:27:51 Tim Deegan wrote: >>> This seems to change the logic so it doesn''t clear the intercepts if >>> debug_state == 0. Is that OK? >> >> No, that''s not ok. I fixed that in the new patch. >> >>> More generally, I''m not sure I like having all the VMCB accessor >>> functions in files called "cleanbits" -- wouldn''t it make sense to have >>> all that in the vmcb files so people will see them and know to use them? >>> You could rename the actual vmcb fields as well to catch anyone writing >>> them directly, e.g. in forward-ported patches. >> >> I renamed the ''svmcleanbits.[ch]'' files to ''vmc_funcs.[ch]'' >> >> Thanks for your review. > > I went through this patch quite brutally when I applied it (c/s 22546). In > particular I made the VMCB field accessor functions more consistent in name > and semantics, and pulled out their implementations into a common macro to > make the code clearer. > > There should be no significant changes compared with your patch *EXCEPT*: > 1. Updates to the MSR and I/O bitmaps do not affect clear bits > 2. Updates to lbr_control.enable do not affect clear bits > 3. Updates to debugctlmsr *do* affect clear bits > > In the above I am following what is described in AMD Volume 2 Section > 15.15.3 "VMCB Clean Field". > > I note that the MSRPM_BASE and IOPM_BASE fields are listed as cacheable, but > *no* mention is made of caching the bitmap contents. > > Also, bit 10 (LBR) has debugctlmsr listed as cacheable, but again *no* > mention is made of the lbr_control.enable bit flag. > > If any of the above is wrong, then: (a) the reference manual should be > fixed; (b) I would accept a fixup patch, with a patch description > explaininbg why behaviour is deviating from cleanbits behaviour describved > in the latest version of the AMD reference manuals. > > -- Keir > >> Signed-off-by: Wei Huang <Wei.Huang2@amd.com> >> Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> >> >> Christoph >> > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Maybe Matching Threads
- [PATCH 10/14] Nested Virtualization: svm specific implementation
- [PATCH v6 01/76] KVM: SVM: nested: Don't allocate VMCB structures on stack
- [PATCH 1/1] svm: dump VMCB physical address
- [PATCH 5/5] SVM: Clear VMCB''s EFER.LME when guest disables paging
- [PATCH][SVM] Fix 32bit Windows guest VMs save/restore