Hi! Attached patch cleans up apic msr-handling. This patch is extracted from the big msr patch. Signed-off-by: Christoph Egger <Christoph.Egger@amd.com> -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni 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 11.06.10 at 15:22, Christoph Egger <Christoph.Egger@amd.com> wrote: >-static inline void write_efer(u64 val) >-{ >- this_cpu(efer) = val; >- wrmsrl(MSR_EFER, val); >-} >+#define write_efer(val) do { \ >+ this_cpu(efer) = val; \ >+ wrmsrl(MSR_EFER, val); \ >+} while(0)This isn''t a good change imo: You now require all current and future users of write_efer() to not pass expressions with side effects. Also, is doesn''t really look like a cleanup to me, more like a complication. Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 11/06/2010 14:40, "Jan Beulich" <JBeulich@novell.com> wrote:>>>> On 11.06.10 at 15:22, Christoph Egger <Christoph.Egger@amd.com> wrote: >> -static inline void write_efer(u64 val) >> -{ >> - this_cpu(efer) = val; >> - wrmsrl(MSR_EFER, val); >> -} >> +#define write_efer(val) do { \ >> + this_cpu(efer) = val; \ >> + wrmsrl(MSR_EFER, val); \ >> +} while(0) > > This isn''t a good change imo: You now require all current and future > users of write_efer() to not pass expressions with side effects. > > Also, is doesn''t really look like a cleanup to me, more like a > complication.Hm, I didn''t spot that one. It''s odd since I thought Christoph was changign macros into inlien functions where possible. Maybe he just changes in whichever direction makes his patch bigger? ;-) -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Friday 11 June 2010 15:49:39 Keir Fraser wrote:> On 11/06/2010 14:40, "Jan Beulich" <JBeulich@novell.com> wrote: > >>>> On 11.06.10 at 15:22, Christoph Egger <Christoph.Egger@amd.com> wrote: > >> > >> -static inline void write_efer(u64 val) > >> -{ > >> - this_cpu(efer) = val; > >> - wrmsrl(MSR_EFER, val); > >> -} > >> +#define write_efer(val) do { \ > >> + this_cpu(efer) = val; \ > >> + wrmsrl(MSR_EFER, val); \ > >> +} while(0) > > > > This isn''t a good change imo: You now require all current and future > > users of write_efer() to not pass expressions with side effects. > > > > Also, is doesn''t really look like a cleanup to me, more like a > > complication. > > Hm, I didn''t spot that one. It''s odd since I thought Christoph was changign > macros into inlien functions where possible. Maybe he just changes in > whichever direction makes his patch bigger? ;-)No, this change has to do with ''smp_processor_id() undefined''. The root problem is a circular dependency with inclusion of headers. The right fix is to clean up the headers. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni 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 11/06/2010 16:51, "Christoph Egger" <Christoph.Egger@amd.com> wrote:>> Hm, I didn''t spot that one. It''s odd since I thought Christoph was changign >> macros into inlien functions where possible. Maybe he just changes in >> whichever direction makes his patch bigger? ;-) > > No, this change has to do with ''smp_processor_id() undefined''. > The root problem is a circular dependency with inclusion of headers. > The right fix is to clean up the headers.Yep, that''s what I did. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Friday 11 June 2010 17:57:58 Keir Fraser wrote:> On 11/06/2010 16:51, "Christoph Egger" <Christoph.Egger@amd.com> wrote: > >> Hm, I didn''t spot that one. It''s odd since I thought Christoph was > >> changign macros into inlien functions where possible. Maybe he just > >> changes in whichever direction makes his patch bigger? ;-) > > > > No, this change has to do with ''smp_processor_id() undefined''. > > The root problem is a circular dependency with inclusion of headers. > > The right fix is to clean up the headers. > > Yep, that''s what I did.Yes, I saw that in c/s 21605. Thanks for doing this. But this is not what I meant by ''clean up the headers''. For example, when you do touch xen/include/asm-x86/apic.h then in the next build files that have nothing to do with apic get recompiled because apic.h is indirectly included. Christoph -- ---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Andrew Bowd, Thomas M. McCoy, Giuliano Meroni 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 11/06/2010 17:10, "Christoph Egger" <Christoph.Egger@amd.com> wrote:> But this is not what I meant by ''clean up the headers''. > For example, when you do > > touch xen/include/asm-x86/apic.h > > then in the next build files that have nothing to do with > apic get recompiled because apic.h is indirectly included.Yeah, smp.h should not need to include apic.h. I fixed this up as xen-unstable:21608. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel