From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@qumranet.com>
Subject: Re: KVM: PCIPT: direct mmio
Date: Wed, 04 Jun 2008 17:16:39 +0300
Message-ID: <4846A3C7.8010708@qumranet.com>
References: <alpine.DEB.1.10.0806031418000.8803@cluwyn.haifa.ibm.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Amit Shah <amit.shah@qumranet.com>, kvm@vger.kernel.org,
	Han Weidong <weidong.han@intel.com>,
	"Kay, Allen M" <allen.m.kay@intel.com>,
	Muli Ben-Yehuda <muli@il.ibm.com>
To: Ben-Ami Yassour <benami@il.ibm.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from bzq-179-150-194.static.bezeqint.net ([212.179.150.194]:17646
	"EHLO il.qumranet.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752121AbYFDOQg (ORCPT <rfc822;kvm@vger.kernel.org>);
	Wed, 4 Jun 2008 10:16:36 -0400
In-Reply-To: <alpine.DEB.1.10.0806031418000.8803@cluwyn.haifa.ibm.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Ben-Ami Yassour wrote:
> Amit,
>
> Below is the patch for PCI passthrough tree, it enables a guest to 
> access a device's
> memory mapped I/O regions directly, without requiring the host to trap 
> and
> emulate every MMIO access.
>
> This patch requires only userspace changes and it is relaying on the 
> kernel patch by Anthony: "Handle vma regions with no backing page". 
> Note that this patch requires CONFIG_NUMA to be set. It does require a 
> change to the VT-d that Allen sent a while ago, to avoid mapping of 
> memory slots with no backing page.
>
>
> diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
> index d1e95a4..ce062cb 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;
> +        }

You're using 'len == 0' here to change the semantics of the function.  
It would be better to have two different APIs (perhaps sharing some of 
the implementation by calling a helper).

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.