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

[Wen Pu <puwen@xxxxxxxx>]

On 2020/3/25 23:56, Andrew Cooper wrote:
> 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.

Thanks for the suggestion, will add it to the patch description.

> 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.

call16_int10() is used to set VGA mode and we see no trap operation in log.
We suspected there is something wrong in the interrupt handling process,
after we changed to use interrupt_shadow bit, we found the problem is solved.

-- 
Regards,
Pu Wen