Re: [PATCH for-4.14] x86/msr: Disallow access to Processor Trace MSRs

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.06.2020 19:16, Andrew Cooper wrote:
> On 19/06/2020 14:39, Jan Beulich wrote:
>> On 19.06.2020 15:28, Andrew Cooper wrote:
>>> On 19/06/2020 13:48, Jan Beulich wrote:
>>>> On 19.06.2020 13:58, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/msr.c
>>>>> +++ b/xen/arch/x86/msr.c
>>>>> @@ -168,6 +168,12 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, 
>>>>> uint64_t *val)
>>>>>      case MSR_TSX_FORCE_ABORT:
>>>>>      case MSR_TSX_CTRL:
>>>>>      case MSR_MCU_OPT_CTRL:
>>>>> +    case MSR_RTIT_OUTPUT_BASE:
>>>>> +    case MSR_RTIT_OUTPUT_MASK:
>>>>> +    case MSR_RTIT_CTL:
>>>>> +    case MSR_RTIT_STATUS:
>>>>> +    case MSR_RTIT_CR3_MATCH:
>>>>> +    case MSR_RTIT_ADDR_A(0) ... MSR_RTIT_ADDR_B(3):
>>>> The respective CPUID field is 3 bits wide, so wouldn't it be better
>>>> to cover the full possible range (0...6 afaict)?
>>> Last time I tried, you objected to me covering MSR ranges which weren't
>>> defined.
>> Wasn't that for a range where some number had been re-used from
>> earlier models (with entirely different purpose)?
> 
> I don't recall, but the answer isn't relevant to whether the MSRs at
> those indices ought to be available to guests.
> 
>>> If you want to extend the range like that, it ought to be
>>> MSR_RTIT_OUTPUT_BASE ... MSR_RTIT_ADDR_B(7) to cover the entire area
>>> which seems to be exclusively for PT.
>> I'd be okay with that, just that I'm not sure about (7): While
>> SDM Vol 2 oddly enough doesn't seem to list anything for leaf 7
>> subleaf 1 (or I'm sufficiently blind today), Vol 4 clearly says
>> for n=0...3 "If CPUID.(EAX=07H,ECX=1):EAX[2:0] > <n>", and the
>> field obviously can't be > 7.
> 
> 7 gives the top of the bank of MSRs.  It isn't related to CPUID data.

Well, okay - let's go that route then?

Jan