Re: [PATCH]Fix the bug of guest os installation failure and win2k boot failure

[Keir Fraser <keir.fraser@eu.citrix.com>]

Hi Dongxiao,

Thanks for tracking this one down this far! I'm not fully convinced by your
analysis however, for the following reasons:

 1. In real mode we must surely have CPL==SS.DPL==0, otherwise the write of
CR0.PE would have been disallowed. Hence all I/O port operations must surely
succeed in our emulated real mode.

 2. I don't understand your changes to the shadow emulation path. Firstly,
the shadow emulator does not provide callback functions for I/O-port
operations, so they can never be emulated. Secondly, even if we do fall into
one of the generate_exception_if(!mode_iopl(), EXC_GP) statements, the
shadow emulator does not provide an inject_hw_exception() callback and hence
no exception will be generated and instead the emulation will (correctly)
fail.

This makes sense, since the nasty guest-installation problems have only
appeared (as far as I know) since the old I/O vmexit path was changed to use
x86_emulate(). Before that the 4-instruction shadow emulator and also the
real-mode emulator were working pretty much fine!

So... with regard to what is wrong with the I/O vmexit path. I agree that
the CPL-IOPL check is redundant, but it *should* work! Are we simply falling
down because we are not also checking the TSS bitmap?

Removing the IOPL checks from x86_emulate() may be the right thing to do,
but I would like to really understand the underlying root-cause problem
first.

 -- Keir

On 17/3/08 08:08, "Xu, Dongxiao" <dongxiao.xu@intel.com> wrote:

> Hi, Keir,
> 
>       This patch is to fix the problem of Linux guest installation failure and
> Windows 2000 boot failure.
> 
> ????? In the early code, we use vmx_vmexit_handler() -> vmx_io_instruction()
> function to emulate I/O instructions. But now, we use vmx_vmexit_handler() ->
> handle_mmio -> hvm_emulate_one() -> x86_emulate() to emulate I/O instructions.
> Also nowadays, the realmode emulation code walks through the path:
> vmx_realmode() -> realmode_emulate_one() -> hvm_emulate_one() ->
> x86_emulate().
> 
> ????? The I/O handle code in x86_emulate() checks the cpl and iopl value, and
> if cpl > iopl, it will generate a GP fault. This causes Linux guest
> installation failure and Windows 2000 boot failure. I think this check code
> may be not reasonable for two aspects:
> 
> ????? 1. If x86_emulate() is called from vmexit or from realmode emulation, I
> think this line of code is not needed, because:
> 
> ??????????? a). In I/O emulation, the cpu has already checked the cpl, iopl,
> and also the I/O bitmap before vmx_vmexit_handler() is called,
> ??????????? b). For realmode, we shouldn't check the cpl and iopl, because any
> I/O operation is permitted in realmode.
> 
> ????? 2. If x86_emulate() is called from multi.c, which emulates up to four
> instructions when dealing with PAE guest page tables. In this condition, the
> check is needed, but it is not correct, it should follow the code as follows,
> which is stated in the Intel SDM:
> 
> 　If (cpl <= iopl)
> 　??? Do I/O operation;
> 　Else {
> 　??? If (I/O permission bit for the port == 0)
> 　??????? Do I/O operation;
> 　??? Else 
> 　??????? Generate GP fault;
> 　} 
> 
> ????? Now this patch remove the cpl and iopl check in I/O handler code in
> x86_emulate() function. And it checks the four instructions which would be
> emulated by multi.c, if any of them is IN/INS/OUT/OUTS, or REP
> IN/INS/OUT/OUTS, we will break that four-instruction emulation, and let the
> I/O instruction walk through the path of vmx_vmexit_handler() -> handle_mmio
> -> hvm_emulate_one() -> x86_emulate().
> 
>       Another way to solve this issue could be that, we put the entire io
> permission check in x86_emulate(), and use a flag to indicate whether we
> should do the check. If x86_emulate() is called by vmexit or realmode
> emulation, we skip this check; if it is called by multi.c, then we do the io
> permission check. But it may be a bit complex for hypervisor to read guest
> process’s TSS and find and check its io bitmap.
> 
> BTW: Why the existence code doesn't check the LOCK prefix (which should cause
> #UD injected to guest)
>       
>       Welcome for your comment, thanks!
> 
> Signed-off-by: Xu Dongxiao <dongxiao.xu@intel.com>
> 
> Best Regards,
> --Dongxiao
>