[PATCH] VT-d: Fix iommu map page for mmio pages

[PATCH] VT-d: Fix iommu map page for mmio pages

[Han, Weidong]

[-- Attachment #1: Type: text/plain, Size: 1717 bytes --]

>From 9d8e927a937ff7c9fa2bcc3aa5359e73990658f0 Mon Sep 17 00:00:00 2001
From: Weidong Han <weidong.han@intel.com>
Date: Fri, 19 Sep 2008 14:04:52 +0800
Subject: [PATCH] Fix iommu map page for mmio pages

Don't need to map mmio pages for iommu. When find mmio pages in
kvm_iommu_map_pages(), don't map them, and shouldn't return error due to
it's not an error. If return error (such as -EINVAL), device assigment
will fail.

Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 arch/x86/kvm/vtd.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vtd.c b/arch/x86/kvm/vtd.c
index 667bf3f..b00cdbd 100644
--- a/arch/x86/kvm/vtd.c
+++ b/arch/x86/kvm/vtd.c
@@ -36,14 +36,13 @@ int kvm_iommu_map_pages(struct kvm *kvm,
 {
 	gfn_t gfn = base_gfn;
 	pfn_t pfn;
-	int i, r;
+	int i, r = 0;
 	struct dmar_domain *domain = kvm->arch.intel_iommu_domain;
 
 	/* check if iommu exists and in use */
 	if (!domain)
 		return 0;
 
-	r = -EINVAL;
 	for (i = 0; i < npages; i++) {
 		/* check if already mapped */
 		pfn = (pfn_t)intel_iommu_iova_to_pfn(domain,
@@ -60,13 +59,14 @@ int kvm_iommu_map_pages(struct kvm *kvm,
 						     DMA_PTE_READ |
 						     DMA_PTE_WRITE);
 			if (r) {
-				printk(KERN_DEBUG "kvm_iommu_map_pages:"
+				printk(KERN_ERR "kvm_iommu_map_pages:"
 				       "iommu failed to map pfn=%lx\n",
pfn);
 				goto unmap_pages;
 			}
 		} else {
-			printk(KERN_DEBUG "kvm_iommu_map_page:"
-			       "invalid pfn=%lx\n", pfn);
+			printk(KERN_DEBUG "kvm_iommu_map_pages:"
+			       "invalid pfn=%lx, iommu needn't map "
+			       "MMIO pages!\n", pfn);
 			goto unmap_pages;
 		}
 		gfn++;
-- 
1.5.1

[-- Attachment #2: 0001-Fix-iommu-map-page-for-mmio-pages.patch --]
[-- Type: application/octet-stream, Size: 1655 bytes --]

From 9d8e927a937ff7c9fa2bcc3aa5359e73990658f0 Mon Sep 17 00:00:00 2001
From: Weidong Han <weidong.han@intel.com>
Date: Fri, 19 Sep 2008 14:04:52 +0800
Subject: [PATCH] Fix iommu map page for mmio pages

Don't need to map mmio pages for iommu. When find mmio pages in kvm_iommu_map_pages(), don't map them, and shouldn't return error. It's not an error. If return error (such as -EINVAL), device assigment will fail.

Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 arch/x86/kvm/vtd.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/vtd.c b/arch/x86/kvm/vtd.c
index 667bf3f..b00cdbd 100644
--- a/arch/x86/kvm/vtd.c
+++ b/arch/x86/kvm/vtd.c
@@ -36,14 +36,13 @@ int kvm_iommu_map_pages(struct kvm *kvm,
 {
 	gfn_t gfn = base_gfn;
 	pfn_t pfn;
-	int i, r;
+	int i, r = 0;
 	struct dmar_domain *domain = kvm->arch.intel_iommu_domain;
 
 	/* check if iommu exists and in use */
 	if (!domain)
 		return 0;
 
-	r = -EINVAL;
 	for (i = 0; i < npages; i++) {
 		/* check if already mapped */
 		pfn = (pfn_t)intel_iommu_iova_to_pfn(domain,
@@ -60,13 +59,14 @@ int kvm_iommu_map_pages(struct kvm *kvm,
 						     DMA_PTE_READ |
 						     DMA_PTE_WRITE);
 			if (r) {
-				printk(KERN_DEBUG "kvm_iommu_map_pages:"
+				printk(KERN_ERR "kvm_iommu_map_pages:"
 				       "iommu failed to map pfn=%lx\n", pfn);
 				goto unmap_pages;
 			}
 		} else {
-			printk(KERN_DEBUG "kvm_iommu_map_page:"
-			       "invalid pfn=%lx\n", pfn);
+			printk(KERN_DEBUG "kvm_iommu_map_pages:"
+			       "invalid pfn=%lx, iommu needn't map "
+			       "MMIO pages!\n", pfn);
 			goto unmap_pages;
 		}
 		gfn++;
-- 
1.5.1

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Avi Kivity]

Han, Weidong wrote:
> Don't need to map mmio pages for iommu. When find mmio pages in
> kvm_iommu_map_pages(), don't map them, and shouldn't return error due to
> it's not an error. If return error (such as -EINVAL), device assigment
> will fail.
>
>   


I don't understand.  Why don't we need to map mmio pages?  We certainly 
don't want them emulated.

> @@ -36,14 +36,13 @@ int kvm_iommu_map_pages(struct kvm *kvm,
>  {
>  	gfn_t gfn = base_gfn;
>  	pfn_t pfn;
> -	int i, r;
> +	int i, r = 0;
>  	struct dmar_domain *domain = kvm->arch.intel_iommu_domain;
>  
>  	/* check if iommu exists and in use */
>  	if (!domain)
>  		return 0;
>  
> -	r = -EINVAL;
>  	for (i = 0; i < npages; i++) {
>  		/* check if already mapped */
>  		pfn = (pfn_t)intel_iommu_iova_to_pfn(domain,
> @@ -60,13 +59,14 @@ int kvm_iommu_map_pages(struct kvm *kvm,
>  						     DMA_PTE_READ |
>  						     DMA_PTE_WRITE);
>  			if (r) {
> -				printk(KERN_DEBUG "kvm_iommu_map_pages:"
> +				printk(KERN_ERR "kvm_iommu_map_pages:"
>  				       "iommu failed to map pfn=%lx\n",
> pfn);
>  				goto unmap_pages;
>  			}
>  		} else {
> -			printk(KERN_DEBUG "kvm_iommu_map_page:"
> -			       "invalid pfn=%lx\n", pfn);
> +			printk(KERN_DEBUG "kvm_iommu_map_pages:"
> +			       "invalid pfn=%lx, iommu needn't map "
> +			       "MMIO pages!\n", pfn);
>  			goto unmap_pages;
>  		}

If a slot has a mix of mmio and non-mmio pages, you will unmap the 
non-mmio pages, yet return no error.

-- 
error compiling committee.c: too many arguments to function

RE: [PATCH] VT-d: Fix iommu map page for mmio pages

[Han, Weidong]

Avi Kivity wrote:
> Han, Weidong wrote:
>> Don't need to map mmio pages for iommu. When find mmio pages in
>> kvm_iommu_map_pages(), don't map them, and shouldn't return error
>> due to it's not an error. If return error (such as -EINVAL), device
>> assigment will fail. 
>> 
>> 
> 
> 
> I don't understand.  Why don't we need to map mmio pages?  We
> certainly don't want them emulated.

mmio pages need not to be mapped in VT-d page table, which only
translate DMA addresses. Amit's userspace patch register memslot for
mmios of assigned devices, it doesn't emulate them.

> 
>> @@ -36,14 +36,13 @@ int kvm_iommu_map_pages(struct kvm *kvm,  {
>>  	gfn_t gfn = base_gfn;
>>  	pfn_t pfn;
>> -	int i, r;
>> +	int i, r = 0;
>>  	struct dmar_domain *domain = kvm->arch.intel_iommu_domain;
>> 
>>  	/* check if iommu exists and in use */
>>  	if (!domain)
>>  		return 0;
>> 
>> -	r = -EINVAL;
>>  	for (i = 0; i < npages; i++) {
>>  		/* check if already mapped */
>>  		pfn = (pfn_t)intel_iommu_iova_to_pfn(domain,
>> @@ -60,13 +59,14 @@ int kvm_iommu_map_pages(struct kvm *kvm,

>>  						     DMA_PTE_READ |
DMA_PTE_WRITE);
>>  			if (r) {
>> -				printk(KERN_DEBUG "kvm_iommu_map_pages:"
>> +				printk(KERN_ERR "kvm_iommu_map_pages:"
>>  				       "iommu failed to map pfn=%lx\n",
>> pfn);
>>  				goto unmap_pages;
>>  			}
>>  		} else {
>> -			printk(KERN_DEBUG "kvm_iommu_map_page:"
>> -			       "invalid pfn=%lx\n", pfn);
>> +			printk(KERN_DEBUG "kvm_iommu_map_pages:"
>> +			       "invalid pfn=%lx, iommu needn't map "
>> +			       "MMIO pages!\n", pfn);
>>  			goto unmap_pages;
>>  		}
> 
> If a slot has a mix of mmio and non-mmio pages, you will unmap the
> non-mmio pages, yet return no error.
> 

I didn't consider this mix case. In this mix case, we don't goto
unmap_pages, actually we should remove else {} block. That maps non-mmio
pages while don't map mmio pages. I will resend the patch.

Randy (Weidong)

> --
> error compiling committee.c: too many arguments to function

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Avi Kivity]

Han, Weidong wrote:
> Avi Kivity wrote:
>   
>> Han, Weidong wrote:
>>     
>>> Don't need to map mmio pages for iommu. When find mmio pages in
>>> kvm_iommu_map_pages(), don't map them, and shouldn't return error
>>> due to it's not an error. If return error (such as -EINVAL), device
>>> assigment will fail. 
>>>
>>>
>>>       
>> I don't understand.  Why don't we need to map mmio pages?  We
>> certainly don't want them emulated.
>>     
>
> mmio pages need not to be mapped in VT-d page table, which only
> translate DMA addresses. 

Right, I forgot the iommu is only for dma, not cpu accesses.

I suppose one could DMA into an mmio page.  Is there a reason not to map?


-- 
error compiling committee.c: too many arguments to function

RE: [PATCH] VT-d: Fix iommu map page for mmio pages

[Han, Weidong]

Avi Kivity wrote:
> Han, Weidong wrote:
>> Avi Kivity wrote:
>> 
>>> Han, Weidong wrote:
>>> 
>>>> Don't need to map mmio pages for iommu. When find mmio pages in
>>>> kvm_iommu_map_pages(), don't map them, and shouldn't return error
>>>> due to it's not an error. If return error (such as -EINVAL),
>>>> device assigment will fail. 
>>>> 
>>>> 
>>>> 
>>> I don't understand.  Why don't we need to map mmio pages?  We
>>> certainly don't want them emulated.
>>> 
>> 
>> mmio pages need not to be mapped in VT-d page table, which only
>> translate DMA addresses.
> 
> Right, I forgot the iommu is only for dma, not cpu accesses.
> 
> I suppose one could DMA into an mmio page.  Is there a reason not to
> map? 

Is it possible DMA into an mmio page? If yes, we also need to map mmio
pages, and is_mmio_pfn() check is not neccessary here.

Randy (Weidong)

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Avi Kivity]

Han, Weidong wrote:
> Is it possible DMA into an mmio page? 

I don't see why not.

> If yes, we also need to map mmio
> pages, and is_mmio_pfn() check is not neccessary here.
>   

So we get simpler code as well.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Anthony Liguori]

Avi Kivity wrote:
> Han, Weidong wrote:
>> Is it possible DMA into an mmio page? 
>
> I don't see why not.

Yeah, it is.  I mentioned this a long time ago.  We definitely need to 
map mmio pages into the VT-d mapping.

Regards,

Anthony Liguori

>> If yes, we also need to map mmio
>> pages, and is_mmio_pfn() check is not neccessary here.
>>   
>
> So we get simpler code as well.
>

RE: [PATCH] VT-d: Fix iommu map page for mmio pages

[Han, Weidong]

Avi and Anthony,

I will resend the patch soon. Thanks.

Randy (Weidong)

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Han, Weidong wrote:
>>> Is it possible DMA into an mmio page?
>> 
>> I don't see why not.
> 
> Yeah, it is.  I mentioned this a long time ago.  We definitely need to
> map mmio pages into the VT-d mapping.
> 
> Regards,
> 
> Anthony Liguori
> 
>>> If yes, we also need to map mmio
>>> pages, and is_mmio_pfn() check is not neccessary here.
>>> 
>> 
>> So we get simpler code as well.

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Muli Ben-Yehuda]

On Thu, Sep 25, 2008 at 05:45:30PM +0300, Avi Kivity wrote:
> Han, Weidong wrote:
>> Is it possible DMA into an mmio page? 
>
> I don't see why not.

Two reasons. First it makes no sense. MMIO pages don't have RAM
backing them, they have another device's register window. So the
effect of DMA'ing into an MMIO page would be for one device to DMA
into the register window of another device, which sounds to me insane.

Second, and more importantly, I've seen systems where doing the above
caused a nice, immediate, reboot. So I think that unless someone comes
with a valid scenario where we need to support it or something breaks,
we'd better err on the side of caution and not map pages that should
not be DMA targets.

Cheers,
Muli
-- 
The First Workshop on I/O Virtualization (WIOV '08)
Dec 2008, San Diego, CA, http://www.usenix.org/wiov08/
                      xxx
SYSTOR 2009---The Israeli Experimental Systems Conference
http://www.haifa.il.ibm.com/conferences/systor2009/

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Anthony Liguori]

Muli Ben-Yehuda wrote:
> On Thu, Sep 25, 2008 at 05:45:30PM +0300, Avi Kivity wrote:
>   
>> Han, Weidong wrote:
>>     
>>> Is it possible DMA into an mmio page? 
>>>       
>> I don't see why not.
>>     
>
> Two reasons. First it makes no sense. MMIO pages don't have RAM
> backing them, they have another device's register window. So the
> effect of DMA'ing into an MMIO page would be for one device to DMA
> into the register window of another device, which sounds to me insane.
>   

MMIO isn't just a register window.  It may be an on-device buffer.  For 
instance, all packets are stored in a buffer on the ne2k that's mapped 
via mmio.  It would seem entirely reasonable to me to program an IDE 
driver to DMA directly into the devices packet buffer.

> Second, and more importantly, I've seen systems where doing the above
> caused a nice, immediate, reboot. So I think that unless someone comes
> with a valid scenario where we need to support it or something breaks,
> we'd better err on the side of caution and not map pages that should
> not be DMA targets.
>   

Xen maps the MMIO pages into the VT-d table.  The system you were using 
could have just been busted.  I think the burden is to prove that this 
is illegal (via the architecture specification).

Regards,

Anthony Liguori

> Cheers,
> Muli
>   

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Muli Ben-Yehuda]

On Thu, Sep 25, 2008 at 04:51:24PM -0500, Anthony Liguori wrote:
> Muli Ben-Yehuda wrote:
>> On Thu, Sep 25, 2008 at 05:45:30PM +0300, Avi Kivity wrote:
>>   
>>> Han, Weidong wrote:
>>>     
>>>> Is it possible DMA into an mmio page?       
>>> I don't see why not.
>>>     
>>
>> Two reasons. First it makes no sense. MMIO pages don't have RAM
>> backing them, they have another device's register window. So the
>> effect of DMA'ing into an MMIO page would be for one device to DMA
>> into the register window of another device, which sounds to me insane.
>>   
>
> MMIO isn't just a register window.  It may be an on-device buffer.

Unlikely, but ok.

> For instance, all packets are stored in a buffer on the ne2k that's
> mapped via mmio.  It would seem entirely reasonable to me to program
> an IDE driver to DMA directly into the devices packet buffer.

It would be insane to me. Have you tried this on real hardware and
seen it work?

>> Second, and more importantly, I've seen systems where doing the
>> above caused a nice, immediate, reboot. So I think that unless
>> someone comes with a valid scenario where we need to support it or
>> something breaks, we'd better err on the side of caution and not
>> map pages that should not be DMA targets.
>>   
>
> Xen maps the MMIO pages into the VT-d table.  The system you were using 
> could have just been busted.  I think the burden is to prove that this is 
> illegal (via the architecture specification).

I strongly disagree. You are advocating something that is potentially
unsafe---for the sake of code simplicity?! I am advocating caution in
what we let an *untrusted* guest do.

Cheers,
Muli
-- 
The First Workshop on I/O Virtualization (WIOV '08)
Dec 2008, San Diego, CA, http://www.usenix.org/wiov08/
                      xxx
SYSTOR 2009---The Israeli Experimental Systems Conference
http://www.haifa.il.ibm.com/conferences/systor2009/

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Avi Kivity]

Muli Ben-Yehuda wrote:

  

>> MMIO isn't just a register window.  It may be an on-device buffer.
>>     
>
> Unlikely, but ok.
>
>   

It's unlikely in the same ways graphics cards are unlikely :)

With a multi-card setup, perhaps it is even reasonable for one card to 
dma to another.

> I strongly disagree. You are advocating something that is potentially
> unsafe---for the sake of code simplicity?! I am advocating caution in
> what we let an *untrusted* guest do.
>   

Why would it be unsafe?

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Muli Ben-Yehuda]

On Sat, Sep 27, 2008 at 01:24:31PM +0300, Avi Kivity wrote:

>> I strongly disagree. You are advocating something that is
>> potentially unsafe---for the sake of code simplicity?! I am
>> advocating caution in what we let an *untrusted* guest do.
>
> Why would it be unsafe?

Because on at least one machine letting a device DMA to the same
address as another device's MMIO region caused the machine to reboot.

Cheers,
Muli
-- 
The First Workshop on I/O Virtualization (WIOV '08)
Dec 2008, San Diego, CA, http://www.usenix.org/wiov08/
                      xxx
SYSTOR 2009---The Israeli Experimental Systems Conference
http://www.haifa.il.ibm.com/conferences/systor2009/

Re: [PATCH] VT-d: Fix iommu map page for mmio pages

[Avi Kivity]

Muli Ben-Yehuda wrote:
> On Sat, Sep 27, 2008 at 01:24:31PM +0300, Avi Kivity wrote:
>
>   
>>> I strongly disagree. You are advocating something that is
>>> potentially unsafe---for the sake of code simplicity?! I am
>>> advocating caution in what we let an *untrusted* guest do.
>>>       
>> Why would it be unsafe?
>>     
>
> Because on at least one machine letting a device DMA to the same
> address as another device's MMIO region caused the machine to reboot.
>   

That sounds like a hardware bug.  What does the vendor say about this?

-- 
error compiling committee.c: too many arguments to function