Hi Drew, On 2/17/21 10:43 AM, Andrew Jones wrote: > On Tue, Feb 16, 2021 at 12:18:31PM +0000, Alexandru Elisei wrote: >> Hi Marc, >> >> Played with this for a bit to try to understand the problem better, wrote a simple >> MMIO device in kvmtool which maps the memory as a read-only memslot [1] and poked >> it with kvm-unit-tests [2]. >> >> [1] https://gitlab.arm.com/linux-arm/kvmtool-ae/-/tree/mmiodev-wip1 >> >> [2] https://gitlab.arm.com/linux-arm/kvm-unit-tests-ae/-/tree/mmiodev-wip1 > Looks like you forgot to add arm/mmiodev.c to your commit. Fixed, thanks for pointing that out! Thanks, Alex > > Thanks, > drew > >> On 2/11/21 2:27 PM, Marc Zyngier wrote: >>> It appears that when a guest traps into KVM because it is >>> performing a CMO on a Read Only memslot, our handling of >>> this operation is "slightly suboptimal", as we treat it as >>> an MMIO access without a valid syndrome. >>> >>> The chances that userspace is adequately equiped to deal >>> with such an exception being slim, it would be better to >>> handle it in the kernel. >>> >>> What we need to provide is roughly as follows: >>> >>> (a) if a CMO hits writeable memory, handle it as a normal memory acess >>> (b) if a CMO hits non-memory, skip it >>> (c) if a CMO hits R/O memory, that's where things become fun: >>> (1) if the CMO is DC IVAC, the architecture says this should result >>> in a permission fault >>> (2) if the CMO is DC CIVAC, it should work similarly to (a) >>> >>> We already perform (a) and (b) correctly, but (c) is a total mess. >>> Hence we need to distinguish between IVAC (c.1) and CIVAC (c.2). >>> >>> One way to do it is to treat CMOs generating a translation fault as >>> a *read*, even when they are on a RW memslot. This allows us to >>> further triage things: >>> >>> If they come back with a permission fault, that is because this is >>> a DC IVAC instruction: >>> - inside a RW memslot: no problem, treat it as a write (a)(c.2) >>> - inside a RO memslot: inject a data abort in the guest (c.1) >>> >>> The only drawback is that DC IVAC on a yet unmapped page faults >>> twice: one for the initial translation fault that result in a RO >>> mapping, and once for the permission fault. I think we can live with >>> that. >>> >>> Reported-by: Jianyong Wu <jianyong.wu@xxxxxxx> >>> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx> >>> --- >>> >>> Notes: >>> I have taken the option to inject an abort in the guest when >>> it issues a DC IVAC on a R/O memslot, but another option would >>> be to just perform the invalidation ourselves as a DC CIAVAC. >>> >>> This would have the advantage of being consistent with what we >>> do for emulated MMIO. >>> >>> arch/arm64/kvm/mmu.c | 53 ++++++++++++++++++++++++++++++++++---------- >>> 1 file changed, 41 insertions(+), 12 deletions(-) >>> >>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c >>> index 7d2257cc5438..c7f4388bea45 100644 >>> --- a/arch/arm64/kvm/mmu.c >>> +++ b/arch/arm64/kvm/mmu.c >>> @@ -760,7 +760,17 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, >>> struct kvm_pgtable *pgt; >>> >>> fault_granule = 1UL << ARM64_HW_PGTABLE_LEVEL_SHIFT(fault_level); >>> - write_fault = kvm_is_write_fault(vcpu); >>> + /* >>> + * Treat translation faults on CMOs as read faults. Should >>> + * this further generate a permission fault on a R/O memslot, >>> + * it will be caught in kvm_handle_guest_abort(), with >>> + * prejudice. Permission faults on non-R/O memslot will be >>> + * gracefully handled as writes. >>> + */ >>> + if (fault_status == FSC_FAULT && kvm_vcpu_dabt_is_cm(vcpu)) >>> + write_fault = false; >> This means that every DC CIVAC will map the IPA with read permissions in the stage >> 2 tables, regardless of the IPA being already mapped. It's harmless, but a bit >> unexpected. >> >>> + else >>> + write_fault = kvm_is_write_fault(vcpu); >>> exec_fault = kvm_vcpu_trap_is_exec_fault(vcpu); >>> VM_BUG_ON(write_fault && exec_fault); >>> >>> @@ -1013,19 +1023,37 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) >>> } >>> >>> /* >>> - * Check for a cache maintenance operation. Since we >>> - * ended-up here, we know it is outside of any memory >>> - * slot. But we can't find out if that is for a device, >>> - * or if the guest is just being stupid. The only thing >>> - * we know for sure is that this range cannot be cached. >>> + * Check for a cache maintenance operation. Three cases: >>> + * >>> + * - It is outside of any memory slot. But we can't find out >>> + * if that is for a device, or if the guest is just being >>> + * stupid. The only thing we know for sure is that this >>> + * range cannot be cached. So let's assume that the guest >>> + * is just being cautious, and skip the instruction. >>> + * >>> + * - Otherwise, check whether this is a permission fault. >>> + * If so, that's a DC IVAC on a R/O memslot, which is a >>> + * pretty bad idea, and we tell the guest so. >>> * >>> - * So let's assume that the guest is just being >>> - * cautious, and skip the instruction. >>> + * - If this wasn't a permission fault, pass it along for >>> + * further handling (including faulting the page in if it >>> + * was a translation fault). >>> */ >>> - if (kvm_is_error_hva(hva) && kvm_vcpu_dabt_is_cm(vcpu)) { >>> - kvm_incr_pc(vcpu); >>> - ret = 1; >>> - goto out_unlock; >>> + if (kvm_vcpu_dabt_is_cm(vcpu)) { >>> + if (kvm_is_error_hva(hva)) { >>> + kvm_incr_pc(vcpu); >>> + ret = 1; >>> + goto out_unlock; >>> + } >>> + >>> + if (fault_status == FSC_PERM) { >>> + /* DC IVAC on a R/O memslot */ >>> + kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu)); >>> + ret = 1; >>> + goto out_unlock; >>> + } >> I don't like the inconsistency. We go from exiting to userspace for both DC >> IVAC/DC CIVAC to mapping the IPA with read permissions for DC CIVAC, but injecting >> a DABT for a DC IVAC. DC IVAC acts just like a DC CIVAC and requires the same >> permissions when executed by a guest, so I'm not sure we should be handling them >> differently. >> >> Thanks, >> >> Alex >> >>> + >>> + goto handle_access; >>> } >>> >>> /* >>> @@ -1039,6 +1067,7 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu) >>> goto out_unlock; >>> } >>> >>> +handle_access: >>> /* Userspace should not be able to register out-of-bounds IPAs */ >>> VM_BUG_ON(fault_ipa >= kvm_phys_size(vcpu->kvm)); >>> >> _______________________________________________ >> kvmarm mailing list >> kvmarm@xxxxxxxxxxxxxxxxxxxxx >> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm >>
