Re: [PATCH 2/2] KVM: PPC: Book3E: Get vcpu's last instruction for emulation

[Scott Wood <scottwood@freescale.com>]

On 07/10/2013 05:18:10 AM, Alexander Graf wrote:
> 
> On 10.07.2013, at 02:12, Scott Wood wrote:
> 
> > On 07/09/2013 04:45:10 PM, Alexander Graf wrote:
> >> On 28.06.2013, at 11:20, Mihai Caraman wrote:
> >> > +	/* Get page size */
> >> > +	if (MAS0_GET_TLBSEL(mfspr(SPRN_MAS0)) == 0)
> >> > +		psize_shift = PAGE_SHIFT;
> >> > +	else
> >> > +		psize_shift = MAS1_GET_TSIZE(mas1) + 10;
> >> > +
> >> > +	mas7_mas3 = (((u64) mfspr(SPRN_MAS7)) << 32) |
> >> > +		    mfspr(SPRN_MAS3);
> >> > +	addr = (mas7_mas3 & (~0ULL << psize_shift)) |
> >> > +	       (geaddr & ((1ULL << psize_shift) - 1ULL));
> >> > +
> >> > +	/* Map a page and get guest's instruction */
> >> > +	page = pfn_to_page(addr >> PAGE_SHIFT);
> >> While looking at this I just realized that you're missing a check  
> here. What if our IP is in some PCI BAR? Or can't we execute from  
> those?
> >
> > We at least need to check pfn_valid() first.  That'll just keep us  
> from accessing a bad pointer in the host kernel, though -- it won't  
> make the emulation actually work.  If we need that, we'll probably  
> need to create a temporary TLB entry manually.
> 
> ioremap()?

That's a bit heavy... also we'd need to deal with cacheability.  This  
code is already engaged in directly creating TLB entries, so it doesn't  
seem like much of a stretch to create one for this.  It should be  
faster than ioremap() or kmap_atomic().

The one complication is allocating the virtual address space, but maybe  
we could just use the page that kmap_atomic would have used?  Of  
course, if we want to handle execution from other than normal kernel  
memory, we'll need to make sure that the virtual address space is  
allocated even when highmem is not present (e.g. 64-bit).

> However, if we were walking the guest TLB cache instead we would get  
> a guest physical address which we can always resolve to a host  
> virtual address.
> 
> I'm not sure how important that whole use case is though. Maybe we  
> should just error out to the guest for now.

It's not that important, now that we are using hugetlb rather than  
directly mapping a large hunk of reserved memory.  It would be nice to  
handle it though, if we can do without too much hassle.  And I think  
manually creating a TLB entry could be faster than kmap_atomic(), or  
searching the guest TLB and then doing a reverse memslot lookup.

-Scott