On 17.09.2010, at 14:33, Liu Yu-B13201 wrote: > > >> -----Original Message----- >> From: Alexander Graf [mailto:agraf@xxxxxxx] >> Sent: Friday, September 17, 2010 7:34 PM >> To: Liu Yu-B13201 >> Cc: kvm@xxxxxxxxxxxxxxx; kvm-ppc@xxxxxxxxxxxxxxx >> Subject: Re: [PATCH 2/2] kvm/e500v2: mapping guest TLB1 to host TLB0 >> >> >> On 17.09.2010, at 13:28, Liu Yu-B13201 wrote: >> >>> >>> >>>> -----Original Message----- >>>> From: Alexander Graf [mailto:agraf@xxxxxxx] >>>> Sent: Friday, September 17, 2010 6:20 PM >>>> To: Liu Yu-B13201 >>>> Cc: kvm@xxxxxxxxxxxxxxx; kvm-ppc@xxxxxxxxxxxxxxx >>>> 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-owner@xxxxxxxxxxxxxxx >>>>>> [mailto:kvm-ppc-owner@xxxxxxxxxxxxxxx] On Behalf Of >> Alexander Graf >>>>>> Sent: Thursday, September 16, 2010 7:44 PM >>>>>> To: Liu Yu-B13201 >>>>>> Cc: kvm@xxxxxxxxxxxxxxx; kvm-ppc@xxxxxxxxxxxxxxx >>>>>> 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? > > There's no mfmsr. > The MAS0 contain the position we are going to write the entry in. > >> >>> >>>> 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 :) >> > > Sound reasonable.:) > but why using irqsave? So the same function can be used from an irq disabled and an irq enabled context. Explicit irq_disable/irq_enable calls don't stack well ;) Alex -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
