Re: [PATCH for-4.10] passthrough/vtd: Don't DMA to the stack in queue_invalidate_wait()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 19/10/17 13:11, Jan Beulich wrote:
>>>> On 19.10.17 at 13:26, <andrew.cooper3@citrix.com> wrote:
>> --- a/xen/drivers/passthrough/vtd/qinval.c
>> +++ b/xen/drivers/passthrough/vtd/qinval.c
>> @@ -147,7 +147,8 @@ static int __must_check queue_invalidate_wait(struct iommu *iommu,
>>                                                u8 iflag, u8 sw, u8 fn,
>>                                                bool_t flush_dev_iotlb)
>>  {
>> -    volatile u32 poll_slot = QINVAL_STAT_INIT;
> You've lost the initializer.

Deliberately so.

>
>> +    static DEFINE_PER_CPU(u32, poll_slot);
> volatile u32

You've clipped out the bit declaring the pointer as volatile, which
suffices to retain the previous properties.

>
>> @@ -182,7 +183,7 @@ static int __must_check queue_invalidate_wait(struct iommu *iommu,
>>          timeout = NOW() + MILLISECS(flush_dev_iotlb ?
>>                                      iommu_dev_iotlb_timeout : VTD_QI_TIMEOUT);
>>  
>> -        while ( poll_slot != QINVAL_STAT_DONE )
>> +        while ( *this_poll_slot != QINVAL_STAT_DONE )
>>          {
>>              if ( NOW() > timeout )
>>              {
> Okay, you indeed improve the situation. But is that improvement
> enough?

For not corrupting the stack, yes.

> I.e. what if the write of a first (timed out) request happens
> while waiting for a subsequent one? Don't you need distinct addresses
> for every possible slot?

Certainly everything which is currently pending.

> Or alternatively isn't it high time for the
> interrupt approach to be made work (perhaps not by you, but rather
> by Intel folks)?

I'm not going to pretend that the current implementation is great, but I
really don't have time to address the other remaining swamps here.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel