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

Pierre Morel
2022-01-20T09:45:56Z

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