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