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 thetypo: 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