On Wed, Oct 13, 2021, Brijesh Singh wrote:
> > The more I look at this, the more strongly I feel that private <=> shared conversions
> > belong in the MMU, and that KVM's SPTEs should be the single source of truth for
> > shared vs. private. E.g. add a SPTE_TDP_PRIVATE_MASK in the software available bits.
> > I believe the only hiccup is the snafu where not zapping _all_ SPTEs on memslot
> > deletion breaks QEMU+VFIO+GPU, i.e. KVM would lose its canonical info on unrelated
> > memslot deletion.
> >
> > But that is a solvable problem. Ideally the bug, wherever it is, would be root
> > caused and fixed. I believe Peter (and Marc?) is going to work on reproducing
> > the bug.
> We have been also setting up VM with Qemu + VFIO + GPU usecase to repro
> the bug on AMD HW and so far we no luck in reproducing it. Will continue
> stressing the system to recreate it. Lets hope that Peter (and Marc) can
> easily recreate on Intel HW so that we can work towards fixing it.
Are you trying on a modern kernel? If so, double check that nx_huge_pages is off,
turning that on caused the bug to disappear. It should be off for AMD systems,
but it's worth checking.
> >> + if (!rc) {
> >> + /*
> >> + * This may happen if another vCPU unmapped the page
> >> + * before we acquire the lock. Retry the PSC.
> >> + */
> >> + write_unlock(&kvm->mmu_lock);
> >> + return 0;
> > How will the caller (guest?) know to retry the PSC if KVM returns "success"?
>
> If a guest is adhering to the GHCB spec then it will see that hypervisor
> has not processed all the entry and it should retry the PSC.
But AFAICT that information isn't passed to the guest. Even in this single-page
MSR-based case, the caller will say "all good" on a return of 0.
The "full" path is more obvious, as the caller clearly continues to process
entries unless there's an actual failure.
+ for (; cur <= end; cur++) {
+ entry = &info->entries[cur];
+ gpa = gfn_to_gpa(entry->gfn);
+ level = RMP_TO_X86_PG_LEVEL(entry->pagesize);
+ op = entry->operation;
+
+ if (!IS_ALIGNED(gpa, page_level_size(level))) {
+ rc = PSC_INVALID_ENTRY;
+ goto out;
+ }
+
+ rc = __snp_handle_page_state_change(vcpu, op, gpa, level);
+ if (rc)
+ goto out;
+ }
