On Sat, Sep 13, 2014 at 01:15:45PM +0200, Ard Biesheuvel wrote: > On 13 September 2014 12:41, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > > Hi Ard, > > > > On 2014-09-13 11:17, Ard Biesheuvel wrote: > >> > >> Now that we support read-only memslots, we need to make sure that > >> pass-through device mappings are not mapped writable if the guest > >> has requested them to be read-only. The existing implementation > >> already honours this by calling kvm_set_s2pte_writable() on the new > >> pte in case of writable mappings, so all we need to do is define > >> the default pgprot_t value used for devices to be PTE_S2_RDONLY. > >> > >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > > > > > > I feel very uncomfortable with this change. Why would we map a device RO? Is > > that only for completeness sake? > > > > We would map a device RO so that QEMU (or whatever is managing KVM) > can emulate the writes. I don't have a clear cut use case, to be > honest, but setting up a writable mapping for a memslot that was > explicitly set up as read-only seems wrong in any case. Agreed, if it doesn't ever make sense to do so, then we should return an error to user space if userspace attempts such a configuration. The current code is just weird. > > Note that the particular problem I was seeing was primarily caused by > kvm_is_mmio_pfn()'s false positive on the zero page, but it unveiled > this particular issue as well. > > > Note that we also use PAGE_S2_DEVICE for things that are not mapped through > > a memslot, such as the GIC. > > > > Yes, and I realize now that this breaks it. > My apologies: I have an additional patch locally that sets up MMIO > ranges in one go instead of faulting them in one page at a time as we > do now, and there the read-write case is handled correctly in > kvm_phys_addr_ioremap(). However, I thought it was better to send > these out separately first, but apparently not. I think it is better to change this separately, and then add the ioremap stuff. However, you need to change all places that call PAGE_S2_DEVICE and expect a RDWR memory region, this happens to be only kvm_phys_addr_ioremap() for now. > > So if we can agree on whether or not MMIO backed mappings should be > read-write even if the memslot says no, I will follow up with a proper > series if there are still changes required. > I guess it could be theoretically useful to have read-only device memory regions, and I can't think of why it would violate the architecture. That said, I don't have any more clear use cases in mind, and we definitely shouldn't just silently ignore the read-only flag from user space and make the region writeable. If we don't want to allow this behavior, we can return an error in kvm_arch_create_memslot(), which will cause the KVM_CREATE_USER_MEMORY_REGION ioctl to return -ENOMEM. Thanks, -Christoffer _______________________________________________ kvmarm mailing list kvmarm@xxxxxxxxxxxxxxxxxxxxx https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
