On 18.07.14 06:10, Stewart Smith wrote:
Alexander Graf <ag...@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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to