| Message ID | 20221028114913.88921-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | amd/virt_ssbd: refactoring and fixes | expand |
On 28.10.2022 13:49, Roger Pau Monne wrote: > Add MSR_VIRT_SPEC_CTRL to the list of MSRs handled by > hvm_load_cpu_msrs(), or else it would be lost. > > Fixes: 8ffd5496f4 ('amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- > I'm confused as to why we have two different list of MSR to send and > load, one in msrs_to_send[] and the other open-coded in > hvm_load_cpu_msrs(), but given the release status it's no time to > clean that up. I guess this is an optimization, as looking up the MSR in msrs_to_send[] would be an inefficient inner loop, with the result only used in a boolean manner (entry present or not present). I do think though that both places should have a comment referencing the respectively other one, so both will (hopefully) be updated together. The same looks to apply to arch_do_domctl()'s MSR handling, where I've screwed up for XFD{,_ERR}. I'm puzzled by the ctxt->msr[i]._rsvd checking in the default: case. It was me who added this 8.5 years ago, but I can't see the value of that check considering the check in the next to final loop in the function. Hmm, wait - this had an error checking purpose until commit f61685a66903 ("x86: remove defunct init/load/save_msr() hvm_funcs"). I think the check should have been removed at that point. That'll be a post-4.17 patch, I suppose ... Jan
Hi both, > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Subject: Re: [PATCH for-4.17 v2 1/3] hvm/msr: load VIRT_SPEC_CTRL > > On 28.10.2022 13:49, Roger Pau Monne wrote: > > Add MSR_VIRT_SPEC_CTRL to the list of MSRs handled by > > hvm_load_cpu_msrs(), or else it would be lost. > > > > Fixes: 8ffd5496f4 ('amd/msr: implement VIRT_SPEC_CTRL for HVM guests > on top of SPEC_CTRL') > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> Sorry for the late reply. Release-acked-by: Henry Wang <Henry.Wang@arm.com> Kind regards, Henry
On 28/10/2022 12:49, Roger Pau Monne wrote: > Add MSR_VIRT_SPEC_CTRL to the list of MSRs handled by > hvm_load_cpu_msrs(), or else it would be lost. > > Fixes: 8ffd5496f4 ('amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL') > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > I'm confused as to why we have two different list of MSR to send and > load, one in msrs_to_send[] and the other open-coded in > hvm_load_cpu_msrs(), but given the release status it's no time to > clean that up. It's necessary (for now). guest_wrmsr() started as only safe in current context. The conversion work (to make it safe in remote-but-paused context) is in progress. e.g. guest_wrmsr()'s call into vmce_wrmsr() will malfunction in non-current context. There are probably others (although I think most of problem went away when restructured the handlers.) The list is the list of MSRs is the subset known safe for remote writes. It should be dropped when guest_wrmsr() is fully safe. ~Andrew
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 44b432ec5a..15a9b34c59 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -1498,6 +1498,7 @@ static int cf_check hvm_load_cpu_msrs(struct domain *d, hvm_domain_context_t *h) case MSR_INTEL_MISC_FEATURES_ENABLES: case MSR_IA32_BNDCFGS: case MSR_IA32_XSS: + case MSR_VIRT_SPEC_CTRL: case MSR_AMD64_DR0_ADDRESS_MASK: case MSR_AMD64_DR1_ADDRESS_MASK ... MSR_AMD64_DR3_ADDRESS_MASK: rc = guest_wrmsr(v, ctxt->msr[i].index, ctxt->msr[i].val);
Add MSR_VIRT_SPEC_CTRL to the list of MSRs handled by hvm_load_cpu_msrs(), or else it would be lost. Fixes: 8ffd5496f4 ('amd/msr: implement VIRT_SPEC_CTRL for HVM guests on top of SPEC_CTRL') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- I'm confused as to why we have two different list of MSR to send and load, one in msrs_to_send[] and the other open-coded in hvm_load_cpu_msrs(), but given the release status it's no time to clean that up. --- Changes since v1: - New in this version. --- xen/arch/x86/hvm/hvm.c | 1 + 1 file changed, 1 insertion(+)