> -----Original Message----- > From: Andrew Cooper [mailto:andrew.coop...@citrix.com] > Sent: 09 April 2019 18:54 > To: Xen-devel <xen-de...@lists.xen.org> > Cc: Andrew Cooper <andrew.coop...@citrix.com>; Jan Beulich > <jbeul...@suse.com>; Wei Liu > <wei.l...@citrix.com>; Roger Pau Monne <roger....@citrix.com>; Paul Durrant > <paul.durr...@citrix.com>; > Jun Nakajima <jun.nakaj...@intel.com>; Kevin Tian <kevin.t...@intel.com> > Subject: [PATCH] x86/msr: Fix fallout from mostly c/s 832c180 > > The series 832c1803^..f61685a6 was committed without adequate review. >
v1 of the series was posted on 7th Jan and v4 on 14th March. It was committed yesterday. I'd say that there was certainly adequate time for review. > * Fix the shim build by providing a !CONFIG_HVM declaration for > hvm_get_guest_bndcfgs() > * Revert the bogus de-const'ing of the vcpu pointer in > vmx_get_guest_bndcfgs(). vmx_vmcs_enter() really does mutate the vcpu, and > may cause it to undergo a full de/reschedule, which is in violation of the > ABI described by hvm_get_guest_bndcfgs(). guest_rdmsr() was always going > to need to lose its const parameter, and this was the correct time for it > to happen. In the case of vmx_get_guest_bndcfgs() is there actually possibility of a re-schedule? It's either going to be in current context (in which case IIUC vmx_vmcs_enter() is going to be largely a no-op) or the vcpu in question should have already been paused. I'm no > * Remove the introduced ASSERT(is_hvm_domain(d)) and check the predicate > directly. While we expect it to be true, the result is potential type > confusion in release builds based on several subtle aspects of the CPUID > feature derivation logic with no other safety checks. This also fixes the > a linker error in the release build of the shim, again for !CONFIG_HVM > reasons. Again, digging back in mail... ----- [From Jan] > + case MSR_IA32_BNDCFGS: > + if ( !is_hvm_domain(d) || !cp->feat.mpx || > + !hvm_set_guest_bndcfgs(v, val) ) > + goto gp_fault; In both cases the is_hvm_*() check looks to be redundant, as for PV guests cp->feat.mpx can't be set. Personally I'd prefer this to be an ASSERT() instead, but I'd listen to Andrew (as the main author of this code) saying otherwise. ----- ...and I do recall asking for your opinion at the time. I guess you changed your mind. > * The MSRs in vcpu_msrs are in numeric order. Re-position XSS to match. > That was not at all obvious. If this is the case then there should be comment above the declaration of struct vcpu_msrs. > Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> However, the changes look ok to me and brings things closer to my earlier code anyway so, with the comment requested above added... Reviewed-by: Paul Durrant <paul.durr...@citrix.com> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
