[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PULL v2 2/5] hw/xen: Expose handle_bufioreq in xen_register_ioreq
On Mon, Oct 07, 2024 at 04:42:49PM +0100, Peter Maydell wrote: > On Thu, 3 Oct 2024 at 19:57, Edgar E. Iglesias <edgar.iglesias@xxxxxxxxx> > wrote: > > > > From: "Edgar E. Iglesias" <edgar.iglesias@xxxxxxx> > > > > Expose handle_bufioreq in xen_register_ioreq(). > > This is to allow machines to enable or disable buffered ioreqs. > > > > No functional change since all callers still set it to > > HVM_IOREQSRV_BUFIOREQ_ATOMIC. > > > > Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx> > > Signed-off-by: Edgar E. Iglesias <edgar.iglesias@xxxxxxx> > > Hi; Coverity has noticed a problem (CID 1563383) with this change: > > > diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c > > index 3a9d6f981b..7d2b72853b 100644 > > --- a/hw/xen/xen-hvm-common.c > > +++ b/hw/xen/xen-hvm-common.c > > @@ -667,6 +667,8 @@ static int xen_map_ioreq_server(XenIOState *state) > > xen_pfn_t ioreq_pfn; > > xen_pfn_t bufioreq_pfn; > > evtchn_port_t bufioreq_evtchn; > > In this function bufioreq_evtchn is declared uninitialized... > > > + unsigned long num_frames = 1; > > + unsigned long frame = 1; > > int rc; > > > > /* > > @@ -675,59 +677,78 @@ static int xen_map_ioreq_server(XenIOState *state) > > */ > > QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_bufioreq != 0); > > QEMU_BUILD_BUG_ON(XENMEM_resource_ioreq_server_frame_ioreq(0) != 1); > > + > > + if (state->has_bufioreq) { > > + frame = 0; > > + num_frames = 2; > > + } > > state->fres = xenforeignmemory_map_resource(xen_fmem, xen_domid, > > XENMEM_resource_ioreq_server, > > - state->ioservid, 0, 2, > > + state->ioservid, > > + frame, num_frames, > > &addr, > > PROT_READ | PROT_WRITE, 0); > > if (state->fres != NULL) { > > trace_xen_map_resource_ioreq(state->ioservid, addr); > > - state->buffered_io_page = addr; > > - state->shared_page = addr + XC_PAGE_SIZE; > > + state->shared_page = addr; > > + if (state->has_bufioreq) { > > + state->buffered_io_page = addr; > > + state->shared_page = addr + XC_PAGE_SIZE; > > + } > > } else if (errno != EOPNOTSUPP) { > > error_report("failed to map ioreq server resources: error %d > > handle=%p", > > errno, xen_xc); > > return -1; > > } > > > > - rc = xen_get_ioreq_server_info(xen_domid, state->ioservid, > > - (state->shared_page == NULL) ? > > - &ioreq_pfn : NULL, > > - (state->buffered_io_page == NULL) ? > > - &bufioreq_pfn : NULL, > > - &bufioreq_evtchn); > > ...which was OK prior to this change, because (ignoring the > early-exit case) we would always pass through this function > call, which initializes bufioreq_evtchn... > > > - if (rc < 0) { > > - error_report("failed to get ioreq server info: error %d handle=%p", > > - errno, xen_xc); > > - return rc; > > - } > > + /* > > + * If we fail to map the shared page with > > xenforeignmemory_map_resource() > > + * or if we're using buffered ioreqs, we need > > xen_get_ioreq_server_info() > > + * to provide the the addresses to map the shared page and/or to get > > the > > + * event-channel port for buffered ioreqs. > > + */ > > + if (state->shared_page == NULL || state->has_bufioreq) { > > + rc = xen_get_ioreq_server_info(xen_domid, state->ioservid, > > + (state->shared_page == NULL) ? > > + &ioreq_pfn : NULL, > > + (state->has_bufioreq && > > + state->buffered_io_page == NULL) ? > > + &bufioreq_pfn : NULL, > > + &bufioreq_evtchn); > > ...but now the initialization has moved inside an if() so it only > happens under certain conditions... > > > + if (rc < 0) { > > + error_report("failed to get ioreq server info: error %d > > handle=%p", > > + errno, xen_xc); > > + return rc; > > + } > > > > - if (state->shared_page == NULL) { > > - trace_xen_map_ioreq_server_shared_page(ioreq_pfn); > > + if (state->shared_page == NULL) { > > + trace_xen_map_ioreq_server_shared_page(ioreq_pfn); > > > > - state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid, > > - PROT_READ | PROT_WRITE, > > - 1, &ioreq_pfn, NULL); > > + state->shared_page = xenforeignmemory_map(xen_fmem, xen_domid, > > + PROT_READ | > > PROT_WRITE, > > + 1, &ioreq_pfn, NULL); > > + } > > if (state->shared_page == NULL) { > > error_report("map shared IO page returned error %d handle=%p", > > errno, xen_xc); > > } > > - } > > > > - if (state->buffered_io_page == NULL) { > > - trace_xen_map_ioreq_server_buffered_io_page(bufioreq_pfn); > > + if (state->has_bufioreq && state->buffered_io_page == NULL) { > > + trace_xen_map_ioreq_server_buffered_io_page(bufioreq_pfn); > > > > - state->buffered_io_page = xenforeignmemory_map(xen_fmem, xen_domid, > > - PROT_READ | > > PROT_WRITE, > > - 1, &bufioreq_pfn, > > - NULL); > > - if (state->buffered_io_page == NULL) { > > - error_report("map buffered IO page returned error %d", errno); > > - return -1; > > + state->buffered_io_page = xenforeignmemory_map(xen_fmem, > > xen_domid, > > + PROT_READ | > > PROT_WRITE, > > + 1, &bufioreq_pfn, > > + NULL); > > + if (state->buffered_io_page == NULL) { > > + error_report("map buffered IO page returned error %d", > > errno); > > + return -1; > > + } > > } > > } > > > > - if (state->shared_page == NULL || state->buffered_io_page == NULL) { > > + if (state->shared_page == NULL || > > + (state->has_bufioreq && state->buffered_io_page == NULL)) { > > return -1; > > } > > ...and the tail end of the function has not been modified, so > (not visible in this diff context) when we do: > > trace_xen_map_ioreq_server_buffered_io_evtchn(bufioreq_evtchn); > > state->bufioreq_remote_port = bufioreq_evtchn; > > return 0; > > we may be using it uninitialized (in the trace statement > and when assigning it to state->bufioreq_remote_port). > > Could you have a look at this and send a fix, please? > Thanks Peter, I sent a fix for this yesterday. Best regards, Edgar
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |