> -----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? Yu -- 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
