Re: [PATCH v3] Use the POWER8 Micro Partition Prefetch Engine in KVM HV on POWER8

[Alexander Graf <agraf@suse.de>]

On 18.07.14 06:10, Stewart Smith wrote:
> Alexander Graf <agraf@suse.de> writes:
>>> diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
>>> index 1eaea2d..5769497 100644
>>> --- a/arch/powerpc/include/asm/kvm_host.h
>>> +++ b/arch/powerpc/include/asm/kvm_host.h
>>> @@ -305,6 +305,8 @@ struct kvmppc_vcore {
>>>    	u32 arch_compat;
>>>    	ulong pcr;
>>>    	ulong dpdes;		/* doorbell state (POWER8) */
>>> +	unsigned long mpp_buffer; /* Micro Partition Prefetch buffer */
>> Just make this a void*?
> get_free_pages returns an unsigned long and free_pages accepts an
> unsigned long, so I was just avoiding the cast. Is the style in this
> case to do void* rather than unsigned long and cast it everywhere?
>
> In v4 of patch I've gone to void* anyway.

It's probably just a matter of personal taste, but I personally prefer 
to keep pointers to memory locations in pointers.

>
>>> @@ -1516,6 +1540,37 @@ static int on_primary_thread(void)
>>>    	return 1;
>>>    }
>>>    
>>> +static void ppc_start_saving_l2_cache(struct kvmppc_vcore *vc)
>> Please use the "kvmppc" name space.
> ack, done.
>
>>> +	phys_addr_t phy_addr, tmp;
>>> +
>>> +	phy_addr = (phys_addr_t)virt_to_phys((void *)vc->mpp_buffer);
>>> +
>>> +	tmp = phy_addr & PPC_MPPE_ADDRESS_MASK;
>> I would prefer if you give the variable a more telling name.
> ack.
>
>>> +
>>> +	mtspr(SPRN_MPPR, tmp | PPC_MPPR_FETCH_ABORT);
>>> +
>>> +	asm volatile(PPC_LOGMPP(R1) : : "r" (tmp | PPC_LOGMPP_LOG_L2));
>> Can you move this asm() into a static inline function in generic code
>> somewhere?
> okay. It seems the best place may be powerpc/include/asm/cache.h -
> simply because it deals with cache things. I'm open to better
> suggestions :)
>
>>> +
>>> +	vc->mpp_buffer_is_valid = true;
>> Where does this ever get unset? And what point does this variable make?
>> Can't you just check on if (vc->mpp_buffer)?
> The problem with having moved the memory allocation for mpp_buffer to
> vcore setup is that we'll have vc->mpp_buffer != NULL but have some
> random contents in it, so when we first start executing the vcore, we
> shouldn't initiate prefetching (hence mpp_buffer_is_valid).
>
> If we point the prefetch engine to random memory contents, we get the
> most amazing array of incomprehensible illegal accesses :)

I see :). That makes a lot of sense indeed. Maybe rename the variable to 
mpp_content_is_valid to indicate that we are not looking at a valid 
buffer, but valid content?


Alex