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@xxxxxxxxx>
>
> 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
