From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: [PATCH]Fix the bug of guest os installation failure and win2k boot failure Date: Mon, 17 Mar 2008 08:32:14 +0000 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="ISO-2022-JP" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: "Xu, Dongxiao" , xen-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org 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" 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 > > Best Regards, > --Dongxiao >