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

[Pierre Morel <pmorel@linux.ibm.com>]

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