From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts Date: Mon, 6 May 2013 22:04:52 -0500 Message-ID: <1367895892.3398.13@snotra> References: <51886A59.80807@windriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="Flowed"; DelSp="Yes" Content-Transfer-Encoding: 7bit Cc: linuxppc-dev@lists.ozlabs.org, agraf@suse.de, kvm-ppc@vger.kernel.org, kvm@vger.kernel.org To: tiejun.chen Return-path: In-Reply-To: <51886A59.80807@windriver.com> (from tiejun.chen@windriver.com on Mon May 6 21:43:37 2013) Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: linuxppc-dev-bounces+glppe-linuxppc-embedded-2=m.gmane.org@lists.ozlabs.org Sender: "Linuxppc-dev" List-Id: kvm.vger.kernel.org On 05/06/2013 09:43:37 PM, tiejun.chen wrote: > On 05/07/2013 10:06 AM, Scott Wood wrote: >> On 05/06/2013 08:56:25 PM, tiejun.chen wrote: >>> On 05/07/2013 07:50 AM, Scott Wood wrote: >>>> On 05/05/2013 10:13:17 PM, tiejun.chen wrote: >>>>> On 05/06/2013 11:10 AM, Tiejun Chen wrote: >>>>>> For the external interrupt, the decrementer exception and the >>>>>> doorbell >>>>>> excpetion, we also need to soft-disable interrupts while doing >>>>>> as host >>>>>> interrupt handlers since the DO_KVM hook is always performed to >>>>>> skip >>>>>> EXCEPTION_COMMON then miss this original chance with the 'ints' >>>>>> (INTS_DISABLE). >>>> >>>> http://patchwork.ozlabs.org/patch/241344/ >>>> http://patchwork.ozlabs.org/patch/241412/ >>>> >>>> :-) >>> >>> I'm observing the same behaviour as well: >>> >>> WARN_ON_ONCE(!irqs_disabled()); >> >> So, could you explain the benefits of your approach over what's >> being discussed >> in those threads? > > They're a long thread so I think I need to take time to see :) > >> >>>> Why wouldn't we always disable them? kvmppc_handle_exit() will >>>> enable >>>> interrupts when it's ready. >>> >>> This only disable soft interrupt for kvmppc_restart_interrupt() >>> that restarts >>> interrupts if they were meant for the host: >>> >>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL | >>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL >> >> Those aren't the only exceptions that can end up going to the host. >> We could >> get a TLB miss that results in a heavyweight MMIO exit, etc. > > This is like host handler, so I'm just disabling soft interrupt > during kvmppc_restart_interrupt() for Doorbell interrupt/Decrementer > Interrupt/External Input Interrupt. > > I don't see anything should be disabled for any TLB exception in host > handler. Every exception type needs consistent lazy EE state once we hit the local_irq_enable() (or any other C code that cares). Plus, if you're going to add code to make something conditional, you should have a good reason for making it conditional. Being more like the 64-bit host handler for its own sake isn't good enough, especially if you introduce differences between 32 and 64 bit in the process. >> And I'd rather see any fix for this problem stay out of the asm code. > > We already have an appropriate SOFT_DISABLE_INTS so I think we can > take this easily :) An unconditional call to hard_irq_disable() at the beginning of kvmppc_handle_exit() should suffice. I already have this in the next version of my patch that I'll be posting shortly. Note that we need this on 32-bit as well, so that trace_hardirqs_off() gets called. -Scott