Re: [RFC PATCH 02/22] x86/msr: implement MSR_SMI_COUNT for Dom0 on Intel

[Edwin Torok <edwin.torok@xxxxxxxxx>]

> On 31 Oct 2023, at 09:57, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 31.10.2023 10:42, Edwin Torok wrote:
>>> On 30 Oct 2023, at 16:20, Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>> On 25.10.2023 21:29, Edwin Török wrote:
>>>> Dom0 should always be able to read this MSR: it is useful when
>>>> investigating performance issues in production.
>>> 
>>> While I'm not outright opposed, I'm also not convinced. At the very least
>>> ...
>>> 
>>>> Although the count is Thread scoped, in practice all cores were observed
>>>> to return the same count (perhaps due to implementation details of SMM),
>>>> so do not require the cpu to be pinned in order to read it.
>>> 
>>> ... this, even if matching your observations, is going to be properly
>>> misleading in case counts end up diverging.
>>> 
>>>> This MSR exists on Intel since Nehalem.
>>>> 
>>>> Backport: 4.15+
>>> 
>>> If this was a backporting candidate, I think a Fixes: tag would need
>>> to indicate what's being fixed here.
>> 
>> 
>> I used the Backport tag to indicate what is the oldest release that it is 
>> backportable to.
>> IIRC the choices were:
>> * 4.0+ for issues that were present for a long time (didn't look further 
>> back than that in history), so there isn't any particular commit that 
>> introduces the problem, it was like that from the very beginning, i.e. not a 
>> regression.
>> 
>> * 4.13+ for issues affecting only new CPU support (I think that is the 
>> release that added Icelake support). I can attempt to find the commit that 
>> added Icelake support and mark them as Fixes: that commit (although there 
>> might be several of them)
>> 
>> * 4.15+ for bugs introduced by the default read-write msr changes
>> 
>> 
>>> Otherwise this is merely a new
>>> feature.
>>> 
>> 
>> Prior to the default rdwrmsr changes it was possible to read any MSR, so I 
>> consider it a bug that after the default rdwrmsr changes you can no longer 
>> do that, it takes away a valuable debugging tool.
> 
> As said elsewhere, making MSRs generally inaccessible was a deliberate change.
> I don't think any of us x86 maintainers has so far considered that as 
> introducing
> a bug. MSRs being accessible as a debugging tool may be worthwhile to have as 
> an
> optional feature (see my suggestion elsewhere as to a possible way to approach
> this), but I don't think this can be taken as an indication that we should 
> revert
> back to "blind" exposure.
> 


This particular patch (unlike the other one that is more general for all MSRs) 
is about one specific MSR, and makes it accessible to Dom0 only in a read-only 
way.
That is very different from blindly exposing it to all guests.
Anyway let me get something ready for master first, and then distributions that 
package Xen can decide whether to backport this patch or not, I'll replace 
Backport: 4.X with Backportable: 4.X (and add a Fixes: line) to indicate that 
this patch is recommended to be backported (for anyone who uses that version of 
Xen in production).

[I'm not saying that this patch is complete, as you've indicated there is more 
work required to make it work correctly wrt to pinning vs non-pinning, and I'll 
try to require a pinned Dom0 core in my next version]

Best regards,
--Edwin