On 17.09.2010, at 10:47, Liu Yu-B13201 wrote: > > >> -----Original Message----- >> From: kvm-ppc-ow...@vger.kernel.org >> [mailto:kvm-ppc-ow...@vger.kernel.org] On Behalf Of Alexander Graf >> Sent: Thursday, September 16, 2010 7:44 PM >> To: Liu Yu-B13201 >> Cc: kvm@vger.kernel.org; kvm-...@vger.kernel.org >> Subject: Re: [PATCH 2/2] kvm/e500v2: mapping guest TLB1 to host TLB0 >> >>>> >>>>> /* >>>>> * writing shadow tlb entry to host TLB >>>>> */ >>>>> -static inline void __write_host_tlbe(struct tlbe *stlbe) >>>>> +static inline void __host_tlbe_write(struct tlbe *stlbe) >>>>> { >>>>> mtspr(SPRN_MAS1, stlbe->mas1); >>>>> mtspr(SPRN_MAS2, stlbe->mas2); >>>>> @@ -109,25 +110,22 @@ static inline void >>>>> >>>> __write_host_tlbe(struct tlbe *stlbe) >>>> >>>>> __asm__ __volatile__ ("tlbwe\n" : : ); >>>>> } >>>>> >>>>> -static inline void write_host_tlbe(struct kvmppc_vcpu_e500 >>>>> >>>> *vcpu_e500, >>>> >>>>> - int tlbsel, int esel, struct tlbe *stlbe) >>>>> +static inline u32 host_tlb0_write(struct kvmppc_vcpu_e500 >>>>> >>>> *vcpu_e500, >>>> >>>>> + u32 gvaddr, struct >> tlbe *stlbe) >>>>> { >>>>> - local_irq_disable(); >>>>> - if (tlbsel == 0) { >>>>> - __write_host_tlbe(stlbe); >>>>> - } else { >>>>> - unsigned register mas0; >>>>> + unsigned register mas0; >>>>> >>>>> - mas0 = mfspr(SPRN_MAS0); >>>>> + local_irq_disable(); >>>>> >>>> Do you have to disable interrupts for a tlb write? If you get >>>> preempted before the write, the entry you overwrite could be >>>> different. But you don't protect against that either way. And >>>> if you get preempted afterwards, you could lose the entry. >>>> But since you enable interrupts beyond that, that could >>>> happen either way too. >>>> >>>> So what's the reason for the disable here? >>>> >>>> >>> >>> Hello Alex, >>> >>> Doesn't local_irq_disable() also disable preempt? >>> The reason to disable interrupts is because it's possible >> to have tlb >>> misses in interrupt service route. >>> >> >> Yes, local_irq_disable disables preempt. That's exactly what I was >> referring to :). >> I don't understand the statement about the service route. A tlb miss >> still arrives with local_irq_disable. >> >> > > Use local_irq_disable here is to make sure we update masN in atomic. > If get preempted when we update part of masN, > then we may write wrong TLB entry in hardware.
I see, makes sense. Doing the mfmsr of MAS0 in an irq disabled section doesn't make sense to me though. Also, wouldn't it be better to do the irq disabling inside of __host_tlbe_write using irqsave, not the explicit enable/disable? > And local_irq_disable blocks external and decrement interrupts. > Althought it's unlikely to have tlb miss in external and decrement interrrupt > handler of current kernel. > It still has the possibility.. Yup :). Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
