Re: [RFC 04/11] KVM, arm, arm64: Offer PAs to IPAs idmapping to internal VMs

[Christoffer Dall]

On Tue, Sep 26, 2017 at 11:14:45PM +0200, Florent Revest wrote:
> On Thu, 2017-08-31 at 11:23 +0200, Christoffer Dall wrote:
> > > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> > > index 2ea21da..1d2d3df 100644
> > > --- a/virt/kvm/arm/mmu.c
> > > +++ b/virt/kvm/arm/mmu.c
> > > @@ -772,6 +772,11 @@ static void stage2_unmap_memslot(struct kvm
> > > *kvm,
> > >         phys_addr_t size = PAGE_SIZE * memslot->npages;
> > >         hva_t reg_end = hva + size;
> > > 
> > > +       if (unlikely(!kvm->mm)) {
> > I think you should consider using a predicate so that it's clear that
> > this is for in-kernel VMs and not just some random situation where mm
> > can be NULL.
> 
> Internal VMs should be the only usage when kvm->mm would be NULL.
> However if you'd prefer it otherwise, I'll make sure this condition
> will be made clearer.
> 

My point was then when I see (!kvm->mm) it looks like a bug, but if I
saw is_in_kernel_vm(kvm) then it looks like a feature.

> > So it's unclear to me why we don't need any special casing in
> > kvm_handle_guest_abort, related to MMIO exits etc.  You probably
> > assume that we will never do emulation, but that should be described
> > and addressed somewhere before I can critically review this patch.
> 
> This is indeed what I was assuming. This RFC does not allow MMIO with
> internal VMs. I can not think of a usage when this would be useful. I'd
> make sure this would be documented in an eventual later RFC.
> 

OK, sounds good.  It's important for me as a reviewer to be able to tell
the differenc between 'assumed valid guest behavior' and 'limitations of
in-kernel VM support' which are handled in such and such way.


> > > +static int internal_vm_prep_mem(struct kvm *kvm,
> > > +                               const struct
> > > kvm_userspace_memory_region *mem)
> > > +{
> > > +       phys_addr_t addr, end;
> > > +       unsigned long pfn;
> > > +       int ret;
> > > +       struct kvm_mmu_memory_cache cache = { 0 };
> > > +
> > > +       end = mem->guest_phys_addr + mem->memory_size;
> > > +       pfn = __phys_to_pfn(mem->guest_phys_addr);
> > > +       addr = mem->guest_phys_addr;
> > My main concern here is that we don't do any checks on this region
> > and we could be mapping device memory here as well.  Are we intending
> > that to be ok, and are we then relying on the guest to use proper
> > memory attributes ?
> 
> Indeed, being able to map device memory is intended. It is needed for
> Runtime Services sandboxing. It also relies on the guest being
> correctly configured.
> 

So the reason why we wanted to enforce device attribute mappings in
stage 2 was to avoid a guest having the potential to do cached writes to
a device, which would hit at a later time while no longer running the
VM, potentially breaking isolation through manipulation of a device.

This seems to break with that level of isolation, and that property of
in-kernel VMs should be clearly pointed out somewhere.

> > > +
> > > +       for (; addr < end; addr += PAGE_SIZE) {
> > > +               pte_t pte = pfn_pte(pfn, PAGE_S2);
> > > +
> > > +               pte = kvm_s2pte_mkwrite(pte);
> > > +
> > > +               ret = mmu_topup_memory_cache(&cache,
> > > +                                            KVM_MMU_CACHE_MIN_PAGE
> > > S,
> > > +                                            KVM_NR_MEM_OBJS);
> > You should be able to allocate all you need up front instead of doing
> > it in sequences.
> 
> Ok.
> 
> > > 
> > > +               if (ret) {
> > > +                       mmu_free_memory_cache(&cache);
> > > +                       return ret;
> > > +               }
> > > +               spin_lock(&kvm->mmu_lock);
> > > +               ret = stage2_set_pte(kvm, &cache, addr, &pte, 0);
> > > +               spin_unlock(&kvm->mmu_lock);
> > Since you're likely to allocate some large contiguous chunks here,
> > can you have a look at using section mappings?
> 
> Will do.
> 

Thanks!
-Christoffer