Re: [PATCH v2 5/4] x86/hvm: Drop hvm_{get,set}_guest_bndcfgs() and use {get,set}_regs() instead

[Andrew Cooper <amc96@xxxxxxxx>]

On 19/01/2022 13:50, Jan Beulich wrote:
> On 17.01.2022 20:25, Andrew Cooper wrote:
>> hvm_{get,set}_guest_bndcfgs() are thin wrappers around accessing MSR_BNDCFGS.
>>
>> MPX was implemented on Skylake uarch CPUs and dropped in subsequent CPUs, and
>> is disabled by default in Xen VMs.
>>
>> It would be nice to move all the logic into vmx_msr_{read,write}_intercept(),
>> but the common HVM migration code uses guest_{rd,wr}msr().  Therefore, use
>> {get,set}_regs() to reduce the quantity of "common" HVM code.
>>
>> In lieu of having hvm_set_guest_bndcfgs() split out, use some #ifdef
>> CONFIG_HVM in guest_wrmsr().  In vmx_{get,set}_regs(), split the switch
>> statements into two depending on whether the require remote VMCS acquisition
>> or not.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> One remark:
>
>> @@ -323,10 +324,9 @@ int guest_rdmsr(struct vcpu *v, uint32_t msr, uint64_t 
>> *val)
>>          break;
>>  
>>      case MSR_IA32_BNDCFGS:
>> -        if ( !cp->feat.mpx || !is_hvm_domain(d) ||
>> -             !hvm_get_guest_bndcfgs(v, val) )
>> +        if ( !cp->feat.mpx ) /* Implies Intel HVM only */
> Wouldn't it make sense to accompany this comment by ...
>
>>              goto gp_fault;
>> -        break;
>     ASSERT(is_hvm_domain(d));
>
> (and then the same on the "set" path)?

So this is the reason for the default logic in the {get,set}_reg()
path.  The absence of MSR_BNDCFGS in the PV and SVM paths will cause the
VM to be crashed cleanly.  If you're on a VMX on a non-MPX capable
system, the VMREAD/VMWRITE will hit a BUG (which in due course I want to
downgrade to a domain crash).

It's a bit more friendly than an ASSERT() (doesn't take the system
down), is present in release builds too, and more precise as it excludes
SVM too.

~Andrew

P.S. I'm still trying to decide on an acceptable name to hide {
ASSERT_UNREACHABLE(); gprink(); domain_crash() } behind, so we can
downgrade more BUG()/etc to more runtime-friendly options.