|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [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@xxxxxxxxxxx> 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@xxxxxxxxxxx>
Date: Fri Jan 30 13:54:43 2015 -0500
fixup use of hvm_send_assist_req
Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
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
>>
>>
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |