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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 20.10.17 at 19:55, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 20/10/17 08:12, Jan Beulich wrote:
>>>>> On 19.10.17 at 18:22, <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>
>> 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@xxxxxxxx>
> 
> 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.

Jan


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