Re: [Xen-devel] [PATCH 2/3] xen/mcelog: add PPIN to record when available

[Boris Ostrovsky]

On 11/13/19 8:44 AM, Jan Beulich wrote:
> On 13.11.2019 01:11, Boris Ostrovsky wrote:
>> On 11/11/19 9:46 AM, Jan Beulich wrote:
>>> --- a/arch/x86/include/asm/msr-index.h
>>> +++ b/arch/x86/include/asm/msr-index.h
>>> @@ -393,6 +393,8 @@
>>>  #define MSR_AMD_PSTATE_DEF_BASE		0xc0010064
>>>  #define MSR_AMD64_OSVW_ID_LENGTH	0xc0010140
>>>  #define MSR_AMD64_OSVW_STATUS		0xc0010141
>>> +#define MSR_AMD_PPIN_CTL		0xc00102f0
>>> +#define MSR_AMD_PPIN			0xc00102f1
>> Which processors are these defined for? I looked at a couple (fam 15h
>> and 17h) and didn't see those. And I don't see them in Linux.
> Certain Fam17 ones, Rome in particular (which is where I've
> tested this).

I was looking at Naples, can't find Rome on AMD's page.

>
>>> --- a/include/xen/interface/xen-mca.h
>>> +++ b/include/xen/interface/xen-mca.h
>>> @@ -332,7 +332,11 @@ struct xen_mc {
>>>  };
>>>  DEFINE_GUEST_HANDLE_STRUCT(xen_mc);
>>>  
>>> -/* Fields are zero when not available */
>>> +/*
>>> + * Fields are zero when not available. Also, this struct is shared with
>>> + * userspace mcelog and thus must keep existing fields at current offsets.
>>> + * Only add new fields to the end of the structure
>>> + */
>>>  struct xen_mce {
>> Why is this structure is part of the interface?
> That's a question to whoever put it there. There look to have
> been decisions (see also patch 1) to have the Linux clones of
> Xen's public headers deviate far more from their original
> than I would consider reasonable.

Yes, this goes back to when the file was first created.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>

(but if you want to move non-interface parts into, say,
drivers/xen/mcelog.h I won't stand in your way ;-) )