Re: [PATCH v4 02/11] vpci: cancel pending map/unmap on vpci removal

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

On 18.11.21 17:16, Jan Beulich wrote:
> On 18.11.2021 16:11, Oleksandr Andrushchenko wrote:
>>
>> On 18.11.21 16:35, Jan Beulich wrote:
>>> On 18.11.2021 15:14, Oleksandr Andrushchenko wrote:
>>>> On 18.11.21 16:04, Roger Pau Monné wrote:
>>>>> Indeed. In the physdevop failure case this comes from an hypercall
>>>>> context, so maybe you could do the mapping in place using hypercall
>>>>> continuations if required. Not sure how complex that would be,
>>>>> compared to just deferring to guest entry point and then dealing with
>>>>> the possible cleanup on failure.
>>>> This will solve one part of the equation:
>>>>
>>>> pci_physdev_op
>>>>           pci_add_device
>>>>               init_bars -> modify_bars -> defer_map -> 
>>>> raise_softirq(SCHEDULE_SOFTIRQ)
>>>>           iommu_add_device <- FAILS
>>>>           vpci_remove_device -> xfree(pdev->vpci)
>>>>
>>>> But what about the other one, e.g. vpci_process_pending is triggered in
>>>> parallel with PCI device de-assign for example?
>>> Well, that's again in hypercall context, so using hypercall continuations
>>> may again be an option. Of course at the point a de-assign is initiated,
>>> you "only" need to drain requests (for that device, but that's unlikely
>>> to be worthwhile optimizing for), while ensuring no new requests can be
>>> issued. Again, for the device in question, but here this is relevant -
>>> a flag may want setting to refuse all further requests. Or maybe the
>>> register handling hooks may want tearing down before draining pending
>>> BAR mapping requests; without the hooks in place no new such requests
>>> can possibly appear.
>> This can be probably even easier to solve as we were talking about
>> pausing all vCPUs:
> I have to admit I'm not sure. It might be easier, but it may also be
> less desirable.
>
>> void vpci_cancel_pending(const struct pci_dev *pdev)
>> {
>>       struct domain *d = pdev->domain;
>>       struct vcpu *v;
>>       int rc;
>>
>>       while ( (rc = domain_pause_except_self(d)) == -ERESTART )
>>           cpu_relax();
>>
>>       if ( rc )
>>           printk(XENLOG_G_ERR
>>                  "Failed to pause vCPUs while canceling vPCI map/unmap for 
>> %pp %pd: %d\n",
>>                  &pdev->sbdf, pdev->domain, rc);
>>
>>       for_each_vcpu ( d, v )
>>       {
>>           if ( v->vpci.map_pending && (v->vpci.pdev == pdev) )
>>
>> This will prevent all vCPUs to run, but current, thus making it impossible
>> to run vpci_process_pending in parallel with any hypercall.
>> So, even without locking in vpci_process_pending the above should
>> be enough.
>> The only concern here is that domain_pause_except_self may return
>> the error code we somehow need to handle...
> Not just this. The -ERESTART handling isn't appropriate this way
> either.
Are you talking about cpu_relax()?
>   For the moment I can't help thinking that draining would
> be preferable over canceling.
Given that cancellation is going to happen on error path or
on device de-assign/remove I think this can be acceptable.
Any reason why not?
>
> Jan
>
Thank you,
Oleksandr