* [PATCH 0/5] Skip unneeded VMENTRY & VMEXIT @ 2015-01-30 0:52 Don Slutz 2015-01-30 0:52 ` [PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO Don Slutz ` (4 more replies) 0 siblings, 5 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 0:52 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, Don Slutz, Paul Durrant, Jan Beulich, Wei Liu Using a new enough QEMU that has: commit 7665d6ba98e20fb05c420de947c1750fd47e5c07 Author: Paul Durrant <paul.durrant@citrix.com> Date: Tue Jan 20 11:06:19 2015 +0000 Xen: Use the ioreq-server API when available means that hvmloader and the guest will do pci config accesses that will not be sent to QEMU. However hvm_complete_assist_req() (which is the routine that does the work) returns a 1 which hvm_send_assist_req() returns to the caller which means that the request was sent to QEMU and need to be waited for. This is not the case. hvm_complete_assist_req() has called on hvm_io_assist() which has changed the io_state from HVMIO_awaiting_completion to HVMIO_completed if needed. But hvmemul_do_io() thinks that it needs to wait on the IOREQ_READ case, and returns X86EMUL_RETRY. [PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO -- No need to apply [PATCH 2/5] xentrace: Adjust IOPORT & MMIO format -- Simple bugfix. xentrace_format is not converting these correctly [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do -- Simple bugfix. Do the same thing as when hvm_has_dm() returns a zero. [PATCH 4/5] hvm_complete_assist_req: We should not be able to get -- This ASSERT was sugested by Paul Durrant. Since I was in the file, just add it. [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send -- The real fix. Does depend on [PATCH 3/5] hvmemul_do_io: If the send ... Here is the before xentrace output using the "[PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO" (which I do not expect to be applied. It is first because that is the order you need to use to reproduce the xentrace output): CPU0 685992932190 (+ 2292) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU0 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 5 ret = 0 ] CPU0 685992934938 (+ 2748) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU0 685992935526 (+ 588) VMENTRY CPU0 685992937650 (+ 2124) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU0 0 (+ 0) IOPORT_READ [ port = 0x0cfe, data = 0x00000000 ] CPU0 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 1 ] CPU0 685992940248 (+ 2598) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU0 685992940968 (+ 720) VMENTRY And after: CPU2 1028654638584 (+ 2388) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU2 0 (+ 0) IOPORT_READ [ port = 0x0cfe, data = 0xffffffff ] CPU2 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 1 ] CPU2 1028654641932 (+ 3348) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU2 1028654642586 (+ 654) VMENTRY Don Slutz (5): DEBUG: xentrace: Add debug of HANDLE_PIO xentrace: Adjust IOPORT & MMIO format hvmemul_do_io: If the send to the ioreq server failed do not retry. hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG hvm_complete_assist_req: Tell caller we failed to send tools/xentrace/formats | 10 ++++++---- xen/arch/x86/hvm/emulate.c | 4 ++++ xen/arch/x86/hvm/hvm.c | 3 ++- xen/arch/x86/hvm/io.c | 4 ++++ xen/include/asm-x86/hvm/trace.h | 2 +- xen/include/public/trace.h | 2 ++ 6 files changed, 19 insertions(+), 6 deletions(-) -- 1.8.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO 2015-01-30 0:52 [PATCH 0/5] Skip unneeded VMENTRY & VMEXIT Don Slutz @ 2015-01-30 0:52 ` Don Slutz 2015-01-30 0:52 ` [PATCH 2/5] xentrace: Adjust IOPORT & MMIO format Don Slutz ` (3 subsequent siblings) 4 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 0:52 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, Don Slutz, Paul Durrant, Jan Beulich, Wei Liu This is not needed. However I provide it as it is used to generate data in the cover letter. Signed-off-by: Don Slutz <dslutz@verizon.com> --- tools/xentrace/formats | 2 ++ xen/arch/x86/hvm/io.c | 4 ++++ xen/include/asm-x86/hvm/trace.h | 2 +- xen/include/public/trace.h | 2 ++ 4 files changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/xentrace/formats b/tools/xentrace/formats index da658bf..089022a 100644 --- a/tools/xentrace/formats +++ b/tools/xentrace/formats @@ -80,6 +80,8 @@ 0x00082021 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) NPF [ gpa = 0x%(2)08x%(1)08x mfn = 0x%(4)08x%(3)08x qual = 0x%(5)04x p2mt = 0x%(6)04x ] 0x00082023 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) TRAP [ vector = 0x%(1)02x ] +0x00082040 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) HANDLE_PIO [ port = 0x%(1)04x size = %(2)d dir = %(3)d io_state = %(4)d ret = %(5)d ] + 0x0010f001 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_map [ domid = %(1)d ] 0x0010f002 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_unmap [ domid = %(1)d ] 0x0010f003 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) page_grant_transfer [ domid = %(1)d ] diff --git a/xen/arch/x86/hvm/io.c b/xen/arch/x86/hvm/io.c index 68fb890..8584d96 100644 --- a/xen/arch/x86/hvm/io.c +++ b/xen/arch/x86/hvm/io.c @@ -155,7 +155,10 @@ int handle_pio(uint16_t port, unsigned int size, int dir) break; case X86EMUL_RETRY: if ( vio->io_state != HVMIO_awaiting_completion ) + { + HVMTRACE_5D(HANDLE_PIO, port, size, dir, vio->io_state, 0); return 0; + } /* Completion in hvm_io_assist() with no re-emulation required. */ ASSERT(dir == IOREQ_READ); vio->io_state = HVMIO_handle_pio_awaiting_completion; @@ -166,6 +169,7 @@ int handle_pio(uint16_t port, unsigned int size, int dir) break; } + HVMTRACE_5D(HANDLE_PIO, port, size, dir, vio->io_state, 1); return 1; } diff --git a/xen/include/asm-x86/hvm/trace.h b/xen/include/asm-x86/hvm/trace.h index de802a6..32af344 100644 --- a/xen/include/asm-x86/hvm/trace.h +++ b/xen/include/asm-x86/hvm/trace.h @@ -54,7 +54,7 @@ #define DO_TRC_HVM_TRAP DEFAULT_HVM_MISC #define DO_TRC_HVM_TRAP_DEBUG DEFAULT_HVM_MISC #define DO_TRC_HVM_VLAPIC DEFAULT_HVM_MISC - +#define DO_TRC_HVM_HANDLE_PIO DEFAULT_HVM_MISC #define TRC_PAR_LONG(par) ((par)&0xFFFFFFFF),((par)>>32) diff --git a/xen/include/public/trace.h b/xen/include/public/trace.h index 5211ae7..5703551 100644 --- a/xen/include/public/trace.h +++ b/xen/include/public/trace.h @@ -228,6 +228,8 @@ #define TRC_HVM_TRAP_DEBUG (TRC_HVM_HANDLER + 0x24) #define TRC_HVM_VLAPIC (TRC_HVM_HANDLER + 0x25) +#define TRC_HVM_HANDLE_PIO (TRC_HVM_HANDLER + 0x40) + #define TRC_HVM_IOPORT_WRITE (TRC_HVM_HANDLER + 0x216) #define TRC_HVM_IOMEM_WRITE (TRC_HVM_HANDLER + 0x217) -- 1.8.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/5] xentrace: Adjust IOPORT & MMIO format 2015-01-30 0:52 [PATCH 0/5] Skip unneeded VMENTRY & VMEXIT Don Slutz 2015-01-30 0:52 ` [PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO Don Slutz @ 2015-01-30 0:52 ` Don Slutz 2015-01-30 0:52 ` [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry Don Slutz ` (2 subsequent siblings) 4 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 0:52 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, Don Slutz, Paul Durrant, Jan Beulich, Wei Liu The 1st item is not data, but the port (address). The 2nd item is the data. Signed-off-by: Don Slutz <dslutz@verizon.com> --- tools/xentrace/formats | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tools/xentrace/formats b/tools/xentrace/formats index 089022a..51b652c 100644 --- a/tools/xentrace/formats +++ b/tools/xentrace/formats @@ -68,10 +68,10 @@ 0x00082014 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) INVLPG [ is invlpga? = %(1)d, virt = 0x%(2)08x ] 0x00082114 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) INVLPG [ is invlpga? = %(1)d, virt = 0x%(3)08x%(2)08x ] 0x00082015 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) MCE -0x00082016 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) IOPORT_READ [ data = 0x%(1)04x ] -0x00082216 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) IOPORT_WRITE [ data = 0x%(1)04x ] -0x00082017 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) MMIO_READ [ data = 0x%(1)04x ] -0x00082217 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) MMIO_WRITE [ data = 0x%(1)04x ] +0x00082016 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) IOPORT_READ [ port = 0x%(1)04x, data = 0x%(2)08x ] +0x00082216 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) IOPORT_WRITE [ port = 0x%(1)04x, data = 0x%(2)08x ] +0x00082017 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) MMIO_READ [ port = 0x%(1)08x, data = 0x%(2)08x ] +0x00082217 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) MMIO_WRITE [ port = 0x%(1)08x, data = 0x%(2)08x ] 0x00082018 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) CLTS 0x00082019 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) LMSW [ value = 0x%(1)08x ] 0x00082119 CPU%(cpu)d %(tsc)d (+%(reltsc)8d) LMSW [ value = 0x%(2)08x%(1)08x ] -- 1.8.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-01-30 0:52 [PATCH 0/5] Skip unneeded VMENTRY & VMEXIT Don Slutz 2015-01-30 0:52 ` [PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO Don Slutz 2015-01-30 0:52 ` [PATCH 2/5] xentrace: Adjust IOPORT & MMIO format Don Slutz @ 2015-01-30 0:52 ` Don Slutz 2015-01-30 10:23 ` Jan Beulich 2015-01-30 10:37 ` Paul Durrant 2015-01-30 0:52 ` [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG Don Slutz 2015-01-30 0:52 ` [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send Don Slutz 4 siblings, 2 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 0:52 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, Don Slutz, Paul Durrant, Jan Beulich, Wei Liu I.E. do just what no backing DM does. Signed-off-by: Don Slutz <dslutz@verizon.com> --- xen/arch/x86/hvm/emulate.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 2ed4344..e9fc070 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -228,7 +228,11 @@ static int hvmemul_do_io( { rc = X86EMUL_RETRY; if ( !hvm_send_assist_req(&p) ) + { + /* Since the send failed, do not retry */ + rc = X86EMUL_OKAY; vio->io_state = HVMIO_none; + } else if ( p_data == NULL ) rc = X86EMUL_OKAY; } -- 1.8.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-01-30 0:52 ` [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry Don Slutz @ 2015-01-30 10:23 ` Jan Beulich 2015-01-30 18:17 ` Don Slutz 2015-01-30 10:37 ` Paul Durrant 1 sibling, 1 reply; 26+ messages in thread From: Jan Beulich @ 2015-01-30 10:23 UTC (permalink / raw) To: Don Slutz Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Paul Durrant, Keir Fraser >>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote: > I.E. do just what no backing DM does. _If_ this is correct, the if() modified here should be folded with the one a few lines up. But looking at the description of the commit that introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an instruction emulation...", almost immediately modified by f20f3c8ece "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt this is really what we want, or at the very least your change description should explain what was wrong with the original commit. Jan > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -228,7 +228,11 @@ static int hvmemul_do_io( > { > rc = X86EMUL_RETRY; > if ( !hvm_send_assist_req(&p) ) > + { > + /* Since the send failed, do not retry */ > + rc = X86EMUL_OKAY; > vio->io_state = HVMIO_none; > + } > else if ( p_data == NULL ) > rc = X86EMUL_OKAY; > } > -- > 1.8.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-01-30 10:23 ` Jan Beulich @ 2015-01-30 18:17 ` Don Slutz 2015-01-30 19:19 ` Don Slutz 0 siblings, 1 reply; 26+ messages in thread From: Don Slutz @ 2015-01-30 18:17 UTC (permalink / raw) To: Jan Beulich, Don Slutz, Paul Durrant Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Paul Durrant, Keir Fraser On 01/30/15 05:23, Jan Beulich wrote: >>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote: >> I.E. do just what no backing DM does. > > _If_ this is correct, the if() modified here should be folded with the > one a few lines up. Ok, will fold with the one a few lines up. As expected without this change the guest will hang (more details below). > But looking at the description of the commit that > introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an > instruction emulation...", almost immediately modified by f20f3c8ece > "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt > this is really what we want, or at the very least your change > description should explain what was wrong with the original commit. > Looking at these 2 commits, it looks to me like I need to handle these cases also. So it looks like having hvm_send_assist_req() return an int 0, 1, & 2 is the simpler way. V2 on the way soon. Which is the better codding style: 1) Add #defines for the return values? 2) Just use numbers? 3) Invert the sense 0 means worked, 1 is shutdown_deferral or domain_crash, 2 is hvm_complete_assist_req()? I.E. (for just adding 2): diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 2ed4344..c565151 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -227,10 +227,19 @@ static int hvmemul_do_io( else { rc = X86EMUL_RETRY; - if ( !hvm_send_assist_req(&p) ) - vio->io_state = HVMIO_none; - else if ( p_data == NULL ) + switch ( hvm_send_assist_req(&p) ) + { + case 2: rc = X86EMUL_OKAY; + /* fall through */ + case 0: + vio->io_state = HVMIO_none; + break; + case 1: + if ( p_data == NULL ) + rc = X86EMUL_OKAY; + break; + } } ??? I would think it would be good for the code using hvm_has_dm() to also call on hvm_complete_assist_req(). hvm_has_dm() is a subset of the cases that hvm_select_ioreq_server() checks for. Based on this, I think it would be better to remove the call on hvm_has_dm() instead of adding a call to hvm_complete_assist_req(). -Don Slutz P.S. Details for hang: Using: static bool_t hvm_complete_assist_req(ioreq_t *p) { + HVMTRACE_1D(COMPLETE_ASSIST, p->type); ... ): I get the trace: CPU1 745455716325 (+ 1362) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 0 ] CPU1 745455717846 (+ 1521) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU1 745455718209 (+ 363) VMENTRY CPU1 745455719568 (+ 1359) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 0 ] CPU1 745455721083 (+ 1515) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU1 745455721422 (+ 339) VMENTRY CPU1 745455722781 (+ 1359) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 0 ] CPU1 745455724299 (+ 1518) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU1 745455724656 (+ 357) VMENTRY CPU1 745455726009 (+ 1353) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 0 ] CPU1 745455727539 (+ 1530) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU1 745455727899 (+ 360) VMENTRY CPU1 745455729276 (+ 1377) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 0 ] CPU1 745455730803 (+ 1527) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU1 745455731163 (+ 360) VMENTRY CPU1 745455732525 (+ 1362) VMEXIT [ exitcode = 0x0000001e, rIP = 0x00101581 ] CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 io_state = 0 ret = 0 ] CPU1 745455734049 (+ 1524) vlapic_accept_pic_intr [ i8259_target = 1, accept_pic_int = 1 ] CPU1 745455734385 (+ 336) VMENTRY ... > Jan > >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -228,7 +228,11 @@ static int hvmemul_do_io( >> { >> rc = X86EMUL_RETRY; >> if ( !hvm_send_assist_req(&p) ) >> + { >> + /* Since the send failed, do not retry */ >> + rc = X86EMUL_OKAY; >> vio->io_state = HVMIO_none; >> + } >> else if ( p_data == NULL ) >> rc = X86EMUL_OKAY; >> } >> -- >> 1.8.4 > > > ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-01-30 18:17 ` Don Slutz @ 2015-01-30 19:19 ` Don Slutz 2015-02-02 8:36 ` Jan Beulich 2015-02-02 9:51 ` Paul Durrant 0 siblings, 2 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 19:19 UTC (permalink / raw) To: Don Slutz, Jan Beulich, Paul Durrant Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Keir Fraser On 01/30/15 13:17, Don Slutz wrote: > On 01/30/15 05:23, Jan Beulich wrote: >>>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote: >>> I.E. do just what no backing DM does. >> >> _If_ this is correct, the if() modified here should be folded with the >> one a few lines up. > > Ok, will fold with the one a few lines up. As expected without this > change the guest will hang (more details below). > > >> But looking at the description of the commit that >> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an >> instruction emulation...", almost immediately modified by f20f3c8ece >> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt >> this is really what we want, or at the very least your change >> description should explain what was wrong with the original commit. >> > > Looking at these 2 commits, it looks to me like I need to handle these > cases also. So it looks like having hvm_send_assist_req() return an int > 0, 1, & 2 is the simpler way. V2 on the way soon. > Soon after sending this, I came up with a 2nd way for fix. Change hvm_has_dm() to use hvm_select_ioreq_server(). Then the correct answer will be found, and so will not retry. To avoid 2 calls to hvm_select_ioreq_server(), it makes snes to me to return s. Also adding a call to hvm_complete_assist_req() would help. So based on all this is is a possible v2: commit 24eb5a839427ba80c1adf16ab656c19729f906be Author: Don Slutz <dslutz@verizon.com> Date: Fri Jan 30 13:54:43 2015 -0500 fixup use of hvm_send_assist_req Signed-off-by: Don Slutz <dslutz@verizon.com> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c index 2ed4344..7c3c654 100644 --- a/xen/arch/x86/hvm/emulate.c +++ b/xen/arch/x86/hvm/emulate.c @@ -218,8 +218,11 @@ static int hvmemul_do_io( vio->io_state = HVMIO_handle_mmio_awaiting_completion; break; case X86EMUL_UNHANDLEABLE: + { + void *s = hvm_has_dm(curr->domain, &p); + /* If there is no backing DM, just ignore accesses */ - if ( !hvm_has_dm(curr->domain) ) + if ( !s ) { rc = X86EMUL_OKAY; vio->io_state = HVMIO_none; @@ -227,11 +230,12 @@ static int hvmemul_do_io( else { rc = X86EMUL_RETRY; - if ( !hvm_send_assist_req(&p) ) + if ( !hvm_send_assist_req(s, &p) ) vio->io_state = HVMIO_none; else if ( p_data == NULL ) rc = X86EMUL_OKAY; } + } break; default: BUG(); diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 984af81..21d4a73 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p) return 1; } -bool_t hvm_has_dm(struct domain *d) +static bool_t hvm_complete_assist_req(ioreq_t *p) { - return !list_empty(&d->arch.hvm_domain.ioreq_server.list); + HVMTRACE_1D(COMPLETE_ASSIST, p->type); + ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); + switch ( p->type ) + { + case IOREQ_TYPE_COPY: + case IOREQ_TYPE_PIO: + if ( p->dir == IOREQ_READ ) + { + if ( !p->data_is_ptr ) + p->data = ~0ul; + else + { + int i, step = p->df ? -p->size : p->size; + uint32_t data = ~0; + + for ( i = 0; i < p->count; i++ ) + hvm_copy_to_guest_phys(p->data + step * i, &data, + p->size); + } + } + /* FALLTHRU */ + default: + p->state = STATE_IORESP_READY; + hvm_io_assist(p); + break; + } + + return 1; +} + +void *hvm_has_dm(struct domain *d, ioreq_t *p) +{ + struct hvm_ioreq_server *s = hvm_select_ioreq_server(d, p); + + if ( !s ) + hvm_complete_assist_req(p); + return (void *)s; } bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, @@ -2571,44 +2607,15 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, return 0; } -static bool_t hvm_complete_assist_req(ioreq_t *p) -{ - HVMTRACE_1D(COMPLETE_ASSIST, p->type); - ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); - switch ( p->type ) - { - case IOREQ_TYPE_COPY: - case IOREQ_TYPE_PIO: - if ( p->dir == IOREQ_READ ) - { - if ( !p->data_is_ptr ) - p->data = ~0ul; - else - { - int i, step = p->df ? -p->size : p->size; - uint32_t data = ~0; - - for ( i = 0; i < p->count; i++ ) - hvm_copy_to_guest_phys(p->data + step * i, &data, - p->size); - } - } - /* FALLTHRU */ - default: - p->state = STATE_IORESP_READY; - hvm_io_assist(p); - break; - } - - return 0; /* implicitly bins the i/o operation */ -} - -bool_t hvm_send_assist_req(ioreq_t *p) +bool_t hvm_send_assist_req(void *vs, ioreq_t *p) { - struct hvm_ioreq_server *s = hvm_select_ioreq_server(current->domain, p); + struct hvm_ioreq_server *s = vs; if ( !s ) - return hvm_complete_assist_req(p); + { + hvm_complete_assist_req(p); + return 1; + } return hvm_send_assist_req_to_ioreq_server(s, p); } diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h index e3d2d9a..1923842 100644 --- a/xen/include/asm-x86/hvm/hvm.h +++ b/xen/include/asm-x86/hvm/hvm.h @@ -228,7 +228,7 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v); void hvm_vcpu_cacheattr_destroy(struct vcpu *v); void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip); -bool_t hvm_send_assist_req(ioreq_t *p); +bool_t hvm_send_assist_req(void *s, ioreq_t *p); void hvm_broadcast_assist_req(ioreq_t *p); void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat); @@ -359,7 +359,7 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx, void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, unsigned int *ecx, unsigned int *edx); void hvm_migrate_timers(struct vcpu *v); -bool_t hvm_has_dm(struct domain *d); +void *hvm_has_dm(struct domain *d, ioreq_t *p); bool_t hvm_io_pending(struct vcpu *v); void hvm_do_resume(struct vcpu *v); void hvm_migrate_pirqs(struct vcpu *v); -Don Slutz > Which is the better codding style: > > 1) Add #defines for the return values? > 2) Just use numbers? > 3) Invert the sense 0 means worked, 1 is shutdown_deferral or > domain_crash, 2 is hvm_complete_assist_req()? > > > I.E. (for just adding 2): > > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 2ed4344..c565151 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -227,10 +227,19 @@ static int hvmemul_do_io( > else > { > rc = X86EMUL_RETRY; > - if ( !hvm_send_assist_req(&p) ) > - vio->io_state = HVMIO_none; > - else if ( p_data == NULL ) > + switch ( hvm_send_assist_req(&p) ) > + { > + case 2: > rc = X86EMUL_OKAY; > + /* fall through */ > + case 0: > + vio->io_state = HVMIO_none; > + break; > + case 1: > + if ( p_data == NULL ) > + rc = X86EMUL_OKAY; > + break; > + } > } > > > ??? > > > > I would think it would be good for the code using hvm_has_dm() > to also call on hvm_complete_assist_req(). hvm_has_dm() is a subset of > the cases that hvm_select_ioreq_server() checks for. > > Based on this, I think it would be better to remove the call on > hvm_has_dm() instead of adding a call to hvm_complete_assist_req(). > > > -Don Slutz > P.S. Details for hang: > > Using: > > static bool_t hvm_complete_assist_req(ioreq_t *p) > { > + HVMTRACE_1D(COMPLETE_ASSIST, p->type); > ... > ): > > I get the trace: > > CPU1 745455716325 (+ 1362) VMEXIT [ exitcode = 0x0000001e, rIP > = 0x00101581 ] > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > io_state = 0 ret = 0 ] > CPU1 745455717846 (+ 1521) vlapic_accept_pic_intr [ i8259_target = > 1, accept_pic_int = 1 ] > CPU1 745455718209 (+ 363) VMENTRY > CPU1 745455719568 (+ 1359) VMEXIT [ exitcode = 0x0000001e, rIP > = 0x00101581 ] > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > io_state = 0 ret = 0 ] > CPU1 745455721083 (+ 1515) vlapic_accept_pic_intr [ i8259_target = > 1, accept_pic_int = 1 ] > CPU1 745455721422 (+ 339) VMENTRY > CPU1 745455722781 (+ 1359) VMEXIT [ exitcode = 0x0000001e, rIP > = 0x00101581 ] > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > io_state = 0 ret = 0 ] > CPU1 745455724299 (+ 1518) vlapic_accept_pic_intr [ i8259_target = > 1, accept_pic_int = 1 ] > CPU1 745455724656 (+ 357) VMENTRY > CPU1 745455726009 (+ 1353) VMEXIT [ exitcode = 0x0000001e, rIP > = 0x00101581 ] > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > io_state = 0 ret = 0 ] > CPU1 745455727539 (+ 1530) vlapic_accept_pic_intr [ i8259_target = > 1, accept_pic_int = 1 ] > CPU1 745455727899 (+ 360) VMENTRY > CPU1 745455729276 (+ 1377) VMEXIT [ exitcode = 0x0000001e, rIP > = 0x00101581 ] > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > io_state = 0 ret = 0 ] > CPU1 745455730803 (+ 1527) vlapic_accept_pic_intr [ i8259_target = > 1, accept_pic_int = 1 ] > CPU1 745455731163 (+ 360) VMENTRY > CPU1 745455732525 (+ 1362) VMEXIT [ exitcode = 0x0000001e, rIP > = 0x00101581 ] > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > io_state = 0 ret = 0 ] > CPU1 745455734049 (+ 1524) vlapic_accept_pic_intr [ i8259_target = > 1, accept_pic_int = 1 ] > CPU1 745455734385 (+ 336) VMENTRY > ... > > >> Jan >> >>> --- a/xen/arch/x86/hvm/emulate.c >>> +++ b/xen/arch/x86/hvm/emulate.c >>> @@ -228,7 +228,11 @@ static int hvmemul_do_io( >>> { >>> rc = X86EMUL_RETRY; >>> if ( !hvm_send_assist_req(&p) ) >>> + { >>> + /* Since the send failed, do not retry */ >>> + rc = X86EMUL_OKAY; >>> vio->io_state = HVMIO_none; >>> + } >>> else if ( p_data == NULL ) >>> rc = X86EMUL_OKAY; >>> } >>> -- >>> 1.8.4 >> >> >> ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-01-30 19:19 ` Don Slutz @ 2015-02-02 8:36 ` Jan Beulich 2015-02-02 13:53 ` Don Slutz 2015-02-02 9:51 ` Paul Durrant 1 sibling, 1 reply; 26+ messages in thread From: Jan Beulich @ 2015-02-02 8:36 UTC (permalink / raw) To: Paul Durrant, Don Slutz Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Keir Fraser >>> On 30.01.15 at 20:19, <dslutz@verizon.com> wrote: > Soon after sending this, I came up with a 2nd way for fix. > Change hvm_has_dm() to use hvm_select_ioreq_server(). Then > the correct answer will be found, and so will not retry. > > To avoid 2 calls to hvm_select_ioreq_server(), it makes > snes to me to return s. Also adding a call to hvm_complete_assist_req() > would help. So based on all this is is a possible v2: Looks reasonable (awaiting Paul's opinion though) except for mechanical aspects: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p) > return 1; > } > > -bool_t hvm_has_dm(struct domain *d) > +static bool_t hvm_complete_assist_req(ioreq_t *p) > { > - return !list_empty(&d->arch.hvm_domain.ioreq_server.list); > + HVMTRACE_1D(COMPLETE_ASSIST, p->type); > + ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); > + switch ( p->type ) > + { > + case IOREQ_TYPE_COPY: > + case IOREQ_TYPE_PIO: > + if ( p->dir == IOREQ_READ ) > + { > + if ( !p->data_is_ptr ) > + p->data = ~0ul; > + else > + { > + int i, step = p->df ? -p->size : p->size; > + uint32_t data = ~0; > + > + for ( i = 0; i < p->count; i++ ) > + hvm_copy_to_guest_phys(p->data + step * i, &data, > + p->size); > + } > + } > + /* FALLTHRU */ > + default: > + p->state = STATE_IORESP_READY; > + hvm_io_assist(p); > + break; > + } > + > + return 1; > +} > + > +void *hvm_has_dm(struct domain *d, ioreq_t *p) I can't see why this needs to return void * instead of a properly typed pointer. > @@ -2571,44 +2607,15 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, > return 0; > } > > -static bool_t hvm_complete_assist_req(ioreq_t *p) > -{ > - HVMTRACE_1D(COMPLETE_ASSIST, p->type); > - ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); > - switch ( p->type ) > - { > - case IOREQ_TYPE_COPY: > - case IOREQ_TYPE_PIO: > - if ( p->dir == IOREQ_READ ) > - { > - if ( !p->data_is_ptr ) > - p->data = ~0ul; > - else > - { > - int i, step = p->df ? -p->size : p->size; > - uint32_t data = ~0; > - > - for ( i = 0; i < p->count; i++ ) > - hvm_copy_to_guest_phys(p->data + step * i, &data, > - p->size); > - } > - } > - /* FALLTHRU */ > - default: > - p->state = STATE_IORESP_READY; > - hvm_io_assist(p); > - break; > - } > - > - return 0; /* implicitly bins the i/o operation */ > -} Moving hvm_has_dm() down here would make the patch smaller and hence quite a bit easier to review (as one doesn't need to manually check the differences between the much larger added/removed pieces of code. Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-02-02 8:36 ` Jan Beulich @ 2015-02-02 13:53 ` Don Slutz 0 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-02-02 13:53 UTC (permalink / raw) To: Jan Beulich, Paul Durrant, Don Slutz Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Keir Fraser On 02/02/15 03:36, Jan Beulich wrote: >>>> On 30.01.15 at 20:19, <dslutz@verizon.com> wrote: >> Soon after sending this, I came up with a 2nd way for fix. >> Change hvm_has_dm() to use hvm_select_ioreq_server(). Then >> the correct answer will be found, and so will not retry. >> >> To avoid 2 calls to hvm_select_ioreq_server(), it makes >> snes to me to return s. Also adding a call to hvm_complete_assist_req() >> would help. So based on all this is is a possible v2: > > Looks reasonable (awaiting Paul's opinion though) except for > mechanical aspects: > >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p) >> return 1; >> } >> >> -bool_t hvm_has_dm(struct domain *d) >> +static bool_t hvm_complete_assist_req(ioreq_t *p) >> { ... >> +} >> + >> +void *hvm_has_dm(struct domain *d, ioreq_t *p) > > I can't see why this needs to return void * instead of a properly typed > pointer. > Using "struct hvm_ioreq_server *s" did not build: In file included from /home/don/xen-master/xen/include/asm/hvm/irq.h:26:0, from /home/don/xen-master/xen/include/asm/hvm/vpt.h:32, from /home/don/xen-master/xen/include/asm/hvm/vlapic.h:27, from /home/don/xen-master/xen/include/asm/hvm/vcpu.h:25, from /home/don/xen-master/xen/include/asm/domain.h:7, from /home/don/xen-master/xen/include/xen/domain.h:6, from /home/don/xen-master/xen/include/xen/sched.h:10, from x86_64/asm-offsets.c:10: /home/don/xen-master/xen/include/asm/hvm/hvm.h:231:35: error: 'struct hvm_ioreq_server' declared inside parameter list [-Werror] /home/don/xen-master/xen/include/asm/hvm/hvm.h:231:35: error: its scope is only this definition or declaration, which is probably not what you want [-Werror] cc1: all warnings being treated as errors So I went with void * to check the that the code would pass my unit testing. No clue why I did not say: +struct hvm_ioreq_server; +bool_t hvm_send_assist_req(struct hvm_ioreq_server *s, ioreq_t *p); Unless I hear otherwise, this is the way I will go. >> @@ -2571,44 +2607,15 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, >> return 0; >> } >> >> -static bool_t hvm_complete_assist_req(ioreq_t *p) >> -{ ... >> - return 0; /* implicitly bins the i/o operation */ >> -} > > Moving hvm_has_dm() down here would make the patch smaller and > hence quite a bit easier to review (as one doesn't need to manually > check the differences between the much larger added/removed > pieces of code. > Will do. -Don Slutz > Jan > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-01-30 19:19 ` Don Slutz 2015-02-02 8:36 ` Jan Beulich @ 2015-02-02 9:51 ` Paul Durrant 2015-02-02 10:04 ` Jan Beulich 1 sibling, 1 reply; 26+ messages in thread From: Paul Durrant @ 2015-02-02 9:51 UTC (permalink / raw) To: Don Slutz, Jan Beulich Cc: Wei Liu, Ian Campbell, Andrew Cooper, George Dunlap, xen-devel, Stefano Stabellini, Ian Jackson, Keir (Xen.org) > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 30 January 2015 19:19 > To: Don Slutz; Jan Beulich; Paul Durrant > Cc: Andrew Cooper; Ian Campbell; Wei Liu; George Dunlap; Ian Jackson; > Stefano Stabellini; xen-devel@lists.xen.org; Keir (Xen.org) > Subject: Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server > failed do not retry. > > On 01/30/15 13:17, Don Slutz wrote: > > On 01/30/15 05:23, Jan Beulich wrote: > >>>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote: > >>> I.E. do just what no backing DM does. > >> > >> _If_ this is correct, the if() modified here should be folded with the > >> one a few lines up. > > > > Ok, will fold with the one a few lines up. As expected without this > > change the guest will hang (more details below). > > > > > >> But looking at the description of the commit that > >> introduced this (bac0999325 "x86 hvm: Do not incorrectly retire an > >> instruction emulation...", almost immediately modified by f20f3c8ece > >> "x86 hvm: On failed hvm_send_assist_req(), io emulation...") I doubt > >> this is really what we want, or at the very least your change > >> description should explain what was wrong with the original commit. > >> > > > > Looking at these 2 commits, it looks to me like I need to handle these > > cases also. So it looks like having hvm_send_assist_req() return an int > > 0, 1, & 2 is the simpler way. V2 on the way soon. > > > > Soon after sending this, I came up with a 2nd way for fix. > Change hvm_has_dm() to use hvm_select_ioreq_server(). Then > the correct answer will be found, and so will not retry. > > To avoid 2 calls to hvm_select_ioreq_server(), it makes > snes to me to return s. Also adding a call to hvm_complete_assist_req() > would help. So based on all this is is a possible v2: > Yes, I think this approach does make a lot of sense. > > commit 24eb5a839427ba80c1adf16ab656c19729f906be > Author: Don Slutz <dslutz@verizon.com> > Date: Fri Jan 30 13:54:43 2015 -0500 > > fixup use of hvm_send_assist_req > > Signed-off-by: Don Slutz <dslutz@verizon.com> > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 2ed4344..7c3c654 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -218,8 +218,11 @@ static int hvmemul_do_io( > vio->io_state = HVMIO_handle_mmio_awaiting_completion; > break; > case X86EMUL_UNHANDLEABLE: > + { > + void *s = hvm_has_dm(curr->domain, &p); > + > /* If there is no backing DM, just ignore accesses */ > - if ( !hvm_has_dm(curr->domain) ) > + if ( !s ) > { > rc = X86EMUL_OKAY; > vio->io_state = HVMIO_none; > @@ -227,11 +230,12 @@ static int hvmemul_do_io( > else > { > rc = X86EMUL_RETRY; > - if ( !hvm_send_assist_req(&p) ) > + if ( !hvm_send_assist_req(s, &p) ) > vio->io_state = HVMIO_none; > else if ( p_data == NULL ) > rc = X86EMUL_OKAY; > } > + } > break; > default: > BUG(); > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 984af81..21d4a73 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p) > return 1; > } > > -bool_t hvm_has_dm(struct domain *d) > +static bool_t hvm_complete_assist_req(ioreq_t *p) You'll need to change this to a void return as Jan requested. > { > - return !list_empty(&d->arch.hvm_domain.ioreq_server.list); > + HVMTRACE_1D(COMPLETE_ASSIST, p->type); > + ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); > + switch ( p->type ) > + { > + case IOREQ_TYPE_COPY: > + case IOREQ_TYPE_PIO: > + if ( p->dir == IOREQ_READ ) > + { > + if ( !p->data_is_ptr ) > + p->data = ~0ul; > + else > + { > + int i, step = p->df ? -p->size : p->size; > + uint32_t data = ~0; > + > + for ( i = 0; i < p->count; i++ ) > + hvm_copy_to_guest_phys(p->data + step * i, &data, > + p->size); > + } > + } > + /* FALLTHRU */ > + default: > + p->state = STATE_IORESP_READY; > + hvm_io_assist(p); > + break; > + } > + > + return 1; > +} > + > +void *hvm_has_dm(struct domain *d, ioreq_t *p) > +{ > + struct hvm_ioreq_server *s = hvm_select_ioreq_server(d, p); > + > + if ( !s ) > + hvm_complete_assist_req(p); If you're going to complete the I/O here then... > + return (void *)s; > } > > bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, > @@ -2571,44 +2607,15 @@ bool_t > hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, > return 0; > } > > -static bool_t hvm_complete_assist_req(ioreq_t *p) > -{ > - HVMTRACE_1D(COMPLETE_ASSIST, p->type); > - ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); > - switch ( p->type ) > - { > - case IOREQ_TYPE_COPY: > - case IOREQ_TYPE_PIO: > - if ( p->dir == IOREQ_READ ) > - { > - if ( !p->data_is_ptr ) > - p->data = ~0ul; > - else > - { > - int i, step = p->df ? -p->size : p->size; > - uint32_t data = ~0; > - > - for ( i = 0; i < p->count; i++ ) > - hvm_copy_to_guest_phys(p->data + step * i, &data, > - p->size); > - } > - } > - /* FALLTHRU */ > - default: > - p->state = STATE_IORESP_READY; > - hvm_io_assist(p); > - break; > - } > - > - return 0; /* implicitly bins the i/o operation */ > -} > - > -bool_t hvm_send_assist_req(ioreq_t *p) > +bool_t hvm_send_assist_req(void *vs, ioreq_t *p) > { > - struct hvm_ioreq_server *s = > hvm_select_ioreq_server(current->domain, p); > + struct hvm_ioreq_server *s = vs; > > if ( !s ) ... I think you can ASSERT(s) here, as you should never get here if an ioreq server was chosen. > - return hvm_complete_assist_req(p); > + { > + hvm_complete_assist_req(p); > + return 1; > + } So, this function basically collapses down to just this call: > > return hvm_send_assist_req_to_ioreq_server(s, p); > } Paul > diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm- > x86/hvm/hvm.h > index e3d2d9a..1923842 100644 > --- a/xen/include/asm-x86/hvm/hvm.h > +++ b/xen/include/asm-x86/hvm/hvm.h > @@ -228,7 +228,7 @@ int hvm_vcpu_cacheattr_init(struct vcpu *v); > void hvm_vcpu_cacheattr_destroy(struct vcpu *v); > void hvm_vcpu_reset_state(struct vcpu *v, uint16_t cs, uint16_t ip); > > -bool_t hvm_send_assist_req(ioreq_t *p); > +bool_t hvm_send_assist_req(void *s, ioreq_t *p); > void hvm_broadcast_assist_req(ioreq_t *p); > > void hvm_get_guest_pat(struct vcpu *v, u64 *guest_pat); > @@ -359,7 +359,7 @@ void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx, > void hvm_cpuid(unsigned int input, unsigned int *eax, unsigned int *ebx, > unsigned int *ecx, unsigned int *edx); > void hvm_migrate_timers(struct vcpu *v); > -bool_t hvm_has_dm(struct domain *d); > +void *hvm_has_dm(struct domain *d, ioreq_t *p); > bool_t hvm_io_pending(struct vcpu *v); > void hvm_do_resume(struct vcpu *v); > void hvm_migrate_pirqs(struct vcpu *v); > > > -Don Slutz > > > > > Which is the better codding style: > > > > 1) Add #defines for the return values? > > 2) Just use numbers? > > 3) Invert the sense 0 means worked, 1 is shutdown_deferral or > > domain_crash, 2 is hvm_complete_assist_req()? > > > > > > I.E. (for just adding 2): > > > > > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > > index 2ed4344..c565151 100644 > > --- a/xen/arch/x86/hvm/emulate.c > > +++ b/xen/arch/x86/hvm/emulate.c > > @@ -227,10 +227,19 @@ static int hvmemul_do_io( > > else > > { > > rc = X86EMUL_RETRY; > > - if ( !hvm_send_assist_req(&p) ) > > - vio->io_state = HVMIO_none; > > - else if ( p_data == NULL ) > > + switch ( hvm_send_assist_req(&p) ) > > + { > > + case 2: > > rc = X86EMUL_OKAY; > > + /* fall through */ > > + case 0: > > + vio->io_state = HVMIO_none; > > + break; > > + case 1: > > + if ( p_data == NULL ) > > + rc = X86EMUL_OKAY; > > + break; > > + } > > } > > > > > > ??? > > > > > > > > I would think it would be good for the code using hvm_has_dm() > > to also call on hvm_complete_assist_req(). hvm_has_dm() is a subset of > > the cases that hvm_select_ioreq_server() checks for. > > > > Based on this, I think it would be better to remove the call on > > hvm_has_dm() instead of adding a call to hvm_complete_assist_req(). > > > > > > -Don Slutz > > P.S. Details for hang: > > > > Using: > > > > static bool_t hvm_complete_assist_req(ioreq_t *p) > > { > > + HVMTRACE_1D(COMPLETE_ASSIST, p->type); > > ... > > ): > > > > I get the trace: > > > > CPU1 745455716325 (+ 1362) VMEXIT [ exitcode = 0x0000001e, rIP > > = 0x00101581 ] > > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > > io_state = 0 ret = 0 ] > > CPU1 745455717846 (+ 1521) vlapic_accept_pic_intr [ i8259_target = > > 1, accept_pic_int = 1 ] > > CPU1 745455718209 (+ 363) VMENTRY > > CPU1 745455719568 (+ 1359) VMEXIT [ exitcode = 0x0000001e, rIP > > = 0x00101581 ] > > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > > io_state = 0 ret = 0 ] > > CPU1 745455721083 (+ 1515) vlapic_accept_pic_intr [ i8259_target = > > 1, accept_pic_int = 1 ] > > CPU1 745455721422 (+ 339) VMENTRY > > CPU1 745455722781 (+ 1359) VMEXIT [ exitcode = 0x0000001e, rIP > > = 0x00101581 ] > > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > > io_state = 0 ret = 0 ] > > CPU1 745455724299 (+ 1518) vlapic_accept_pic_intr [ i8259_target = > > 1, accept_pic_int = 1 ] > > CPU1 745455724656 (+ 357) VMENTRY > > CPU1 745455726009 (+ 1353) VMEXIT [ exitcode = 0x0000001e, rIP > > = 0x00101581 ] > > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > > io_state = 0 ret = 0 ] > > CPU1 745455727539 (+ 1530) vlapic_accept_pic_intr [ i8259_target = > > 1, accept_pic_int = 1 ] > > CPU1 745455727899 (+ 360) VMENTRY > > CPU1 745455729276 (+ 1377) VMEXIT [ exitcode = 0x0000001e, rIP > > = 0x00101581 ] > > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > > io_state = 0 ret = 0 ] > > CPU1 745455730803 (+ 1527) vlapic_accept_pic_intr [ i8259_target = > > 1, accept_pic_int = 1 ] > > CPU1 745455731163 (+ 360) VMENTRY > > CPU1 745455732525 (+ 1362) VMEXIT [ exitcode = 0x0000001e, rIP > > = 0x00101581 ] > > CPU1 0 (+ 0) COMPLETE_ASSIST [ type = 0 ] > > CPU1 0 (+ 0) HANDLE_PIO [ port = 0x0cfe size = 2 dir = 1 > > io_state = 0 ret = 0 ] > > CPU1 745455734049 (+ 1524) vlapic_accept_pic_intr [ i8259_target = > > 1, accept_pic_int = 1 ] > > CPU1 745455734385 (+ 336) VMENTRY > > ... > > > > > >> Jan > >> > >>> --- a/xen/arch/x86/hvm/emulate.c > >>> +++ b/xen/arch/x86/hvm/emulate.c > >>> @@ -228,7 +228,11 @@ static int hvmemul_do_io( > >>> { > >>> rc = X86EMUL_RETRY; > >>> if ( !hvm_send_assist_req(&p) ) > >>> + { > >>> + /* Since the send failed, do not retry */ > >>> + rc = X86EMUL_OKAY; > >>> vio->io_state = HVMIO_none; > >>> + } > >>> else if ( p_data == NULL ) > >>> rc = X86EMUL_OKAY; > >>> } > >>> -- > >>> 1.8.4 > >> > >> > >> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-02-02 9:51 ` Paul Durrant @ 2015-02-02 10:04 ` Jan Beulich 2015-02-02 10:06 ` Paul Durrant 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2015-02-02 10:04 UTC (permalink / raw) To: Paul Durrant, Don Slutz Cc: Wei Liu, Ian Campbell, Andrew Cooper, George Dunlap, xen-devel, StefanoStabellini, Ian Jackson, Keir (Xen.org) >>> On 02.02.15 at 10:51, <Paul.Durrant@citrix.com> wrote: >> From: Don Slutz [mailto:dslutz@verizon.com] >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p) >> return 1; >> } >> >> -bool_t hvm_has_dm(struct domain *d) >> +static bool_t hvm_complete_assist_req(ioreq_t *p) > > You'll need to change this to a void return as Jan requested. I don't think I did. Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-02-02 10:04 ` Jan Beulich @ 2015-02-02 10:06 ` Paul Durrant 2015-02-02 10:14 ` Jan Beulich 0 siblings, 1 reply; 26+ messages in thread From: Paul Durrant @ 2015-02-02 10:06 UTC (permalink / raw) To: Jan Beulich, Don Slutz Cc: Wei Liu, Ian Campbell, Andrew Cooper, George Dunlap, xen-devel, Stefano Stabellini, Ian Jackson, Keir (Xen.org) > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 02 February 2015 10:04 > To: Paul Durrant; Don Slutz > Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Stefano > Stabellini; Wei Liu; xen-devel@lists.xen.org; Keir (Xen.org) > Subject: RE: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server > failed do not retry. > > >>> On 02.02.15 at 10:51, <Paul.Durrant@citrix.com> wrote: > >> From: Don Slutz [mailto:dslutz@verizon.com] > >> --- a/xen/arch/x86/hvm/hvm.c > >> +++ b/xen/arch/x86/hvm/hvm.c > >> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p) > >> return 1; > >> } > >> > >> -bool_t hvm_has_dm(struct domain *d) > >> +static bool_t hvm_complete_assist_req(ioreq_t *p) > > > > You'll need to change this to a void return as Jan requested. > > I don't think I did. > Oh, sorry. I thought you suggested that the constant return of 1 from hvm_complete_assist_req() was a bad idea. Paul > Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-02-02 10:06 ` Paul Durrant @ 2015-02-02 10:14 ` Jan Beulich 2015-02-02 13:54 ` Don Slutz 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2015-02-02 10:14 UTC (permalink / raw) To: Paul Durrant Cc: Wei Liu, Ian Campbell, Andrew Cooper, George Dunlap, Don Slutz, xen-devel, Stefano Stabellini, IanJackson, Keir (Xen.org) >>> On 02.02.15 at 11:06, <Paul.Durrant@citrix.com> wrote: >> -----Original Message----- >> From: Jan Beulich [mailto:JBeulich@suse.com] >> Sent: 02 February 2015 10:04 >> To: Paul Durrant; Don Slutz >> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Stefano >> Stabellini; Wei Liu; xen-devel@lists.xen.org; Keir (Xen.org) >> Subject: RE: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server >> failed do not retry. >> >> >>> On 02.02.15 at 10:51, <Paul.Durrant@citrix.com> wrote: >> >> From: Don Slutz [mailto:dslutz@verizon.com] >> >> --- a/xen/arch/x86/hvm/hvm.c >> >> +++ b/xen/arch/x86/hvm/hvm.c >> >> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p) >> >> return 1; >> >> } >> >> >> >> -bool_t hvm_has_dm(struct domain *d) >> >> +static bool_t hvm_complete_assist_req(ioreq_t *p) >> > >> > You'll need to change this to a void return as Jan requested. >> >> I don't think I did. > > Oh, sorry. I thought you suggested that the constant return of 1 from > hvm_complete_assist_req() was a bad idea. Oh, I see, but that was in another thread, and I really suggested to remove the function altogether (folding it back into its only caller). Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-02-02 10:14 ` Jan Beulich @ 2015-02-02 13:54 ` Don Slutz 0 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-02-02 13:54 UTC (permalink / raw) To: Jan Beulich, Paul Durrant Cc: Wei Liu, Ian Campbell, Andrew Cooper, George Dunlap, Don Slutz, xen-devel, Stefano Stabellini, IanJackson, Keir (Xen.org) On 02/02/15 05:14, Jan Beulich wrote: >>>> On 02.02.15 at 11:06, <Paul.Durrant@citrix.com> wrote: >>> -----Original Message----- >>> From: Jan Beulich [mailto:JBeulich@suse.com] >>> Sent: 02 February 2015 10:04 >>> To: Paul Durrant; Don Slutz >>> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Stefano >>> Stabellini; Wei Liu; xen-devel@lists.xen.org; Keir (Xen.org) >>> Subject: RE: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server >>> failed do not retry. >>> >>>>>> On 02.02.15 at 10:51, <Paul.Durrant@citrix.com> wrote: >>>>> From: Don Slutz [mailto:dslutz@verizon.com] >>>>> --- a/xen/arch/x86/hvm/hvm.c >>>>> +++ b/xen/arch/x86/hvm/hvm.c >>>>> @@ -2507,9 +2507,45 @@ int hvm_buffered_io_send(ioreq_t *p) >>>>> return 1; >>>>> } >>>>> >>>>> -bool_t hvm_has_dm(struct domain *d) >>>>> +static bool_t hvm_complete_assist_req(ioreq_t *p) >>>> >>>> You'll need to change this to a void return as Jan requested. >>> >>> I don't think I did. >> >> Oh, sorry. I thought you suggested that the constant return of 1 from >> hvm_complete_assist_req() was a bad idea. > > Oh, I see, but that was in another thread, and I really suggested to > remove the function altogether (folding it back into its only caller). > I was planing to also do this. This is why the code was not in patch form. -Don Slutz > Jan > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-01-30 0:52 ` [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry Don Slutz 2015-01-30 10:23 ` Jan Beulich @ 2015-01-30 10:37 ` Paul Durrant 2015-01-30 17:51 ` Don Slutz 1 sibling, 1 reply; 26+ messages in thread From: Paul Durrant @ 2015-01-30 10:37 UTC (permalink / raw) To: Don Slutz, xen-devel Cc: Wei Liu, Keir (Xen.org), Ian Campbell, Andrew Cooper, George Dunlap, Stefano Stabellini, Jan Beulich, Ian Jackson > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 30 January 2015 00:52 > To: xen-devel@lists.xen.org > Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich; > Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz > Subject: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed > do not retry. > > I.E. do just what no backing DM does. > > Signed-off-by: Don Slutz <dslutz@verizon.com> > --- > xen/arch/x86/hvm/emulate.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c > index 2ed4344..e9fc070 100644 > --- a/xen/arch/x86/hvm/emulate.c > +++ b/xen/arch/x86/hvm/emulate.c > @@ -228,7 +228,11 @@ static int hvmemul_do_io( > { > rc = X86EMUL_RETRY; > if ( !hvm_send_assist_req(&p) ) > + { > + /* Since the send failed, do not retry */ > + rc = X86EMUL_OKAY; I guess this is probably ok. It's a bit moot though as the only circumstance under which hvm_send_assist_req() will return 0 AFAICT, domain_crash() will have been called, or the vcpu is shutting down. Paul > vio->io_state = HVMIO_none; > + } > else if ( p_data == NULL ) > rc = X86EMUL_OKAY; > } > -- > 1.8.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry. 2015-01-30 10:37 ` Paul Durrant @ 2015-01-30 17:51 ` Don Slutz 0 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 17:51 UTC (permalink / raw) To: Paul Durrant, Don Slutz, xen-devel Cc: Wei Liu, Keir (Xen.org), Ian Campbell, Andrew Cooper, George Dunlap, Stefano Stabellini, Jan Beulich, Ian Jackson On 01/30/15 05:37, Paul Durrant wrote: >> -----Original Message----- >> From: Don Slutz [mailto:dslutz@verizon.com] >> Sent: 30 January 2015 00:52 >> To: xen-devel@lists.xen.org >> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich; >> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz >> Subject: [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed >> do not retry. >> >> I.E. do just what no backing DM does. >> >> Signed-off-by: Don Slutz <dslutz@verizon.com> >> --- >> xen/arch/x86/hvm/emulate.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c >> index 2ed4344..e9fc070 100644 >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -228,7 +228,11 @@ static int hvmemul_do_io( >> { >> rc = X86EMUL_RETRY; >> if ( !hvm_send_assist_req(&p) ) >> + { >> + /* Since the send failed, do not retry */ >> + rc = X86EMUL_OKAY; > > I guess this is probably ok. It's a bit moot though as the only circumstance under which hvm_send_assist_req() will return 0 AFAICT, domain_crash() will have been called, or the vcpu is shutting down. > Turns out this is not ok. Jan pointed out that for guest suspend to work correctly, you need to retry. -Don Slutz > Paul > >> vio->io_state = HVMIO_none; >> + } >> else if ( p_data == NULL ) >> rc = X86EMUL_OKAY; >> } >> -- >> 1.8.4 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG 2015-01-30 0:52 [PATCH 0/5] Skip unneeded VMENTRY & VMEXIT Don Slutz ` (2 preceding siblings ...) 2015-01-30 0:52 ` [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry Don Slutz @ 2015-01-30 0:52 ` Don Slutz 2015-01-30 10:24 ` Jan Beulich 2015-01-30 0:52 ` [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send Don Slutz 4 siblings, 1 reply; 26+ messages in thread From: Don Slutz @ 2015-01-30 0:52 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, Don Slutz, Paul Durrant, Jan Beulich, Wei Liu Signed-off-by: Don Slutz <dslutz@verizon.com> --- xen/arch/x86/hvm/hvm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index c7984d1..6f7b407 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2573,6 +2573,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct hvm_ioreq_server *s, static bool_t hvm_complete_assist_req(ioreq_t *p) { + ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); switch ( p->type ) { case IOREQ_TYPE_COPY: -- 1.8.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG 2015-01-30 0:52 ` [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG Don Slutz @ 2015-01-30 10:24 ` Jan Beulich 2015-01-30 17:17 ` Don Slutz 0 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2015-01-30 10:24 UTC (permalink / raw) To: Don Slutz Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Paul Durrant, Keir Fraser >>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote: According to your 0/5 Suggested-by: Paul Durrant <paul.durrant@citrix.com> ? Jan > Signed-off-by: Don Slutz <dslutz@verizon.com> > --- > xen/arch/x86/hvm/hvm.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index c7984d1..6f7b407 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2573,6 +2573,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct > hvm_ioreq_server *s, > > static bool_t hvm_complete_assist_req(ioreq_t *p) > { > + ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); > switch ( p->type ) > { > case IOREQ_TYPE_COPY: > -- > 1.8.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG 2015-01-30 10:24 ` Jan Beulich @ 2015-01-30 17:17 ` Don Slutz 0 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 17:17 UTC (permalink / raw) To: Jan Beulich, Don Slutz Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Paul Durrant, Keir Fraser On 01/30/15 05:24, Jan Beulich wrote: >>>> >>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote: > According to your 0/5 > Suggested-by: Paul Durrant <paul.durrant@citrix.com> > ? Yes, added. -Don Slutz > > Jan > >> > Signed-off-by: Don Slutz <dslutz@verizon.com> >> > --- >> > xen/arch/x86/hvm/hvm.c | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> > index c7984d1..6f7b407 100644 >> > --- a/xen/arch/x86/hvm/hvm.c >> > +++ b/xen/arch/x86/hvm/hvm.c >> > @@ -2573,6 +2573,7 @@ bool_t hvm_send_assist_req_to_ioreq_server(struct >> > hvm_ioreq_server *s, >> > >> > static bool_t hvm_complete_assist_req(ioreq_t *p) >> > { >> > + ASSERT(p->type != IOREQ_TYPE_PCI_CONFIG); >> > switch ( p->type ) >> > { >> > case IOREQ_TYPE_COPY: >> > -- >> > 1.8.4 > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send 2015-01-30 0:52 [PATCH 0/5] Skip unneeded VMENTRY & VMEXIT Don Slutz ` (3 preceding siblings ...) 2015-01-30 0:52 ` [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG Don Slutz @ 2015-01-30 0:52 ` Don Slutz 2015-01-30 10:24 ` Jan Beulich ` (2 more replies) 4 siblings, 3 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 0:52 UTC (permalink / raw) To: xen-devel Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, Don Slutz, Paul Durrant, Jan Beulich, Wei Liu This saves a VMENTRY and a VMEXIT since we not longer retry the ioport read. hvmemul_do_io() will retry in order to complete an ioport read when hvm_send_assist_req() is successful. Signed-off-by: Don Slutz <dslutz@verizon.com> --- xen/arch/x86/hvm/hvm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c index 6f7b407..bad410e 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p) break; } - return 1; + return 0; /* implicitly bins the i/o operation */ } bool_t hvm_send_assist_req(ioreq_t *p) -- 1.8.4 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send 2015-01-30 0:52 ` [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send Don Slutz @ 2015-01-30 10:24 ` Jan Beulich 2015-01-30 17:47 ` Don Slutz 2015-01-30 10:40 ` Paul Durrant 2015-01-30 10:53 ` Paul Durrant 2 siblings, 1 reply; 26+ messages in thread From: Jan Beulich @ 2015-01-30 10:24 UTC (permalink / raw) To: Paul Durrant, Don Slutz Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Keir Fraser >>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote: > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p) > break; > } > > - return 1; > + return 0; /* implicitly bins the i/o operation */ > } This change points out that having hvm_complete_assist_req() be a separate function yet having only a single caller, and it returning non-void with only a single possible return value isn't the best arrangement. I think this should be brought back into hvm_send_assist_req(), by inverting the if() condition there. Unless there are intentions for it to have another caller, but in that case it should still be made return void, with the caller choosing what to return. Jan ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send 2015-01-30 10:24 ` Jan Beulich @ 2015-01-30 17:47 ` Don Slutz 0 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 17:47 UTC (permalink / raw) To: Jan Beulich, Paul Durrant, Don Slutz Cc: Wei Liu, Ian Campbell, Stefano Stabellini, George Dunlap, Andrew Cooper, Ian Jackson, xen-devel, Keir Fraser On 01/30/15 05:24, Jan Beulich wrote: >>>> On 30.01.15 at 01:52, <dslutz@verizon.com> wrote: >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p) >> break; >> } >> >> - return 1; >> + return 0; /* implicitly bins the i/o operation */ >> } > > This change points out that having hvm_complete_assist_req() be a > separate function yet having only a single caller, and it returning > non-void with only a single possible return value isn't the best > arrangement. I think this should be brought back into > hvm_send_assist_req(), by inverting the if() condition there. Unless > there are intentions for it to have another caller, but in that case > it should still be made return void, with the caller choosing what to > return. > Sounds good to me will do. -Don Slutz > Jan > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send 2015-01-30 0:52 ` [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send Don Slutz 2015-01-30 10:24 ` Jan Beulich @ 2015-01-30 10:40 ` Paul Durrant 2015-01-30 17:48 ` Don Slutz 2015-01-30 10:53 ` Paul Durrant 2 siblings, 1 reply; 26+ messages in thread From: Paul Durrant @ 2015-01-30 10:40 UTC (permalink / raw) To: Don Slutz, xen-devel Cc: Wei Liu, Keir (Xen.org), Ian Campbell, Andrew Cooper, George Dunlap, Stefano Stabellini, Jan Beulich, Ian Jackson > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 30 January 2015 00:53 > To: xen-devel@lists.xen.org > Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich; > Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz > Subject: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send > > This saves a VMENTRY and a VMEXIT since we not longer retry the > ioport read. > > hvmemul_do_io() will retry in order to complete an ioport read when > hvm_send_assist_req() is successful. > > Signed-off-by: Don Slutz <dslutz@verizon.com> It seems like this change should be folded into patch 3? I think the two changes only make sense in combination. Paul > --- > xen/arch/x86/hvm/hvm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 6f7b407..bad410e 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p) > break; > } > > - return 1; > + return 0; /* implicitly bins the i/o operation */ > } > > bool_t hvm_send_assist_req(ioreq_t *p) > -- > 1.8.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send 2015-01-30 10:40 ` Paul Durrant @ 2015-01-30 17:48 ` Don Slutz 0 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 17:48 UTC (permalink / raw) To: Paul Durrant, Don Slutz, xen-devel Cc: Wei Liu, Keir (Xen.org), Ian Campbell, Andrew Cooper, George Dunlap, Stefano Stabellini, Jan Beulich, Ian Jackson On 01/30/15 05:40, Paul Durrant wrote: >> -----Original Message----- >> From: Don Slutz [mailto:dslutz@verizon.com] >> Sent: 30 January 2015 00:53 >> To: xen-devel@lists.xen.org >> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich; >> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz >> Subject: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send >> >> This saves a VMENTRY and a VMEXIT since we not longer retry the >> ioport read. >> >> hvmemul_do_io() will retry in order to complete an ioport read when >> hvm_send_assist_req() is successful. >> >> Signed-off-by: Don Slutz <dslutz@verizon.com> > > It seems like this change should be folded into patch 3? I think the two changes only make sense in combination. > Happy to do so. -Don Slutz > Paul > >> --- >> xen/arch/x86/hvm/hvm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index 6f7b407..bad410e 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p) >> break; >> } >> >> - return 1; >> + return 0; /* implicitly bins the i/o operation */ >> } >> >> bool_t hvm_send_assist_req(ioreq_t *p) >> -- >> 1.8.4 > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send 2015-01-30 0:52 ` [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send Don Slutz 2015-01-30 10:24 ` Jan Beulich 2015-01-30 10:40 ` Paul Durrant @ 2015-01-30 10:53 ` Paul Durrant 2015-01-30 17:49 ` Don Slutz 2 siblings, 1 reply; 26+ messages in thread From: Paul Durrant @ 2015-01-30 10:53 UTC (permalink / raw) To: Don Slutz, xen-devel Cc: Wei Liu, Keir (Xen.org), Ian Campbell, Andrew Cooper, George Dunlap, Stefano Stabellini, Jan Beulich, Ian Jackson > -----Original Message----- > From: Don Slutz [mailto:dslutz@verizon.com] > Sent: 30 January 2015 00:53 > To: xen-devel@lists.xen.org > Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich; > Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz > Subject: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send > > This saves a VMENTRY and a VMEXIT since we not longer retry the > ioport read. > > hvmemul_do_io() will retry in order to complete an ioport read when > hvm_send_assist_req() is successful. > > Signed-off-by: Don Slutz <dslutz@verizon.com> > --- > xen/arch/x86/hvm/hvm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c > index 6f7b407..bad410e 100644 > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p) > break; > } > > - return 1; > + return 0; /* implicitly bins the i/o operation */ Actually that comment is not right. The operation is not binned; it's just already been done. Paul > } > > bool_t hvm_send_assist_req(ioreq_t *p) > -- > 1.8.4 ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send 2015-01-30 10:53 ` Paul Durrant @ 2015-01-30 17:49 ` Don Slutz 0 siblings, 0 replies; 26+ messages in thread From: Don Slutz @ 2015-01-30 17:49 UTC (permalink / raw) To: Paul Durrant, Don Slutz, xen-devel Cc: Wei Liu, Keir (Xen.org), Ian Campbell, Andrew Cooper, George Dunlap, Stefano Stabellini, Jan Beulich, Ian Jackson On 01/30/15 05:53, Paul Durrant wrote: >> -----Original Message----- >> From: Don Slutz [mailto:dslutz@verizon.com] >> Sent: 30 January 2015 00:53 >> To: xen-devel@lists.xen.org >> Cc: Andrew Cooper; George Dunlap; Ian Campbell; Ian Jackson; Jan Beulich; >> Keir (Xen.org); Stefano Stabellini; Wei Liu; Paul Durrant; Don Slutz >> Subject: [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send >> >> This saves a VMENTRY and a VMEXIT since we not longer retry the >> ioport read. >> >> hvmemul_do_io() will retry in order to complete an ioport read when >> hvm_send_assist_req() is successful. >> >> Signed-off-by: Don Slutz <dslutz@verizon.com> >> --- >> xen/arch/x86/hvm/hvm.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c >> index 6f7b407..bad410e 100644 >> --- a/xen/arch/x86/hvm/hvm.c >> +++ b/xen/arch/x86/hvm/hvm.c >> @@ -2599,7 +2599,7 @@ static bool_t hvm_complete_assist_req(ioreq_t *p) >> break; >> } >> >> - return 1; >> + return 0; /* implicitly bins the i/o operation */ > > Actually that comment is not right. The operation is not binned; it's just already been done. > Will adjust, I expect the return to go away. -Don Slutz > Paul > >> } >> >> bool_t hvm_send_assist_req(ioreq_t *p) >> -- >> 1.8.4 > ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2015-02-02 13:54 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-30 0:52 [PATCH 0/5] Skip unneeded VMENTRY & VMEXIT Don Slutz 2015-01-30 0:52 ` [PATCH 1/5] DEBUG: xentrace: Add debug of HANDLE_PIO Don Slutz 2015-01-30 0:52 ` [PATCH 2/5] xentrace: Adjust IOPORT & MMIO format Don Slutz 2015-01-30 0:52 ` [PATCH 3/5] hvmemul_do_io: If the send to the ioreq server failed do not retry Don Slutz 2015-01-30 10:23 ` Jan Beulich 2015-01-30 18:17 ` Don Slutz 2015-01-30 19:19 ` Don Slutz 2015-02-02 8:36 ` Jan Beulich 2015-02-02 13:53 ` Don Slutz 2015-02-02 9:51 ` Paul Durrant 2015-02-02 10:04 ` Jan Beulich 2015-02-02 10:06 ` Paul Durrant 2015-02-02 10:14 ` Jan Beulich 2015-02-02 13:54 ` Don Slutz 2015-01-30 10:37 ` Paul Durrant 2015-01-30 17:51 ` Don Slutz 2015-01-30 0:52 ` [PATCH 4/5] hvm_complete_assist_req: We should not be able to get here on IOREQ_TYPE_PCI_CONFIG Don Slutz 2015-01-30 10:24 ` Jan Beulich 2015-01-30 17:17 ` Don Slutz 2015-01-30 0:52 ` [PATCH 5/5] hvm_complete_assist_req: Tell caller we failed to send Don Slutz 2015-01-30 10:24 ` Jan Beulich 2015-01-30 17:47 ` Don Slutz 2015-01-30 10:40 ` Paul Durrant 2015-01-30 17:48 ` Don Slutz 2015-01-30 10:53 ` Paul Durrant 2015-01-30 17:49 ` Don Slutz
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.