Re: [PATCH 1/3] KVM: arm64: Fix S1PTW handling on RO memslots

[Oliver Upton <oliver.upton@linux.dev>]

On Wed, Dec 21, 2022 at 09:35:06AM +0000, Marc Zyngier wrote:

[...]

> > > +	if (kvm_vcpu_abt_iss1tw(vcpu)) {
> > > +		/*
> > > +		 * Only a permission fault on a S1PTW should be
> > > +		 * considered as a write. Otherwise, page tables baked
> > > +		 * in a read-only memslot will result in an exception
> > > +		 * being delivered in the guest.
> > 
> > Somewhat of a tangent, but:
> > 
> > Aren't we somewhat unaligned with the KVM UAPI by injecting an
> > exception in this case? I know we've been doing it for a while, but it
> > flies in the face of the rules outlined in the
> > KVM_SET_USER_MEMORY_REGION documentation.
> 
> That's an interesting point, and I certainly haven't considered that
> for faults introduced by page table walks.
> 
> I'm not sure what userspace can do with that though. The problem is
> that this is a write for which we don't have useful data: although we
> know it is a page-table walker access, we don't know what it was about
> to write. The instruction that caused the write is meaningless (it
> could either be a load, a store, or an instruction fetch). How do you
> populate the data[] field then?
> 
> If anything, this is closer to KVM_EXIT_ARM_NISV, for which we give
> userspace the full ESR and ask it to sort it out. I doubt it will be
> able to, but hey, maybe it is worth a shot. This would need to be a
> different exit reason though, as NISV is explicitly for non-memslot
> stuff.
> 
> In any case, the documentation for KVM_SET_USER_MEMORY_REGION needs to
> reflect the fact that KVM_EXIT_MMIO cannot represent a fault due to a
> S1 PTW.

Oh I completely agree with you here. I probably should have said before,
I think the exit would be useless anyway. Getting the documentation in
line with the intended behavior seems to be the best fix.

> >
> > > +		 * The drawback is that we end-up fauling twice if the
> > 
> > typo: s/fauling/faulting/
> > 
> > > +		 * guest is using any of HW AF/DB: a translation fault
> > > +		 * to map the page containing the PT (read only at
> > > +		 * first), then a permission fault to allow the flags
> > > +		 * to be set.
> > > +		 */
> > > +		switch (kvm_vcpu_trap_get_fault_type(vcpu)) {
> > > +		case ESR_ELx_FSC_PERM:
> > > +			return true;
> > > +		default:
> > > +			return false;
> > > +		}
> > > +	}
> > >  
> > >  	if (kvm_vcpu_trap_is_iabt(vcpu))
> > >  		return false;
> > > -- 
> > > 2.34.1
> > > 
> > 
> > Besides the changelog/comment suggestions, the patch looks good to me.
> > 
> > Reviewed-by: Oliver Upton <oliver.upton@linux.dev>
> 
> Thanks for the quick review! I'll wait a bit before respinning the
> series, as I'd like to get closure on the UAPI point you have raised.

I'm satisfied if you are :)

--
Thanks,
Oliver