On Wed, 25 Apr 2018 10:26:34 +0200,ok, will put a comment on that
Oleksandr Andrushchenko wrote:
On 04/24/2018 07:23 PM, Oleksandr Andrushchenko wrote:If it's guaranteed to work, that's OK.
On 04/24/2018 06:02 PM, Takashi Iwai wrote:Well, after thinking a bit more on that and chatting on #xendevel IRC
On Tue, 24 Apr 2018 16:58:43 +0200,Great, will implement the checks this way then
Oleksandr Andrushchenko wrote:
On 04/24/2018 05:35 PM, Takashi Iwai wrote:Yep, this kind of sanity checks should work.
On Tue, 24 Apr 2018 16:29:15 +0200,no, it doesn't
Oleksandr Andrushchenko wrote:
On 04/24/2018 05:20 PM, Takashi Iwai wrote:Hm, this prevents from accessing outside the ring buffer, but does it
On Mon, 16 Apr 2018 08:24:51 +0200,In this loop I have:
Oleksandr Andrushchenko wrote:
+static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id)I'm not familiar with Xen stuff in general, but through a quick
+{
+ÂÂÂ struct xen_snd_front_evtchnl *channel = dev_id;
+ÂÂÂ struct xen_snd_front_info *front_info = channel->front_info;
+ÂÂÂ struct xensnd_resp *resp;
+ÂÂÂ RING_IDX i, rp;
+ÂÂÂ unsigned long flags;
+
+ÂÂÂ if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED))
+ÂÂÂÂÂÂÂ return IRQ_HANDLED;
+
+ÂÂÂ spin_lock_irqsave(&front_info->io_lock, flags);
+
+again:
+ÂÂÂ rp = channel->u.req.ring.sring->rsp_prod;
+ÂÂÂ /* ensure we see queued responses up to rp */
+ÂÂÂ rmb();
+
+ÂÂÂ for (i = channel->u.req.ring.rsp_cons; i != rp; i++) {
glance, this kind of code worries me a bit.
If channel->u.req.ring.rsp_cons has a bogus number, this may
lead to a
very long loop, no? Better to have a sanity check of the ring
buffer
size.
resp = RING_GET_RESPONSE(&channel->u.req.ring, i);
and the RING_GET_RESPONSE macro is designed in the way that
it wraps around when *i* in the question gets bigger than
the ring size:
#define RING_GET_REQUEST(_r, _idx)ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ \
 Â (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
So, even if the counter has a bogus number it will not last long
change the loop behavior?
Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop belowYou are right here and the comment is totally valid.
would still consume the whole 32bit counts, no?
ÂÂÂÂfor (i = channel->u.req.ring.rsp_cons; i != rp; i++) {
ÂÂÂÂÂÂÂ resp = RING_GET_RESPONSE(&channel->u.req.ring, i);
ÂÂÂÂÂÂÂ ...
ÂÂÂÂ}
I'll put an additional check like here [1] and here [2]
Will this address your comment?
with Juergen (he is on CC list), it seems that the way the code is now
it is all fine without the checks: the assumption here is that
the backend is trusted to always write sane values to the ring counters,
thus no overflow checks on frontend side are required.
Even if I implement the checks then I have no means to recover, but
just print
an error message and bail out not handling any responses.
This is probably why the checks [1] and [2] are only implemented for the
backend side and there are no such macros for the frontend side.
Takashi, please let me know if the above sounds reasonable and
addresses your comments.
But maybe it's worth to comment for readers.
Thank you,
thanks,
Takashi