Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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/ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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/ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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/ -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] VT-d: Fix iommu map page for mmio pages
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] VT-d: Fix iommu map page for mmio pages
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) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] VT-d: Fix iommu map page for mmio pages
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] VT-d: Fix iommu map page for mmio pages
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
