[PATCH 1/2] ARM: kvm: define PAGE_S2_DEVICE as read-only by default

[christoffer.dall@linaro.org (Christoffer Dall)]

On Sat, Sep 13, 2014 at 01:15:45PM +0200, Ard Biesheuvel wrote:
> On 13 September 2014 12:41, Marc Zyngier <marc.zyngier@arm.com> 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@linaro.org>
> >
> >
> > 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