On 17.09.2010, at 13:28, Liu Yu-B13201 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:ag...@suse.de] >> Sent: Friday, September 17, 2010 6:20 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 >> >> >> 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. > > Hardware choose the position and put TLB entry by itself. > I need to get the position from MAS0. > So that I know exactly which old entry is overrided.
Huh? But you do the mfmsr before the tlb write? Or does MAS0 contain the "next" tlb pointer? > >> Also, wouldn't it be better to do the irq disabling inside of >> __host_tlbe_write using irqsave, not the explicit enable/disable? >> > > Oh? What benefit we can get from this? Cleaner structure. If writing a tlb entry needs to happen atomically, the function you call should be atomic. In your case that's reasonably simple to do. Also, you could return MAS0 in that function. That would make things a lot more obvious. While you're at it, a comment explaining what the function does doesn't hurt either :) 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
