Re: [PATCH 1/1] Enble a guest to access a device's memory mapped I/O regions directly.

[Muli Ben-Yehuda <muli@il.ibm.com>]

On Fri, Apr 18, 2008 at 06:56:41PM +0300, Avi Kivity wrote:
> benami@il.ibm.com wrote:
> > From: Ben-Ami Yassour <benami@il.ibm.com>
> >
> > Signed-off-by: Ben-Ami Yassour <benami@il.ibm.com>
> > Signed-off-by: Muli Ben-Yehuda <muli@il.ibm.com>
> > ---
> >  libkvm/libkvm.c           |   24 ++++++++----
> >  qemu/hw/pci-passthrough.c |   89 +++++++++++----------------------------------
> >  qemu/hw/pci-passthrough.h |    2 +
> >  3 files changed, 40 insertions(+), 75 deletions(-)
> >
> > diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
> > index de91328..8c02af9 100644
> > --- a/libkvm/libkvm.c
> > +++ b/libkvm/libkvm.c
> > @@ -400,7 +400,7 @@ void *kvm_create_userspace_phys_mem(kvm_context_t kvm, unsigned long phys_start,
> >  {
> >  	int r;
> >  	int prot = PROT_READ;
> > -	void *ptr;
> > +	void *ptr = NULL;
> >  	struct kvm_userspace_memory_region memory = {
> >  		.memory_size = len,
> >  		.guest_phys_addr = phys_start,
> > @@ -410,16 +410,24 @@ void *kvm_create_userspace_phys_mem(kvm_context_t kvm, unsigned long phys_start,
> >  	if (writable)
> >  		prot |= PROT_WRITE;
> >  
> > -	ptr = mmap(NULL, len, prot, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
> > -	if (ptr == MAP_FAILED) {
> > -		fprintf(stderr, "create_userspace_phys_mem: %s", strerror(errno));
> > -		return 0;
> > -	}
> > +	if (len > 0) {
> > +		ptr = mmap(NULL, len, prot, MAP_ANONYMOUS | MAP_SHARED, -1, 0);
> > +		if (ptr == MAP_FAILED) {
> > +			fprintf(stderr, "create_userspace_phys_mem: %s",
> > +				strerror(errno));
> > +			return 0;
> > +		}
> >  
> > -	memset(ptr, 0, len);
> > +		memset(ptr, 0, len);
> > +	}
> >  
> >  	memory.userspace_addr = (unsigned long)ptr;
> > -	memory.slot = get_free_slot(kvm);
> > +
> > +	if (len > 0)
> > +		memory.slot = get_free_slot(kvm);
> > +	else
> > +		memory.slot = get_slot(phys_start);
> > +
> >  	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &memory);
> >  	if (r == -1) {
> >  		fprintf(stderr, "create_userspace_phys_mem: %s", strerror(errno));
> >   
> 
> This looks like support for zero-length memory slots? Why is it
> needed?
> 
> It needs to be in a separate patch.

We need an interface to remove a memslot. When the guest writes to a
direct assigned device's BAR and changes an MMIO region, we need to
remove the old slot and establish a new one. The kernel side treats
0-sized memslot as "remove", but the userspace side didn't quite
handle it properly.

Personally I would've preferred a proper "remove" interface, rather
than shoehorning it into kvm_create_userspace_phys_mem and a 0-sized
slot. If that's acceptable, we'll put together a patch.

> > diff --git a/qemu/hw/pci-passthrough.c b/qemu/hw/pci-passthrough.c
> > index 7ffcc7b..a5894d9 100644
> > --- a/qemu/hw/pci-passthrough.c
> > +++ b/qemu/hw/pci-passthrough.c
> > @@ -25,18 +25,6 @@ typedef __u64 resource_size_t;
> >  extern kvm_context_t kvm_context;
> >  extern FILE *logfile;
> >  
> > -CPUReadMemoryFunc *pt_mmio_read_cb[3] = {
> > -	pt_mmio_readb,
> > -	pt_mmio_readw,
> > -	pt_mmio_readl
> > -};
> > -
> > -CPUWriteMemoryFunc *pt_mmio_write_cb[3] = {
> > -	pt_mmio_writeb,
> > -	pt_mmio_writew,
> > -	pt_mmio_writel
> > -};
> > -
> >   
> 
> There's at least one use case for keeping mmio in userspace:
> reverse-engineering a device driver. So if it doesn't cause too much
> trouble, please keep this an option.

I don't think it's a big deal to support both, although I'm not sure
how useful it would be (especially considering mmiotrace). Did you
have a user-interface for specifying it in mind?

Cheers,
Muli

-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference 
Don't miss this year's exciting event. There's still time to save $100. 
Use priority code J8TL2D2. 
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone