On Sunday 17 Oct 2021 at 11:41:21 (+0100), Marc Zyngier wrote:
> Not directly related to this code, but it looks to me that
> kvm_host_owns_hyp_mappings() really ought to check for
> is_kernel_in_hyp_mode() on its own. VHE really deals with its own
> mappings, and create_hyp_mappings() already has a check to do nothing
> on VHE.
Sure, I'll stick a patch at the beginning of the series.
>
> > +
> > + return pkvm_share_hyp(kvm_kaddr_to_phys(from), kvm_kaddr_to_phys(to));
> > +}
> > +
> > /**
> > * create_hyp_mappings - duplicate a kernel virtual address range in Hyp mode
> > * @from: The virtual kernel start address of the range
> > @@ -316,12 +327,8 @@ int create_hyp_mappings(void *from, void *to, enum kvm_pgtable_prot prot)
> > if (is_kernel_in_hyp_mode())
> > return 0;
> >
> > - if (!kvm_host_owns_hyp_mappings()) {
> > - if (WARN_ON(prot != PAGE_HYP))
> > - return -EPERM;
> > - return pkvm_share_hyp(kvm_kaddr_to_phys(from),
> > - kvm_kaddr_to_phys(to));
> > - }
> > + if (WARN_ON(!kvm_host_owns_hyp_mappings()))
> > + return -EPERM;
>
> Do we really need this? Can't we just verify that all the code paths
> to create_hyp_mappings() check for kvm_host_owns_hyp_mappings()?
>
> At the very least, make this a VM_BUG_ON() so that this is limited to
> debug. Yes, I'm quickly developing a WARN_ON()-phobia.
Right, that _is_ purely debug. It's just that folks are used to being
able to just call create_hyp_mappings() for anything, so I wanted to
make sure we have an easy way to catch future changes that would
unknowingly break pKVM, but no objection to make this VM_BUG_ON().
Cheers,
Quentin
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
