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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 19.10.17 at 14:54, <andrew.cooper3@citrix.com> wrote:
> 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.

I don't understand: By never writing QINVAL_STAT_INIT, how can
multiple waits work? Afaict you'll find the variable set to
QINVAL_STAT_DONE the 2nd time you come here, and hence you
won't wait at all.

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

Still the variable itself would better also be declared volatile.

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

Right, hence my hint at this really being something the maintainer(s)
should look after.

Jan


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