Re: [PATCH v2 21/30] KVM: s390: pci: handle refresh of PCI translations

From: Pierre Morel
Date: Thu Jan 20 2022 - 04:45:57 EST



On 1/19/22 21:02, Matthew Rosato wrote:
On 1/19/22 1:25 PM, Pierre Morel wrote:


On 1/19/22 17:39, Matthew Rosato wrote:
On 1/19/22 4:29 AM, Pierre Morel wrote:


On 1/14/22 21:31, Matthew Rosato wrote:
...
+static int dma_table_shadow(struct kvm_vcpu *vcpu, struct zpci_dev *zdev,
+                dma_addr_t dma_addr, size_t size)
+{
+    unsigned int nr_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+    struct kvm_zdev *kzdev = zdev->kzdev;
+    unsigned long *entry, *gentry;
+    int i, rc = 0, rc2;
+
+    if (!nr_pages || !kzdev)
+        return -EINVAL;
+
+    mutex_lock(&kzdev->ioat.lock);
+    if (!zdev->dma_table || !kzdev->ioat.head[0]) {
+        rc = -EINVAL;
+        goto out_unlock;
+    }
+
+    for (i = 0; i < nr_pages; i++) {
+        gentry = dma_walk_guest_cpu_trans(vcpu, &kzdev->ioat, dma_addr);
+        if (!gentry)
+            continue;
+        entry = dma_walk_cpu_trans(zdev->dma_table, dma_addr);
+
+        if (!entry) {
+            rc = -ENOMEM;
+            goto out_unlock;
+        }
+
+        rc2 = dma_shadow_cpu_trans(vcpu, entry, gentry);
+        if (rc2 < 0) {
+            rc = -EIO;
+            goto out_unlock;
+        }
+        dma_addr += PAGE_SIZE;
+        rc += rc2;
+    }
+

In case of error, shouldn't we invalidate the shadow tables entries we did validate until the error?

Hmm, I don't think this is strictly necessary - the status returned should indicate the specified DMA range is now in an indeterminate state (putting the onus on the guest to take corrective action via a global refresh).

In fact I think I screwed that up below in kvm_s390_pci_refresh_trans, the fabricated status should always be KVM_S390_RPCIT_INS_RES.

OK



+out_unlock:
+    mutex_unlock(&kzdev->ioat.lock);
+    return rc;
+}
+
+int kvm_s390_pci_refresh_trans(struct kvm_vcpu *vcpu, unsigned long req,
+                   unsigned long start, unsigned long size,
+                   u8 *status)
+{
+    struct zpci_dev *zdev;
+    u32 fh = req >> 32;
+    int rc;
+
+    /* Make sure this is a valid device associated with this guest */
+    zdev = get_zdev_by_fh(fh);
+    if (!zdev || !zdev->kzdev || zdev->kzdev->kvm != vcpu->kvm) {
+        *status = 0;

Wouldn't it be interesting to add some debug information here.
When would this appear?

Yes, I agree -- One of the follow-ons I'd like to add after this series is s390dbf entries; this seems like a good spot for one.

As to when this could happen; it should not under normal circumstances, but consider something like arbitrary function handles coming from the intercepted guest instruction.  We need to ensure that the specified function 1) exists and 2) is associated with the guest issuing the refresh.


Also if we have this error this looks like we have a VM problem, shouldn't we treat this in QEMU and return -EOPNOTSUPP ?


Well, I'm not sure if we can really tell where the problem is (it could for example indicate a misbehaving guest, or a bug in our KVM tracking of hostdevs).

The guest chose the function handle, and if we got here then that means it doesn't indicate that it's an emulated device, which means either we are using the assist and KVM should handle the intercept or we are not and userspace should handle it.  But in both of those cases, there should be a host device and it should be associated with the guest.

That is right if we can not find an associated zdev = F(fh)
but the two other errors are KVM or QEMU errors AFAIU.

I don't think we know for sure for any of the cases...  For a well-behaved guest I agree with your assessment.  However, the guest decides what fh to put into its refresh instruction and so a misbehaving guest could just pick arbitrary numbers for fh and circumstantially match some other host device.  What if the guest just decided to try every single possible fh number in a loop with a refresh instruction? That's neither KVM nor QEMU's fault but can trip each of these cases.

Consider the different cases:

!zdev - Either the guest provided a bogus fh, KVM provided a bad fh via the VFIO ioctl which then QEMU fed into CLP or KVM provided the right fh via ioctl but QEMU clobbered it when providing it to the guest via CLP.

!zdev->kzdev - Either the guest provided a bogus fh that just so happened to match a host fh that has no KVM association, or KVM or QEMU screwed up somewhere (as above or because we failed to make the KVM assocation somehow)

kzdev->kvm != vcpu->kvm - Pretty much the same as above, but the matching device is actually in use by some other guest.  Again it's possible the a misbehaving guest 'got lucky' with an arbitrary fh that happened to match a host fh with an existing KVM association -- or more likely that KVM or QEMU screwed up somewhere.

OK, I understand and you are right, my error was to consider that get_zdev_by_fh() returns a zdev associated with a valid FH for the guest while it returns a zdev associated with a valid FH for the host.

If the comment would have been after the get_zdev_by_fh() and before the test I may be wouldn't have done this mistake.




I think if we decide to throw this to userspace in this event, QEMU needs some extra code to handle it (basically, if QEMU receives the intercept and the device is neither emulated nor using intercept mode then we must treat as an invalid handle as this intercept should have been handled by KVM)

I do not want to start a discussion on this, I think we can let it like this at first and come back to it when we have a good idea on how to handle this.
May be just add a /* TODO */

OK, sure.  In any of the above cases, we are certainly done in KVM anyway.  Whether there's value in passing it onto userspace vs immediately giving an error, let's think about it.

No, I do not think we should anymore.
Sorry for this wrong idea.

--
Pierre Morel
IBM Lab Boeblingen