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

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

On 23/10/2017 08:18, Jan Beulich wrote:
>>>> On 20.10.17 at 19:55, <andrew.cooper3@citrix.com> wrote:
>> On 20/10/17 08:12, Jan Beulich wrote:
>>>>>> On 19.10.17 at 18:22, <andrew.cooper3@citrix.com> wrote:
>>>> DMA-ing to the stack is generally considered bad practice.  In this case, if 
>> a
>>>> timeout occurs because of a sluggish device which is processing the request,
>>>> the completion notification will corrupt the stack of a subsequent deeper 
>> call
>>>> tree.
>>>>
>>>> Place the poll_slot in a percpu area and DMA to that instead.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Please could you extend the commit message to state the issue
>>> remaining with using a single per-CPU slot? With that
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> How about this?
>>
>> Note: This change does not address other issues with the current
>> implementation, such as once a timeout has been suffered, subsequent
>> completions can't be correlated with their requests.
> Sounds good.
>
>>> albeit ...
>>>
>>>> @@ -167,7 +169,7 @@ static int __must_check queue_invalidate_wait(struct iommu *iommu,
>>>>      qinval_entry->q.inv_wait_dsc.lo.res_1 = 0;
>>>>      qinval_entry->q.inv_wait_dsc.lo.sdata = QINVAL_STAT_DONE;
>>>>      qinval_entry->q.inv_wait_dsc.hi.res_1 = 0;
>>>> -    qinval_entry->q.inv_wait_dsc.hi.saddr = virt_to_maddr(&poll_slot) >> 2;
>>>> +    qinval_entry->q.inv_wait_dsc.hi.saddr = virt_to_maddr(this_poll_slot) >> 2;
>>> ... this one is still a literal number rather than something allowing
>>> to associate back where that value is coming from (but since you're
>>> not introducing it here, I also won't insist on you changing it in this
>>> patch).
>> I don't understand.  What is still a literal number?
> There's still that literal 2 there as the shift count.

Ah - that is because of the bitfield definition of saddr.  It is a 32bit
field, but the bottom two bits are reserved, to cause an aligned dword
write.

It would probably be cleaner to not declare saddr as a bitfield, and
rely on the alignment of u32 to keep the bottom two bits clear.

~Andrew

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