Message ID | 20181211132642.3027-3-ppandit@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | rdma: various issues in rdma/pvrdma backend | expand |
On Tue, Dec 11, 2018 at 06:56:39PM +0530, P J P wrote: > From: Prasad J Pandit <pjp@fedoraproject.org> > > Define skeleton 'uar_read' routine. Avoid NULL dereference. > > Reported-by: Li Qiang <liq3ea@163.com> > Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> > --- > hw/rdma/vmw/pvrdma_main.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c > index ca5fa8d981..a6211d416d 100644 > --- a/hw/rdma/vmw/pvrdma_main.c > +++ b/hw/rdma/vmw/pvrdma_main.c > @@ -455,6 +455,11 @@ static const MemoryRegionOps regs_ops = { > }, > }; > > +static uint64_t uar_read(void *opaque, hwaddr addr, unsigned size) > +{ > + return 0; > +} > + > static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > { > PVRDMADev *dev = opaque; > @@ -496,6 +501,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) > } > > static const MemoryRegionOps uar_ops = { > + .read = uar_read, Are you sure it is needed? Looking at memory_region_dispatch_read1 i can see that there is a check but not sure this is the right place. Anyways, if it is not, i believe this should be framework responsibility. > .write = uar_write, > .endianness = DEVICE_LITTLE_ENDIAN, > .impl = { > -- > 2.19.2 >
At 2018-12-11 23:22:32, "Yuval Shaia" <yuval.shaia@oracle.com> wrote: >On Tue, Dec 11, 2018 at 06:56:39PM +0530, P J P wrote: >> From: Prasad J Pandit <pjp@fedoraproject.org> >> >> Define skeleton 'uar_read' routine. Avoid NULL dereference. >> >> Reported-by: Li Qiang <liq3ea@163.com> >> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org> >> --- >> hw/rdma/vmw/pvrdma_main.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c >> index ca5fa8d981..a6211d416d 100644 >> --- a/hw/rdma/vmw/pvrdma_main.c >> +++ b/hw/rdma/vmw/pvrdma_main.c >> @@ -455,6 +455,11 @@ static const MemoryRegionOps regs_ops = { >> }, >> }; >> >> +static uint64_t uar_read(void *opaque, hwaddr addr, unsigned size) >> +{ >> + return 0; >> +} >> + >> static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> { >> PVRDMADev *dev = opaque; >> @@ -496,6 +501,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) >> } >> >> static const MemoryRegionOps uar_ops = { >> + .read = uar_read, > >Are you sure it is needed? I'm quite sure this. The issue here is that in memory_region_dispatch_read1 if there is no mr's read callback, the 'memory_region_read_with_attrs_accessor' will be called, but in that the 'mr->ops->raed_with_attrs' has no check. In fact, I have send out a patch for the framework: -->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg02265.html But no more response. >Looking at memory_region_dispatch_read1 i can see that there is a check but >not sure this is the right place. Anyways, if it is not, i believe this >should be framework responsibility. Reference Peter's answer here: -->https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01404.html "Currently our semantics are "you must provide both read and write, even if one of them just always returns 0 / does nothing / returns an error". We could probably reasonably assert this at the point when the MemoryRegionOps is registered." Thanks, Li Qiang > >> .write = uar_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> .impl = { >> -- >> 2.19.2 >>
diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c index ca5fa8d981..a6211d416d 100644 --- a/hw/rdma/vmw/pvrdma_main.c +++ b/hw/rdma/vmw/pvrdma_main.c @@ -455,6 +455,11 @@ static const MemoryRegionOps regs_ops = { }, }; +static uint64_t uar_read(void *opaque, hwaddr addr, unsigned size) +{ + return 0; +} + static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { PVRDMADev *dev = opaque; @@ -496,6 +501,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) } static const MemoryRegionOps uar_ops = { + .read = uar_read, .write = uar_write, .endianness = DEVICE_LITTLE_ENDIAN, .impl = {