* Re: INDIRECT and NEXT [not found] <20091004143734.GB17578@redhat.com> @ 2009-10-19 2:34 ` Rusty Russell 2009-10-22 9:37 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Rusty Russell @ 2009-10-19 2:34 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization On Mon, 5 Oct 2009 01:07:34 am Michael S. Tsirkin wrote: > Hi! > I note that chaining INDIRECT descriptors with NEXT > currently is broken in lguest, because current > ring index gets overwritten. I agree this should be fixed, but not quite sure what you're referring to. I could force indirect and reproduce it, but I figure asking you for details would be more efficient :) Thanks! Rusty. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: INDIRECT and NEXT 2009-10-19 2:34 ` INDIRECT and NEXT Rusty Russell @ 2009-10-22 9:37 ` Michael S. Tsirkin 2009-10-23 3:31 ` Rusty Russell 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2009-10-22 9:37 UTC (permalink / raw) To: Rusty Russell; +Cc: virtualization On Mon, Oct 19, 2009 at 01:04:20PM +1030, Rusty Russell wrote: > On Mon, 5 Oct 2009 01:07:34 am Michael S. Tsirkin wrote: > > Hi! > > I note that chaining INDIRECT descriptors with NEXT > > currently is broken in lguest, because current > > ring index gets overwritten. > > I agree this should be fixed, but not quite sure what you're referring to. > > I could force indirect and reproduce it, but I figure asking you for details > would be more efficient :) > > Thanks! > Rusty. I refer to this code in lguest: /* * If this is an indirect entry, then this buffer contains a descriptor * table which we handle as if it's any normal descriptor chain. */ if (desc[i].flags & VRING_DESC_F_INDIRECT) { if (desc[i].len % sizeof(struct vring_desc)) errx(1, "Invalid size for indirect buffer table"); max = desc[i].len / sizeof(struct vring_desc); desc = check_pointer(desc[i].addr, desc[i].len); i = 0; } do { /* Grab the first descriptor, and check it's OK. */ iov[*out_num + *in_num].iov_len = desc[i].len; iov[*out_num + *in_num].iov_base = check_pointer(desc[i].addr, desc[i].len); /* If this is an input descriptor, increment that count. */ if (desc[i].flags & VRING_DESC_F_WRITE) (*in_num)++; else { /* * If it's an output descriptor, they're all supposed * to come before any input descriptors. */ if (*in_num) errx(1, "Descriptor has out after in"); (*out_num)++; } /* If we've got too many, that implies a descriptor loop. */ if (*out_num + *in_num > max) errx(1, "Looped descriptor"); } while ((i = next_desc(desc, i, max)) != max); Imagine an indirect entry where NEXT bit is also set. This would be useful for when we can't fit a descriptor in a single indirect entry. This won't work now because we set 'i = 0' above. A solution would be to move handling indirect entry out to a separate function. -- MST ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: INDIRECT and NEXT 2009-10-22 9:37 ` Michael S. Tsirkin @ 2009-10-23 3:31 ` Rusty Russell 2009-10-23 6:36 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Rusty Russell @ 2009-10-23 3:31 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization On Thu, 22 Oct 2009 08:07:45 pm Michael S. Tsirkin wrote: > Imagine an indirect entry where NEXT bit is also set. Hmm, so it's not obvious whether the kvm userspace code handles it correctly either. Want to hack something up to use NEXT + INDIRECT, then we can actually test it? If it doesn't work, this will have to be a new feature bit. Also, we have a limitation that you can't have more descriptors than the ring size, even with indirect, due to overzealous checks... Thanks, Rusty. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: INDIRECT and NEXT 2009-10-23 3:31 ` Rusty Russell @ 2009-10-23 6:36 ` Michael S. Tsirkin 2009-10-23 9:20 ` Rusty Russell 0 siblings, 1 reply; 6+ messages in thread From: Michael S. Tsirkin @ 2009-10-23 6:36 UTC (permalink / raw) To: Rusty Russell; +Cc: virtualization On Fri, Oct 23, 2009 at 02:01:23PM +1030, Rusty Russell wrote: > On Thu, 22 Oct 2009 08:07:45 pm Michael S. Tsirkin wrote: > > Imagine an indirect entry where NEXT bit is also set. > > Hmm, so it's not obvious whether the kvm userspace code handles it > correctly either. You mean qemu? I think it doesn't, either: code there looks basically like lguest. > Want to hack something up to use NEXT + INDIRECT, then we can actually test > it? If it doesn't work, this will have to be a new feature bit. > > Also, we have a limitation that you can't have more descriptors than the ring > size, even with indirect, due to overzealous checks... Yes ... so I wonder: do we want to fix all this and add a feature bit, or wait until some guest actually wants to use such descriptors? For vhost, I implemented INDIRECT without this limitation since it looked neater to me to have a separate function for indirect anyway. This is because direct virtqueues are virtually contigious, so I can access them just by copy from user, but indirect can be spread around and so I have to go through extra translations. > Thanks, > Rusty. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: INDIRECT and NEXT 2009-10-23 6:36 ` Michael S. Tsirkin @ 2009-10-23 9:20 ` Rusty Russell 2009-10-23 9:30 ` Michael S. Tsirkin 0 siblings, 1 reply; 6+ messages in thread From: Rusty Russell @ 2009-10-23 9:20 UTC (permalink / raw) To: Michael S. Tsirkin; +Cc: virtualization On Fri, 23 Oct 2009 05:06:48 pm Michael S. Tsirkin wrote: > On Fri, Oct 23, 2009 at 02:01:23PM +1030, Rusty Russell wrote: > > Also, we have a limitation that you can't have more descriptors than the ring > > size, even with indirect, due to overzealous checks... > > Yes ... so I wonder: do we want to fix all this and add a feature bit, > or wait until some guest actually wants to use such descriptors? Yes, we wait until someone wants it. Then (1) we have a test case, and (2) we can make sure we don't do those (bogus, frankly) >= ringsize checks. Thanks, Rusty. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: INDIRECT and NEXT 2009-10-23 9:20 ` Rusty Russell @ 2009-10-23 9:30 ` Michael S. Tsirkin 0 siblings, 0 replies; 6+ messages in thread From: Michael S. Tsirkin @ 2009-10-23 9:30 UTC (permalink / raw) To: Rusty Russell; +Cc: virtualization On Fri, Oct 23, 2009 at 07:50:31PM +1030, Rusty Russell wrote: > On Fri, 23 Oct 2009 05:06:48 pm Michael S. Tsirkin wrote: > > On Fri, Oct 23, 2009 at 02:01:23PM +1030, Rusty Russell wrote: > > > Also, we have a limitation that you can't have more descriptors than the ring > > > size, even with indirect, due to overzealous checks... > > > > Yes ... so I wonder: do we want to fix all this and add a feature bit, > > or wait until some guest actually wants to use such descriptors? > > Yes, we wait until someone wants it. Then (1) we have a test case, and > (2) we can make sure we don't do those (bogus, frankly) >= ringsize checks. OK. But for vhost, there's no reason not to do it right directly, right? guests won't depend on undefined behaviour. -- MST ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-10-23 9:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20091004143734.GB17578@redhat.com>
2009-10-19 2:34 ` INDIRECT and NEXT Rusty Russell
2009-10-22 9:37 ` Michael S. Tsirkin
2009-10-23 3:31 ` Rusty Russell
2009-10-23 6:36 ` Michael S. Tsirkin
2009-10-23 9:20 ` Rusty Russell
2009-10-23 9:30 ` Michael S. Tsirkin
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.