Tristan Gingold
2006-Aug-18 14:02 UTC
[Xen-devel] PATCH: multicall and auto_translated in netfront.c
Hi, If auto_translated is set (by default on ia64), netfront does multicall with not initialized entries. This patch fixes this bug. [It may be simpler to do an hypercall instead of the multicall in this case. Tell me if I should rewrite the patch this way.] Tristan. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Steven Smith
2006-Aug-18 17:30 UTC
Re: [Xen-devel] PATCH: multicall and auto_translated in netfront.c
> diff -r bef360142b62 -r ee4aef404bce linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c > --- a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c Mon Aug 14 14:21:21 2006 -0600 > +++ b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c Fri Aug 18 14:00:37 2006 +0200 > @@ -1220,13 +1220,20 @@ err: > > /* Do all the remapping work, and M2P updates, in one big hypercall. */ > if (likely(pages_done)) { > - mcl = np->rx_mcl + pages_done; > + unsigned int mcl_off; > + > + if (!xen_feature(XENFEAT_auto_translated_physmap)) > + mcl_off = pages_done; > + else > + mcl_off = 0; > +The intent of this code was that pages_done should be 0 if auto_translated_physmap was set, so this should be a no-op. It gets initialised to 0 at the top of the big loop, and is then only modified by xennet_get_responses through its mcl_offset_p pointer. xennet_get_responses only modifies it if !xen_feature(XENFEAT_auto_translated_physmap). Am I simply confused? (And, in fact, if pages_done != 0 then the mmu update you do next will be incorrect)> + mcl = np->rx_mcl + mcl_off; > mcl->op = __HYPERVISOR_mmu_update; > mcl->args[0] = (unsigned long)np->rx_mmu; > mcl->args[1] = pages_done; > mcl->args[2] = 0; > mcl->args[3] = DOMID_SELF; > - (void)HYPERVISOR_multicall(np->rx_mcl, pages_done + 1); > + (void)HYPERVISOR_multicall(np->rx_mcl, mcl_off + 1); > } > > while ((skb = __skb_dequeue(&errq)))Steven. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2006-Aug-19 09:32 UTC
Re: [Xen-devel] PATCH: multicall and auto_translated in netfront.c
On 18/8/06 6:30 pm, "Steven Smith" <sos22-xen@srcf.ucam.org> wrote:> The intent of this code was that pages_done should be 0 if > auto_translated_physmap was set, so this should be a no-op. It gets > initialised to 0 at the top of the big loop, and is then only modified > by xennet_get_responses through its mcl_offset_p pointer. > xennet_get_responses only modifies it if > !xen_feature(XENFEAT_auto_translated_physmap). Am I simply confused?That''s how the code looks to me too. In fact this means there is a bug, because driver allowance does not get properly decremented (it is incremented even for auto-translated guests in alloc_rx_buffers). I''ve checked in some cleanup as c/s 11199. It explicitly makes the multicall dependent on !auto_translated, so it should simply never be executed on ia64. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Tristan Gingold
2006-Aug-21 06:49 UTC
Re: [Xen-devel] PATCH: multicall and auto_translated in netfront.c
Le Vendredi 18 Août 2006 19:30, Steven Smith a écrit :> > diff -r bef360142b62 -r ee4aef404bce > > linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c --- > > a/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c Mon Aug 14 > > 14:21:21 2006 -0600 +++ > > b/linux-2.6-xen-sparse/drivers/xen/netfront/netfront.c Fri Aug 18 > > 14:00:37 2006 +0200 @@ -1220,13 +1220,20 @@ err: > > > > /* Do all the remapping work, and M2P updates, in one big hypercall. */ > > if (likely(pages_done)) { > > - mcl = np->rx_mcl + pages_done; > > + unsigned int mcl_off; > > + > > + if (!xen_feature(XENFEAT_auto_translated_physmap)) > > + mcl_off = pages_done; > > + else > > + mcl_off = 0; > > + > > The intent of this code was that pages_done should be 0 if > auto_translated_physmap was set, so this should be a no-op. It gets > initialised to 0 at the top of the big loop, and is then only modified > by xennet_get_responses through its mcl_offset_p pointer. > xennet_get_responses only modifies it if > !xen_feature(XENFEAT_auto_translated_physmap). Am I simply confused?I didn''t look deeply into the code. I have just viewed the result. pages_done is incremented by skb_queue_length. At first glance it does not depend on auto_translated.> (And, in fact, if pages_done != 0 then the mmu update you do next will > be incorrect)On ia64 mmu_update is no-op ;-) Tristan. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel