On Thu, Nov 21, 2019 at 11:18:48AM +0100, Paolo Bonzini wrote:
> On 19/11/19 09:49, Yang Weijiang wrote:
> > + if (spte & PT_SPP_MASK) {
> > + fault_handled = true;
> > + vcpu->run->exit_reason = KVM_EXIT_SPP;
> > + vcpu->run->spp.addr = gva;
> > + kvm_skip_emulated_instruction(vcpu);
>
> Do you really want to skip the current instruction? Who will do the write?
>
If the destination memory is SPP protected, the target memory is
expected unchanged on a "write op" in guest, so would like to skip current
instruction.
> > + pr_info("SPP - SPPT entry missing! gfn = 0x%llx\n", gfn);
>
> Please replace pr_info with a tracepoint.
>
OK.
> > + slot = gfn_to_memslot(vcpu->kvm, gfn);
> > + if (!slot)
> > + return -EFAULT;
>
> You want either a goto to the misconfig case, so that there is a warn
>
OK.
> > + spp_info.base_gfn = gfn;
> > + spp_info.npages = 1;
> > +
> > + spin_lock(&vcpu->kvm->mmu_lock);
> > + ret = kvm_spp_get_permission(vcpu->kvm, &spp_info);
> > + if (ret == 1) {
>
> Can you clarify when ret will not be 1? In this case you already have a
> slot, so it seems to me that you do not need to go through
> kvm_spp_get_permission and you can just test "if
> (kvm->arch.spp_active)". But then, spp_active should be 1 if you get
> here, I think?
>
Hmm, getting permission bits from gfn directly should work here.
Thank you!
> > + pr_alert("SPP - SPPT Misconfiguration!\n");
> > + return 0;
>
>
> pr_alert not needed since you've just warned.
>
OK, will remove it.
> Paolo
