x86/HVM: drop stale check from hvm_load_cpu_msrs()

Commit Message

Jan Beulich Nov. 29, 2022, 2:51 p.m. UTC

Up until f61685a66903 ("x86: remove defunct init/load/save_msr()
hvm_funcs") the check of the _rsvd field served as an error check for
the earlier hvm_funcs.save_msr() invocation. With that invocation gone
the check makes no sense anymore. While dropping it also merge the two
paths setting "err" to -ENXIO.

Signed-off-by: Jan Beulich <[e-mail]>
---
We could go further here, removing the local "err" variable altogether,
by using "return -ENXIO". Thoughts.

Comments

Andrew Cooper Nov. 29, 2022, 8:36 p.m. UTC | #1

On 29/11/2022 14:51, Jan Beulich wrote:
> Up until f61685a66903 ("x86: remove defunct init/load/save_msr()
> hvm_funcs") the check of the _rsvd field served as an error check for
> the earlier hvm_funcs.save_msr() invocation. With that invocation gone
> the check makes no sense anymore. While dropping it also merge the two
> paths setting "err" to -ENXIO.
>
> Signed-off-by: Jan Beulich <[e-mail]>
> ---
> We could go further here, removing the local "err" variable altogether,
> by using "return -ENXIO". Thoughts.

'err' is a non-standard variable name, so yeah, why not.

That said, the current code has a split loop checking the incoming _rsvd
fields in a first pass, and then calling guest_wrmsr() on the second
pass.  This was also made pointless by the identified changeset, so the
two loops ought to be merged.

~Andrew

Jan Beulich Nov. 30, 2022, 7:39 a.m. UTC | #2

On 29.11.2022 21:36, Andrew Cooper wrote:
> On 29/11/2022 14:51, Jan Beulich wrote:
>> Up until f61685a66903 ("x86: remove defunct init/load/save_msr()
>> hvm_funcs") the check of the _rsvd field served as an error check for
>> the earlier hvm_funcs.save_msr() invocation. With that invocation gone
>> the check makes no sense anymore. While dropping it also merge the two
>> paths setting "err" to -ENXIO.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
>> ---
>> We could go further here, removing the local "err" variable altogether,
>> by using "return -ENXIO". Thoughts.
> 
> 'err' is a non-standard variable name, so yeah, why not.

Okay, I'll make a v2 for this.

> That said, the current code has a split loop checking the incoming _rsvd
> fields in a first pass, and then calling guest_wrmsr() on the second
> pass.  This was also made pointless by the identified changeset, so the
> two loops ought to be merged.

Not really, no - it would violate the "Checking finished" comment (but
of course we could also delete that one), but I'd also prefer to keep
checking for all errors we can check for early _before_ starting to
make any changes to the guest. Therefore if you really wanted that, I
guess you'd need to make a follow-on change yourself, with a convincing
justification (I wouldn't outright object to such a change, but I
probably also wouldn't ack it, leaving that to someone else).

Jan

Patch

--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1494,13 +1494,12 @@  static int cf_check hvm_load_cpu_msrs(st
         case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK:
             rc = guest_wrmsr(v, ctxt->msr[i].index, ctxt->msr[i].val);
 
-            if ( rc != X86EMUL_OKAY )
-                err = -ENXIO;
-            break;
+            if ( rc == X86EMUL_OKAY )
+                continue;
 
+            fallthrough;
         default:
-            if ( !ctxt->msr[i]._rsvd )
-                err = -ENXIO;
+            err = -ENXIO;
             break;
         }
     }