RE: [PATCH 2/2] kvm/e500v2: mapping guest TLB1 to host TLB0

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



 

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


[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux