On 23/07/2020 11:37, Jan Beulich wrote:
> On 22.07.2020 12:18, Andrew Cooper wrote:
>> --- a/xen/arch/x86/msr.c
>> +++ b/xen/arch/x86/msr.c
>> @@ -227,6 +227,14 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t
>> *val)
>> *val = msrs->misc_features_enables.raw;
>> break;
>>
>> + case MSR_IA32_MCG_CAP ... MSR_IA32_MCG_CTL: /* 0x179 -> 0x17b
>> */
>> + case MSR_IA32_MCx_CTL2(0) ... MSR_IA32_MCx_CTL2(31): /* 0x280 -> 0x29f
>> */
>> + case MSR_IA32_MCx_CTL(0) ... MSR_IA32_MCx_MISC(31): /* 0x400 -> 0x47f
>> */
>> + case MSR_IA32_MCG_EXT_CTL: /* 0x4d0 */
>> + if ( vmce_rdmsr(msr, val) < 0 )
>> + goto gp_fault;
>> + break;
>> +
>> case MSR_X2APIC_FIRST ... MSR_X2APIC_LAST:
>> if ( !is_hvm_domain(d) || v != curr )
>> goto gp_fault;
>> @@ -436,6 +444,14 @@ int guest_wrmsr(struct vcpu *v, uint32_t msr, uint64_t
>> val)
>> break;
>> }
>>
>> + case MSR_IA32_MCG_CAP ... MSR_IA32_MCG_CTL: /* 0x179 -> 0x17b
>> */
>> + case MSR_IA32_MCx_CTL2(0) ... MSR_IA32_MCx_CTL2(31): /* 0x280 -> 0x29f
>> */
>> + case MSR_IA32_MCx_CTL(0) ... MSR_IA32_MCx_MISC(31): /* 0x400 -> 0x47f
>> */
>> + case MSR_IA32_MCG_EXT_CTL: /* 0x4d0 */
>> + if ( vmce_wrmsr(msr, val) < 0 )
>> + goto gp_fault;
>> + break;
>> +
>> case MSR_X2APIC_FIRST ... MSR_X2APIC_LAST:
>> if ( !is_hvm_domain(d) || v != curr )
>> goto gp_fault;
> With this the two functions also possibly returning 0 or 1 becomes
> meaningless. Would you think you can make then return bool at this
> occasion, or would you prefer to leave this to whenever someone
> gets to clean up this resulting anomaly? (I'm fine either way, but
> would prefer to not see the then meaningless tristate return values
> left in place.)
The entire internals of vmce_{wr,rd}msr() need an overhaul.
I tried switching them to use X86EMUL_* (at which point they will match
all the other subsystems we hand off MSR blocks to), but that quickly
turned into a larger mess than I have time for right now. I've still
got the partial work so far, and will finish it at some point.
~Andrew
