| Message ID | 20240206012051.3564035-6-george.dunlap@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | AMD Nested Virt Preparation | expand |
On 06.02.2024 02:20, George Dunlap wrote: > Changeset ef3e8db8068 ("x86/hvm: Corrections and improvements to > unhandled vmexit logging") introduced a printk to the default path of > the switch statement in nestedsvm_check_intercepts(), complaining of > an unknown exit reason. > > Unfortunately, the "core" switch statement which is meant to handle > all vmexit reasons is in nsvm_vmcb_guest_intercepts_exitcode(); the > switch statement in nestedsvm_check_intercepts() is only meant to > superimpose on top of that some special-casing for how to interaction > between L1 and L0 vmexits. > > Remove the printk, and add a comment to prevent future confusion. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> I wonder if a Fixes: tag is warranted here. Jan
On Mon, Feb 19, 2024 at 11:56 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 06.02.2024 02:20, George Dunlap wrote: > > Changeset ef3e8db8068 ("x86/hvm: Corrections and improvements to > > unhandled vmexit logging") introduced a printk to the default path of > > the switch statement in nestedsvm_check_intercepts(), complaining of > > an unknown exit reason. > > > > Unfortunately, the "core" switch statement which is meant to handle > > all vmexit reasons is in nsvm_vmcb_guest_intercepts_exitcode(); the > > switch statement in nestedsvm_check_intercepts() is only meant to > > superimpose on top of that some special-casing for how to interaction > > between L1 and L0 vmexits. > > > > Remove the printk, and add a comment to prevent future confusion. > > > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > I wonder if a Fixes: tag is warranted here. Yes, I think probably so. -George
On 06/02/2024 1:20 am, George Dunlap wrote: > Changeset ef3e8db8068 ("x86/hvm: Corrections and improvements to > unhandled vmexit logging") introduced a printk to the default path of > the switch statement in nestedsvm_check_intercepts(), complaining of > an unknown exit reason. > > Unfortunately, the "core" switch statement which is meant to handle > all vmexit reasons is in nsvm_vmcb_guest_intercepts_exitcode(); the > switch statement in nestedsvm_check_intercepts() is only meant to > superimpose on top of that some special-casing for how to interaction > between L1 and L0 vmexits. > > Remove the printk, and add a comment to prevent future confusion. > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> Erm... The addition of this printk was very deliberate, to point out where security fixes are needed. It's not bogus in the slightest. It is an error for exit reasons to not be inspected for safety in this path. Please revert this patch. ~Andrew
On Tue, Feb 27, 2024 at 12:47 AM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 06/02/2024 1:20 am, George Dunlap wrote: > > Changeset ef3e8db8068 ("x86/hvm: Corrections and improvements to > > unhandled vmexit logging") introduced a printk to the default path of > > the switch statement in nestedsvm_check_intercepts(), complaining of > > an unknown exit reason. > > > > Unfortunately, the "core" switch statement which is meant to handle > > all vmexit reasons is in nsvm_vmcb_guest_intercepts_exitcode(); the > > switch statement in nestedsvm_check_intercepts() is only meant to > > superimpose on top of that some special-casing for how to interaction > > between L1 and L0 vmexits. > > > > Remove the printk, and add a comment to prevent future confusion. > > > > Signed-off-by: George Dunlap <george.dunlap@cloud.com> > > Erm... The addition of this printk was very deliberate, to point out > where security fixes are needed. > > It's not bogus in the slightest. It is an error for exit reasons to not > be inspected for safety in this path. I'm a bit at a loss how to respond to this. As I wrote above, exit reasons are inspected for safety in this path -- in nsvm_vmcb_guest_intercepts_exitcode(). If you want the patch reverted, you'll have to explain why that's not sufficient. -George
diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c index d02a59f184..a5319ab729 100644 --- a/xen/arch/x86/hvm/svm/nestedsvm.c +++ b/xen/arch/x86/hvm/svm/nestedsvm.c @@ -1292,6 +1292,10 @@ nestedsvm_check_intercepts(struct vcpu *v, struct cpu_user_regs *regs, ASSERT(vcpu_nestedhvm(v).nv_vmexit_pending == 0); is_intercepted = nsvm_vmcb_guest_intercepts_exitcode(v, regs, exitcode); + /* + * Handle specific interactions between things the guest and host + * may both want to intercept + */ switch ( exitcode ) { case VMEXIT_INVALID: @@ -1347,8 +1351,6 @@ nestedsvm_check_intercepts(struct vcpu *v, struct cpu_user_regs *regs, /* Always let the guest handle VMMCALL/VMCALL */ return NESTEDHVM_VMEXIT_INJECT; default: - gprintk(XENLOG_ERR, "Unexpected nested vmexit: reason %#"PRIx64"\n", - exitcode); break; }
Changeset ef3e8db8068 ("x86/hvm: Corrections and improvements to unhandled vmexit logging") introduced a printk to the default path of the switch statement in nestedsvm_check_intercepts(), complaining of an unknown exit reason. Unfortunately, the "core" switch statement which is meant to handle all vmexit reasons is in nsvm_vmcb_guest_intercepts_exitcode(); the switch statement in nestedsvm_check_intercepts() is only meant to superimpose on top of that some special-casing for how to interaction between L1 and L0 vmexits. Remove the printk, and add a comment to prevent future confusion. Signed-off-by: George Dunlap <george.dunlap@cloud.com> --- xen/arch/x86/hvm/svm/nestedsvm.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)