Re: vmx(4): remove useless code
On Fri, Aug 06, 2021 at 12:06:04PM +0200, Patrick Wildt wrote: > On Fri, Aug 06, 2021 at 11:05:53AM +0200, Patrick Wildt wrote: > > Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow: > > > Hi, > > > > > > The following diff removes useless code from the driver. As discussed > > > here [1] and committed there [2], the hypervisor doesn't do anything > > > with the data structures. We even just set NULL to the pointer since > > > the initial commit of vmx(4). So, I guess it better to remove all of > > > these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway. > > > > > > OK? > > > > My main concern was if the structs are getting zeroed correctly, but > > they do, so that's fine. > > > > That said, it looks like Linux sets the pointer to ~0ULL, not 0. Should > > we follow Linux' pattern there and do that as well? > > > > Thinking about it a little more, I think we should do that as well. And > maybe explicitly set driver_data_len to 0 even though it's already zero. > Basically for readability. OK? Index: dev/pci/if_vmx.c === RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v retrieving revision 1.66 diff -u -p -r1.66 if_vmx.c --- dev/pci/if_vmx.c23 Jul 2021 00:29:14 - 1.66 +++ dev/pci/if_vmx.c6 Aug 2021 12:28:51 - @@ -157,7 +157,6 @@ struct vmxnet3_softc { #define WRITE_BAR1(sc, reg, val) \ bus_space_write_4((sc)->sc_iot1, (sc)->sc_ioh1, reg, val) #define WRITE_CMD(sc, cmd) WRITE_BAR1(sc, VMXNET3_BAR1_CMD, cmd) -#define vtophys(va) 0 /* XXX ok? */ int vmxnet3_match(struct device *, void *, void *); void vmxnet3_attach(struct device *, struct device *, void *); @@ -468,8 +467,8 @@ vmxnet3_dma_init(struct vmxnet3_softc *s ds->vmxnet3_revision = 1; ds->upt_version = 1; ds->upt_features = UPT1_F_CSUM | UPT1_F_VLAN; - ds->driver_data = vtophys(sc); - ds->driver_data_len = sizeof(struct vmxnet3_softc); + ds->driver_data = ~0ULL; + ds->driver_data_len = 0; ds->queue_shared = qs_pa; ds->queue_shared_len = qs_len; ds->mtu = VMXNET3_MAX_MTU; @@ -546,8 +545,8 @@ vmxnet3_alloc_txring(struct vmxnet3_soft ts->cmd_ring_len = NTXDESC; ts->comp_ring = comp_pa; ts->comp_ring_len = NTXCOMPDESC; - ts->driver_data = vtophys(tq); - ts->driver_data_len = sizeof *tq; + ts->driver_data = ~0ULL; + ts->driver_data_len = 0; ts->intr_idx = intr; ts->stopped = 1; ts->error = 0; @@ -598,8 +597,8 @@ vmxnet3_alloc_rxring(struct vmxnet3_soft rs->cmd_ring_len[1] = NRXDESC; rs->comp_ring = comp_pa; rs->comp_ring_len = NRXCOMPDESC; - rs->driver_data = vtophys(rq); - rs->driver_data_len = sizeof *rq; + rs->driver_data = ~0ULL; + rs->driver_data_len = 0; rs->intr_idx = intr; rs->stopped = 1; rs->error = 0;
Re: vmx(4): remove useless code
Patrick Wildt wrote: > On Fri, Aug 06, 2021 at 11:05:53AM +0200, Patrick Wildt wrote: > > Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow: > > > Hi, > > > > > > The following diff removes useless code from the driver. As discussed > > > here [1] and committed there [2], the hypervisor doesn't do anything > > > with the data structures. We even just set NULL to the pointer since > > > the initial commit of vmx(4). So, I guess it better to remove all of > > > these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway. > > > > > > OK? > > > > My main concern was if the structs are getting zeroed correctly, but > > they do, so that's fine. > > > > That said, it looks like Linux sets the pointer to ~0ULL, not 0. Should > > we follow Linux' pattern there and do that as well? > > > > Thinking about it a little more, I think we should do that as well. And > maybe explicitly set driver_data_len to 0 even though it's already zero. > Basically for readability. The better approach is to zero the whole struct, then fill in non-zero fields... and fill in zero'd fields when doing so idiomaticaly helps future code readers. But if you avoid zero'ing the whole struct, then padding bytes in the struct may be a problem, and depending on the allocator and where the data is later passed to, leak. So full structure zero is the best practice.
Re: vmx(4): remove useless code
On Fri, Aug 06, 2021 at 11:05:53AM +0200, Patrick Wildt wrote: > Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow: > > Hi, > > > > The following diff removes useless code from the driver. As discussed > > here [1] and committed there [2], the hypervisor doesn't do anything > > with the data structures. We even just set NULL to the pointer since > > the initial commit of vmx(4). So, I guess it better to remove all of > > these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway. > > > > OK? > > My main concern was if the structs are getting zeroed correctly, but > they do, so that's fine. > > That said, it looks like Linux sets the pointer to ~0ULL, not 0. Should > we follow Linux' pattern there and do that as well? > Thinking about it a little more, I think we should do that as well. And maybe explicitly set driver_data_len to 0 even though it's already zero. Basically for readability. > > > bye, > > Jan > > > > [1]: https://www.lkml.org/lkml/2021/1/19/1225 > > [2]: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/vmxnet3/vmxnet3_drv.c?id=de1da8bcf40564a2adada2d5d5426e05355f66e8 > > > > Index: dev/pci/if_vmx.c > > === > > RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v > > retrieving revision 1.66 > > diff -u -p -r1.66 if_vmx.c > > --- dev/pci/if_vmx.c23 Jul 2021 00:29:14 - 1.66 > > +++ dev/pci/if_vmx.c5 Aug 2021 11:12:26 - > > @@ -157,7 +157,6 @@ struct vmxnet3_softc { > > #define WRITE_BAR1(sc, reg, val) \ > > bus_space_write_4((sc)->sc_iot1, (sc)->sc_ioh1, reg, val) > > #define WRITE_CMD(sc, cmd) WRITE_BAR1(sc, VMXNET3_BAR1_CMD, cmd) > > -#define vtophys(va) 0 /* XXX ok? */ > > > > int vmxnet3_match(struct device *, void *, void *); > > void vmxnet3_attach(struct device *, struct device *, void *); > > @@ -468,8 +467,6 @@ vmxnet3_dma_init(struct vmxnet3_softc *s > > ds->vmxnet3_revision = 1; > > ds->upt_version = 1; > > ds->upt_features = UPT1_F_CSUM | UPT1_F_VLAN; > > - ds->driver_data = vtophys(sc); > > - ds->driver_data_len = sizeof(struct vmxnet3_softc); > > ds->queue_shared = qs_pa; > > ds->queue_shared_len = qs_len; > > ds->mtu = VMXNET3_MAX_MTU; > > @@ -546,8 +543,6 @@ vmxnet3_alloc_txring(struct vmxnet3_soft > > ts->cmd_ring_len = NTXDESC; > > ts->comp_ring = comp_pa; > > ts->comp_ring_len = NTXCOMPDESC; > > - ts->driver_data = vtophys(tq); > > - ts->driver_data_len = sizeof *tq; > > ts->intr_idx = intr; > > ts->stopped = 1; > > ts->error = 0; > > @@ -598,8 +593,6 @@ vmxnet3_alloc_rxring(struct vmxnet3_soft > > rs->cmd_ring_len[1] = NRXDESC; > > rs->comp_ring = comp_pa; > > rs->comp_ring_len = NRXCOMPDESC; > > - rs->driver_data = vtophys(rq); > > - rs->driver_data_len = sizeof *rq; > > rs->intr_idx = intr; > > rs->stopped = 1; > > rs->error = 0; > > >
Re: vmx(4): remove useless code
Am Thu, Aug 05, 2021 at 02:33:01PM +0200 schrieb Jan Klemkow: > Hi, > > The following diff removes useless code from the driver. As discussed > here [1] and committed there [2], the hypervisor doesn't do anything > with the data structures. We even just set NULL to the pointer since > the initial commit of vmx(4). So, I guess it better to remove all of > these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway. > > OK? My main concern was if the structs are getting zeroed correctly, but they do, so that's fine. That said, it looks like Linux sets the pointer to ~0ULL, not 0. Should we follow Linux' pattern there and do that as well? Patrick > bye, > Jan > > [1]: https://www.lkml.org/lkml/2021/1/19/1225 > [2]: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/vmxnet3/vmxnet3_drv.c?id=de1da8bcf40564a2adada2d5d5426e05355f66e8 > > Index: dev/pci/if_vmx.c > === > RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v > retrieving revision 1.66 > diff -u -p -r1.66 if_vmx.c > --- dev/pci/if_vmx.c 23 Jul 2021 00:29:14 - 1.66 > +++ dev/pci/if_vmx.c 5 Aug 2021 11:12:26 - > @@ -157,7 +157,6 @@ struct vmxnet3_softc { > #define WRITE_BAR1(sc, reg, val) \ > bus_space_write_4((sc)->sc_iot1, (sc)->sc_ioh1, reg, val) > #define WRITE_CMD(sc, cmd) WRITE_BAR1(sc, VMXNET3_BAR1_CMD, cmd) > -#define vtophys(va) 0/* XXX ok? */ > > int vmxnet3_match(struct device *, void *, void *); > void vmxnet3_attach(struct device *, struct device *, void *); > @@ -468,8 +467,6 @@ vmxnet3_dma_init(struct vmxnet3_softc *s > ds->vmxnet3_revision = 1; > ds->upt_version = 1; > ds->upt_features = UPT1_F_CSUM | UPT1_F_VLAN; > - ds->driver_data = vtophys(sc); > - ds->driver_data_len = sizeof(struct vmxnet3_softc); > ds->queue_shared = qs_pa; > ds->queue_shared_len = qs_len; > ds->mtu = VMXNET3_MAX_MTU; > @@ -546,8 +543,6 @@ vmxnet3_alloc_txring(struct vmxnet3_soft > ts->cmd_ring_len = NTXDESC; > ts->comp_ring = comp_pa; > ts->comp_ring_len = NTXCOMPDESC; > - ts->driver_data = vtophys(tq); > - ts->driver_data_len = sizeof *tq; > ts->intr_idx = intr; > ts->stopped = 1; > ts->error = 0; > @@ -598,8 +593,6 @@ vmxnet3_alloc_rxring(struct vmxnet3_soft > rs->cmd_ring_len[1] = NRXDESC; > rs->comp_ring = comp_pa; > rs->comp_ring_len = NRXCOMPDESC; > - rs->driver_data = vtophys(rq); > - rs->driver_data_len = sizeof *rq; > rs->intr_idx = intr; > rs->stopped = 1; > rs->error = 0; >
vmx(4): remove useless code
Hi, The following diff removes useless code from the driver. As discussed here [1] and committed there [2], the hypervisor doesn't do anything with the data structures. We even just set NULL to the pointer since the initial commit of vmx(4). So, I guess it better to remove all of these. The variables are bzero'd in vmxnet3_dma_allocmem() anyway. OK? bye, Jan [1]: https://www.lkml.org/lkml/2021/1/19/1225 [2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/vmxnet3/vmxnet3_drv.c?id=de1da8bcf40564a2adada2d5d5426e05355f66e8 Index: dev/pci/if_vmx.c === RCS file: /cvs/src/sys/dev/pci/if_vmx.c,v retrieving revision 1.66 diff -u -p -r1.66 if_vmx.c --- dev/pci/if_vmx.c23 Jul 2021 00:29:14 - 1.66 +++ dev/pci/if_vmx.c5 Aug 2021 11:12:26 - @@ -157,7 +157,6 @@ struct vmxnet3_softc { #define WRITE_BAR1(sc, reg, val) \ bus_space_write_4((sc)->sc_iot1, (sc)->sc_ioh1, reg, val) #define WRITE_CMD(sc, cmd) WRITE_BAR1(sc, VMXNET3_BAR1_CMD, cmd) -#define vtophys(va) 0 /* XXX ok? */ int vmxnet3_match(struct device *, void *, void *); void vmxnet3_attach(struct device *, struct device *, void *); @@ -468,8 +467,6 @@ vmxnet3_dma_init(struct vmxnet3_softc *s ds->vmxnet3_revision = 1; ds->upt_version = 1; ds->upt_features = UPT1_F_CSUM | UPT1_F_VLAN; - ds->driver_data = vtophys(sc); - ds->driver_data_len = sizeof(struct vmxnet3_softc); ds->queue_shared = qs_pa; ds->queue_shared_len = qs_len; ds->mtu = VMXNET3_MAX_MTU; @@ -546,8 +543,6 @@ vmxnet3_alloc_txring(struct vmxnet3_soft ts->cmd_ring_len = NTXDESC; ts->comp_ring = comp_pa; ts->comp_ring_len = NTXCOMPDESC; - ts->driver_data = vtophys(tq); - ts->driver_data_len = sizeof *tq; ts->intr_idx = intr; ts->stopped = 1; ts->error = 0; @@ -598,8 +593,6 @@ vmxnet3_alloc_rxring(struct vmxnet3_soft rs->cmd_ring_len[1] = NRXDESC; rs->comp_ring = comp_pa; rs->comp_ring_len = NRXCOMPDESC; - rs->driver_data = vtophys(rq); - rs->driver_data_len = sizeof *rq; rs->intr_idx = intr; rs->stopped = 1; rs->error = 0;