On Thu, 2013-01-17 at 19:58 +0200, Gleb Natapov wrote:
> On Thu, Jan 17, 2013 at 10:30:05AM -0700, Alex Williamson wrote:
> > On Thu, 2013-01-17 at 19:20 +0200, Gleb Natapov wrote:
> > > On Thu, Jan 17, 2013 at 10:12:06AM -0700, Alex Williamson wrote:
> > > > On Thu, 2013-01-17 at 09:29 +0200, Gleb Natapov wrote:
> > > > > On Thu, Jan 17, 2013 at 11:20:43AM +0800, Xiao Guangrong wrote:
> > > > > > On 01/17/2013 03:07 AM, Marcelo Tosatti wrote:
> > > > > > > On Fri, Jan 11, 2013 at 06:25:18PM +0900, Takuya Yoshikawa wrote:
> > > > > > >> Patches 1 to 3 are trivial.
> > > > > > >>
> > > > > > >> Patch 4 is the main cause of the increased lines, but I think the new
> > > > > > >> code makes it easier to understand why each condition in
> > > > > > >> __kvm_set_memory_region() is there.
> > > > > > >>
> > > > > > >> If you don't agree with patch 4, please consider taking the rest of the
> > > > > > >> series at this time.
> > > > > > >>
> > > > > > >> Takuya Yoshikawa (4):
> > > > > > >> KVM: set_memory_region: Don't jump to out_free unnecessarily
> > > > > > >> KVM: set_memory_region: Don't check for overlaps unless we create or move a slot
> > > > > > >> KVM: set_memory_region: Remove unnecessary variable memslot
> > > > > > >> KVM: set_memory_region: Identify the requested change explicitly
> > > > > > >>
> > > > > > >> virt/kvm/kvm_main.c | 94 ++++++++++++++++++++++++++++++++-------------------
> > > > > > >> 1 files changed, 59 insertions(+), 35 deletions(-)
> > > > > > >>
> > > > > > >> --
> > > > > > >> 1.7.5.4
> > > > > > >
> > > > > > > Reviewed-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> > > > > > >
> > > > > > > BTW, while at it, its probably worthwhile to restrict flags
> > > > > > > modifications: change from flags = 0 to flags = read-only is
> > > > > > > incomplete. Xiao, should it be allowed only during creation?
> > > > > >
> > > > > > Will Readonly memory be used for VM-mem-sharing in the future?
> > > > > > I remember you mentioned about mem-sharing things before. ;)
> > > > > >
> > > > > > Actually, It is safe on KVM MMU because all the gfns in the slot
> > > > > > can become readonly after calling __kvm_set_memory_region. It is
> > > > > > unsafe on IOMMU, what need to be fixed is unmapping gfns on IOMMU
> > > > > > when the flag is changed.
> > > > >
> > > > > Which means if we allow changing flags from 0 to read_only or back we
> > > > > need to call kvm_iommu_map_pages() when flags change. BTW as far as I
> > > > > see currently IOMMU maps everything as read/write not matter what flags
> > > > > say. Looks like a bug. Alex?
> > > >
> > > > That's correct, legacy device assignment is always r/w. vfio handles
> > > > this correctly. Thanks,
> > > >
> > > Does this mean that device can write into read only memory?
> >
> > For legacy assignment, I would expect yes. On bare metal, DMA from an
> > I/O device can obviously circumvent processor based memory attributes,
> > but here that might also include things like ROMs too. Thanks,
> >
> Why dropping IOMMU_WRITE from flags if slot is read only in
> kvm_iommu_map_pages() will not work?
Something like:
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -76,7 +76,9 @@ int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slo
gfn = slot->base_gfn;
end_gfn = gfn + slot->npages;
- flags = IOMMU_READ | IOMMU_WRITE;
+ flags = IOMMU_READ;
+ if (slot->flags & KVM_MEM_READONLY)
+ flags |= IOMMU_WRITE;
if (kvm->arch.iommu_flags & KVM_IOMMU_CACHE_COHERENCY)
flags |= IOMMU_CACHE;
We'd still need the unmap/remap when flags change. I'll do some testing
on the above and send as a proper patch. Thanks,
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
