Hi, Initial patch sent out to xen-devel: http://lists.xen.org/archives/html/xen-devel/2012-10/msg00567.html Feedback from Jan Beulich: http://lists.xen.org/archives/html/xen-devel/2012-10/msg02135.html This patch integrates feedback from Jan Beulich and did some additional cleanup. Run tests on Intel machines. Works as expected. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
>>> On 27.03.13 at 14:41, Egger Christoph <chegger@amazon.de> wrote:Didn''t you also require a hypervisor side change for>+#define MC4_type_MISC1 0x4 >+#define MC4_type_MISC2 0x5 >+#define MC4_type_MISC3 0x6which also gets me back to the previously asked question why this is done only for bank 4.>- sprintf(path, "/local/domain/%d/memory/target", domid); >+ snprintf(path, sizeof(path), "/local/domain/%d/memory/target", domid);This continues to be valid, but unrelated.>- int type = MCE_SRAO_MEM; >+ int type; >... >+ if (cpu_vendor == CPU_VENDOR_AMD) >+ type = AMD_MCE_MEM; >+ if (cpu_vendor == CPU_VENDOR_INTEL) >+ type = INTEL_MCE_SRAO_MEM;still leaves type uninitialized for the non-Intel, non-AMD case. And some compilers aren''t going to be able to figure out that the variable only gets used for either of these two cases, and will raise a warning. Jan
On 28.03.13 12:44, Jan Beulich wrote:>>>> On 27.03.13 at 14:41, Egger Christoph <chegger@amazon.de> wrote: > > Didn''t you also require a hypervisor side change for > >> +#define MC4_type_MISC1 0x4 >> +#define MC4_type_MISC2 0x5 >> +#define MC4_type_MISC3 0x6 > > which also gets me back to the previously asked question why > this is done only for bank 4.These MSRs only exist on bank 4.>> - sprintf(path, "/local/domain/%d/memory/target", domid); >> + snprintf(path, sizeof(path), "/local/domain/%d/memory/target", domid); > > This continues to be valid, but unrelated. > >> - int type = MCE_SRAO_MEM; >> + int type; >> ... >> + if (cpu_vendor == CPU_VENDOR_AMD) >> + type = AMD_MCE_MEM; >> + if (cpu_vendor == CPU_VENDOR_INTEL) >> + type = INTEL_MCE_SRAO_MEM; > > still leaves type uninitialized for the non-Intel, non-AMD case. And > some compilers aren''t going to be able to figure out that the > variable only gets used for either of these two cases, and will raise > a warning.I haven''t seen any warning but ok, will fix. Christoph
>>> On 28.03.13 at 14:23, Christoph Egger <chegger@amazon.de> wrote: > On 28.03.13 12:44, Jan Beulich wrote: >>>>> On 27.03.13 at 14:41, Egger Christoph <chegger@amazon.de> wrote: >> >> Didn''t you also require a hypervisor side change forYou didn''t answer this one.>>> +#define MC4_type_MISC1 0x4 >>> +#define MC4_type_MISC2 0x5 >>> +#define MC4_type_MISC3 0x6 >> >> which also gets me back to the previously asked question why >> this is done only for bank 4. > > These MSRs only exist on bank 4.Sure, but it still looks pretty arbitrary. Anyway, the comment wasn''t meant to be a NAK of any kind. Jan
On 28.03.13 14:58, Jan Beulich wrote:>>>> On 28.03.13 at 14:23, Christoph Egger <chegger@amazon.de> wrote: >> On 28.03.13 12:44, Jan Beulich wrote: >>>>>> On 27.03.13 at 14:41, Egger Christoph <chegger@amazon.de> wrote: >>> >>> Didn''t you also require a hypervisor side change for > > You didn''t answer this one.Yes, this is right for AMD but not for Intel. I will submit it once I got the permission. Christoph>>>> +#define MC4_type_MISC1 0x4 >>>> +#define MC4_type_MISC2 0x5 >>>> +#define MC4_type_MISC3 0x6 >>> >>> which also gets me back to the previously asked question why >>> this is done only for bank 4. >> >> These MSRs only exist on bank 4. > > Sure, but it still looks pretty arbitrary. Anyway, the comment > wasn''t meant to be a NAK of any kind. > > Jan >
On 28.03.13 15:11, Christoph Egger wrote:> On 28.03.13 14:58, Jan Beulich wrote: >>>>> On 28.03.13 at 14:23, Christoph Egger <chegger@amazon.de> wrote: >>> On 28.03.13 12:44, Jan Beulich wrote: >>>>>>> On 27.03.13 at 14:41, Egger Christoph <chegger@amazon.de> wrote: >>>> >>>> Didn''t you also require a hypervisor side change for >> >> You didn''t answer this one. > > Yes, this is right for AMD but not for Intel.Jan, the patch is here: http://lists.xen.org/archives/html/xen-devel/2012-10/msg02010.html Please go ahead. Christoph> > Christoph > >>>>> +#define MC4_type_MISC1 0x4 >>>>> +#define MC4_type_MISC2 0x5 >>>>> +#define MC4_type_MISC3 0x6 >>>> >>>> which also gets me back to the previously asked question why >>>> this is done only for bank 4. >>> >>> These MSRs only exist on bank 4. >> >> Sure, but it still looks pretty arbitrary. Anyway, the comment >> wasn''t meant to be a NAK of any kind. >> >> Jan >> > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On 28.03.13 14:23, Christoph Egger wrote:> On 28.03.13 12:44, Jan Beulich wrote: >>>>> On 27.03.13 at 14:41, Egger Christoph <chegger@amazon.de> wrote: >> >> Didn''t you also require a hypervisor side change for >> >>> +#define MC4_type_MISC1 0x4 >>> +#define MC4_type_MISC2 0x5 >>> +#define MC4_type_MISC3 0x6 >> >> which also gets me back to the previously asked question why >> this is done only for bank 4. > > These MSRs only exist on bank 4. > >>> - sprintf(path, "/local/domain/%d/memory/target", domid); >>> + snprintf(path, sizeof(path), "/local/domain/%d/memory/target", >>> domid); >> >> This continues to be valid, but unrelated. >> >>> - int type = MCE_SRAO_MEM; >>> + int type; >>> ... >>> + if (cpu_vendor == CPU_VENDOR_AMD) >>> + type = AMD_MCE_MEM; >>> + if (cpu_vendor == CPU_VENDOR_INTEL) >>> + type = INTEL_MCE_SRAO_MEM; >> >> still leaves type uninitialized for the non-Intel, non-AMD case. And >> some compilers aren''t going to be able to figure out that the >> variable only gets used for either of these two cases, and will raise >> a warning. > > I haven''t seen any warning but ok, will fix.I am sending a new with this fixed. Christoph