Re: Re: [PATCH 0/3] Virtual NMI

[Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>]

>Patches 1 and 2 want merging.  They're both enumerations and
>configuration bits, although the very first hunk of patch 1 (the P())
>wants delaying until the final patch; we shouldn't print out the
>capability until it's being used.
>
>The patch subjects want to be:
>
>    x86/svm: Enumerations for virtual NMI
>and
>    x86/svm: Use virtual NMI when available
Ack.
I Will merge these in V2.
I will move (the P()) hunk to patch 2.

>
>
>Everything here is local to SVM.  Notably there should be no edits to
>hvm.c or hvm.h.  By introducing hvm_intblk_vnmi, you break NMI injection
>in other cases.  vNMI is just a hardware-optimised way of handling the
>hvm_intblk_nmi_iret case.
The handling for the introduced hvm_intblk_vnmi is separate from the other
interrupts handling. I will remove the introduced hvm_intblk_vnmi and I will
rather use the existing hvm_intblk_nmi_iret. This should have the side effect
of setting the VMCB's interrupt-shadow. However, it should be benign side
effect as this should not currently cause an actual interrupt shadow on the
AMD platforms prior to the vNMI support.

>
>
>
>svm_inject_nmi() wants to gain a check to see whether vNMI is enabled,
>and in the case that it is, simply set vnmi_pending.  You have this
>partially, but it needs to be dependent on the VMCB vNMI setting, not
>some global idea of enablement.
>
I will add one more is_vnmi_enabled callback for the check functionality and I
will use that introduced check in the svm_inject_nmi

>svm_get_interrupt_shadow() needs a similar adjustment to read
>vnmi_blocked rather than unconditionally depending on INTERCEPT_IRET.
>
Ack, I will add the check in the V2

>In construct_vmcb(), you need to check cpu_has_svm_vnmi.  I think this
>change is simple enough to be enabled unconditionally.  (We'll need to
>change this in due course, but that's going to take other infrastructure
>which we don't have yet.)
>
Ack, I will change it, as suggested, in the V2

>
>A couple of other minor notes:
>
>In the vintr_t union, use an anonymous 3 bit field (literally "u64 :3;",
>which is valid syntax) instead of renumbering the rsvd$N fields.  That
>will shrink the diff.
>
Ack, I will change it, as suggested, in the V2

>Xen's style has spaces inside the outermost brackets for control
>structures, and {'s on new lines.  For the functions you're modifying,
>just copy the surrounding style.
>
Ack, I will stick to the formatting guideline in the V2

--Abdelkareem