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 <jbeulich@suse.com>
---
We could go further here, removing the local "err" variable altogether,
by using "return -ENXIO". Thoughts.
--- 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;
}
}
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 <jbeulich@suse.com> > --- > 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
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 <jbeulich@suse.com> >> --- >> 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
© 2016 - 2024 Red Hat, Inc.