Re: [Xen-devel] [PATCH v2] SVM: Add union intstat_t for offset 68h in vmcb struct

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Pu Wen <puwen@hygon.cn>, <xen-devel@lists.xenproject.org>
Cc: "Wei Liu" <wl@xen.org>, "Jan Beulich" <jbeulich@suse.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v2] SVM: Add union intstat_t for offset 68h in vmcb struct
Date: Wed, 25 Mar 2020 15:56:06 +0000	[thread overview]
Message-ID: <b0200562-dea5-2ba0-a7b2-2663a199c640@citrix.com> (raw)
In-Reply-To: <ee018b0a-6b92-4e87-1d22-c8839393f800@hygon.cn>

On 25/03/2020 15:22, Pu Wen wrote:
> On 2020/3/24 20:28, Andrew Cooper wrote:
>> Hmm - this field doesn't appear to be part of AVIC, which makes me
>> wonder what we're doing without it.
>>
>> It appears to be a shadow copy of EFLAGS.IF which is only written on
>> vmexit, and never consumed, but this is based on Appendix B which is the
>> only reference I can find to the field at all.  Neither the
>> VMRUN/#VMEXIT descriptions discuss it at all.
>>
>> Given its position next to the (ambiguous) INTERRUPT_SHADOW, it just
>> might actually distinguish the STI shadow from the MovSS shadow, but it
>> could only do that by not behaving as described, and being asymmetric
>> with EFLAGS.  I don't have time to investigate this right now.
>>
>> We need the field described in Xen to set it appropriately for virtual
>> vmexit, but I think that is the extent of what we need to do.
> We encountered problem while running xen with new firmware which
> implement the bit[1] of the VMCB offset 68h: the DomU stopped when
> running seabios. We debugged the seabios code and found that the
> seabios hung after call16_int10().
>
> Then we debugged the xen code, and found the cause at this place in
> svm_get_interrupt_shadow():
>     if ( vmcb->interrupt_shadow )
>          intr_shadow |= HVM_INTR_SHADOW_MOV_SS | HVM_INTR_SHADOW_STI;
> the conditional is true if bit[1] is 1 even though bit[0] is zero.
> If just only use bit[0] in the conditional, the problem is solved, DomU
> will run successfully.

Oh - now you point this out, the issue is obvious.

The above content would make a far more informative commit message.  How
about extending the middle paragraph with:

"...part of interrupt_shadow, causing svm_get_interrupt_shadow() to
mistake the guest having interrupts enabled as being in an interrupt
shadow.  This has been observed to cause SeaBIOS to hang on boot."

or words to that effect.  The "it definitely breaks a guest" is the most
important piece of information here.

Do you happen to know call16_int10() was doing, exactly?  We've
presumably trapped for emulation to be using svm_get_interrupt_shadow()
in the first place.

~Andrew