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

[Matthew Rosato]

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.

> > 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.