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

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

 



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


[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