[PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Jan Beulich]

vio->mmio_retry is being set when a repeated string insn is being split
up. In that case we'll exit to the guest, expecting immediate re-entry.
Interruptions, however, may be serviced by the guest before re-entry
from the repeated string insn. Any emulation needed in the course of
handling the interruption must not fetch from the internally maintained
cache.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2109,20 +2109,22 @@ static int _hvm_emulate_one(struct hvm_e
 
     vio->mmio_retry = 0;
 
-    rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
-
-    if ( rc == X86EMUL_OKAY && vio->mmio_retry )
-        rc = X86EMUL_RETRY;
-    if ( rc != X86EMUL_RETRY )
+    switch ( rc = x86_emulate(&hvmemul_ctxt->ctxt, ops) )
     {
+    case X86EMUL_OKAY:
+        if ( vio->mmio_retry )
+            rc = X86EMUL_RETRY;
+        /* fall through */
+    default:
         vio->mmio_cache_count = 0;
         vio->mmio_insn_bytes = 0;
-    }
-    else
-    {
+        break;
+
+    case X86EMUL_RETRY:
         BUILD_BUG_ON(sizeof(vio->mmio_insn) < sizeof(hvmemul_ctxt->insn_buf));
         vio->mmio_insn_bytes = hvmemul_ctxt->insn_buf_bytes;
         memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes);
+        break;
     }
 
     if ( hvmemul_ctxt->ctxt.retire.singlestep )




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 05 December 2017 16:14
> To: xen-devel <xen-devel@lists.xenproject.org>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>
> Subject: [PATCH] x86/HVM: don't retain emulated insn cache when exiting
> back to guest
> 
> vio->mmio_retry is being set when a repeated string insn is being split
> up. In that case we'll exit to the guest, expecting immediate re-entry.
> Interruptions, however, may be serviced by the guest before re-entry
> from the repeated string insn. Any emulation needed in the course of
> handling the interruption must not fetch from the internally maintained
> cache.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2109,20 +2109,22 @@ static int _hvm_emulate_one(struct hvm_e
> 
>      vio->mmio_retry = 0;
> 
> -    rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
> -
> -    if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> -        rc = X86EMUL_RETRY;
> -    if ( rc != X86EMUL_RETRY )
> +    switch ( rc = x86_emulate(&hvmemul_ctxt->ctxt, ops) )
>      {
> +    case X86EMUL_OKAY:
> +        if ( vio->mmio_retry )
> +            rc = X86EMUL_RETRY;
> +        /* fall through */
> +    default:
>          vio->mmio_cache_count = 0;
>          vio->mmio_insn_bytes = 0;
> -    }
> -    else
> -    {
> +        break;
> +
> +    case X86EMUL_RETRY:
>          BUILD_BUG_ON(sizeof(vio->mmio_insn) < sizeof(hvmemul_ctxt-
> >insn_buf));
>          vio->mmio_insn_bytes = hvmemul_ctxt->insn_buf_bytes;
>          memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio-
> >mmio_insn_bytes);
> +        break;

So, we have two distinct cases when X86EMUL_RETRY will be returned: the former when we do want to return to guest part way through a rep operation, and another when an MMIO has been sent for external emulation and we are expecting a completion. The code looks correct so...

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

...but I wonder there should be two distinct return codes for these two cases.

>      }
> 
>      if ( hvmemul_ctxt->ctxt.retire.singlestep )
> 
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Jan Beulich]

>>> On 05.12.17 at 17:44, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 05 December 2017 16:14
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -2109,20 +2109,22 @@ static int _hvm_emulate_one(struct hvm_e
>> 
>>      vio->mmio_retry = 0;
>> 
>> -    rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
>> -
>> -    if ( rc == X86EMUL_OKAY && vio->mmio_retry )
>> -        rc = X86EMUL_RETRY;
>> -    if ( rc != X86EMUL_RETRY )
>> +    switch ( rc = x86_emulate(&hvmemul_ctxt->ctxt, ops) )
>>      {
>> +    case X86EMUL_OKAY:
>> +        if ( vio->mmio_retry )
>> +            rc = X86EMUL_RETRY;
>> +        /* fall through */
>> +    default:
>>          vio->mmio_cache_count = 0;
>>          vio->mmio_insn_bytes = 0;
>> -    }
>> -    else
>> -    {
>> +        break;
>> +
>> +    case X86EMUL_RETRY:
>>          BUILD_BUG_ON(sizeof(vio->mmio_insn) < sizeof(hvmemul_ctxt-
>> >insn_buf));
>>          vio->mmio_insn_bytes = hvmemul_ctxt->insn_buf_bytes;
>>          memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio-
>> >mmio_insn_bytes);
>> +        break;
> 
> So, we have two distinct cases when X86EMUL_RETRY will be returned: the 
> former when we do want to return to guest part way through a rep operation, 
> and another when an MMIO has been sent for external emulation and we are 
> expecting a completion. The code looks correct so...
> 
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Thanks.

> ...but I wonder there should be two distinct return codes for these two 
> cases.

The question is - does any of the callers care about the difference.
I think the relevant information has been recorded in data structures
by the time we get here.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 05 December 2017 17:19
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Julien Grall <julien.grall@arm.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; xen-devel <xen-
> devel@lists.xenproject.org>
> Subject: RE: [PATCH] x86/HVM: don't retain emulated insn cache when
> exiting back to guest
> 
> >>> On 05.12.17 at 17:44, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 05 December 2017 16:14
> >> --- a/xen/arch/x86/hvm/emulate.c
> >> +++ b/xen/arch/x86/hvm/emulate.c
> >> @@ -2109,20 +2109,22 @@ static int _hvm_emulate_one(struct hvm_e
> >>
> >>      vio->mmio_retry = 0;
> >>
> >> -    rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
> >> -
> >> -    if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> >> -        rc = X86EMUL_RETRY;
> >> -    if ( rc != X86EMUL_RETRY )
> >> +    switch ( rc = x86_emulate(&hvmemul_ctxt->ctxt, ops) )
> >>      {
> >> +    case X86EMUL_OKAY:
> >> +        if ( vio->mmio_retry )
> >> +            rc = X86EMUL_RETRY;
> >> +        /* fall through */
> >> +    default:
> >>          vio->mmio_cache_count = 0;
> >>          vio->mmio_insn_bytes = 0;
> >> -    }
> >> -    else
> >> -    {
> >> +        break;
> >> +
> >> +    case X86EMUL_RETRY:
> >>          BUILD_BUG_ON(sizeof(vio->mmio_insn) < sizeof(hvmemul_ctxt-
> >> >insn_buf));
> >>          vio->mmio_insn_bytes = hvmemul_ctxt->insn_buf_bytes;
> >>          memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio-
> >> >mmio_insn_bytes);
> >> +        break;
> >
> > So, we have two distinct cases when X86EMUL_RETRY will be returned: the
> > former when we do want to return to guest part way through a rep
> operation,
> > and another when an MMIO has been sent for external emulation and we
> are
> > expecting a completion. The code looks correct so...
> >
> > Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
> 
> Thanks.
> 
> > ...but I wonder there should be two distinct return codes for these two
> > cases.
> 
> The question is - does any of the callers care about the difference.
> I think the relevant information has been recorded in data structures
> by the time we get here.

Technically, no I don't think the callers need to know but I've certainly got a bit tied in knots while trying to remember how this stuff is supposed to work so I was just hoping something could be done to make the 'don't return to guest because we need to retry mmio' and the 'return to guest even though want to retry a string operation' cases more obviously distinct.

  Paul

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Julien Grall]

Hi Jan,

I guess I have been CCed because you would like this patch is fixing the 
regression you mentioned on IRC?

Cheers,

On 12/05/2017 04:13 PM, Jan Beulich wrote:
> vio->mmio_retry is being set when a repeated string insn is being split
> up. In that case we'll exit to the guest, expecting immediate re-entry.
> Interruptions, however, may be serviced by the guest before re-entry
> from the repeated string insn. Any emulation needed in the course of
> handling the interruption must not fetch from the internally maintained
> cache.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -2109,20 +2109,22 @@ static int _hvm_emulate_one(struct hvm_e
>   
>       vio->mmio_retry = 0;
>   
> -    rc = x86_emulate(&hvmemul_ctxt->ctxt, ops);
> -
> -    if ( rc == X86EMUL_OKAY && vio->mmio_retry )
> -        rc = X86EMUL_RETRY;
> -    if ( rc != X86EMUL_RETRY )
> +    switch ( rc = x86_emulate(&hvmemul_ctxt->ctxt, ops) )
>       {
> +    case X86EMUL_OKAY:
> +        if ( vio->mmio_retry )
> +            rc = X86EMUL_RETRY;
> +        /* fall through */
> +    default:
>           vio->mmio_cache_count = 0;
>           vio->mmio_insn_bytes = 0;
> -    }
> -    else
> -    {
> +        break;
> +
> +    case X86EMUL_RETRY:
>           BUILD_BUG_ON(sizeof(vio->mmio_insn) < sizeof(hvmemul_ctxt->insn_buf));
>           vio->mmio_insn_bytes = hvmemul_ctxt->insn_buf_bytes;
>           memcpy(vio->mmio_insn, hvmemul_ctxt->insn_buf, vio->mmio_insn_bytes);
> +        break;
>       }
>   
>       if ( hvmemul_ctxt->ctxt.retire.singlestep )
> 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Jan Beulich]

>>> On 06.12.17 at 10:47, <julien.grall@linaro.org> wrote:
> I guess I have been CCed because you would like this patch is fixing the 
> regression you mentioned on IRC?

Yes, but first of all we need to see whether the issue goes away in
master once the patch is in.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Julien Grall]

On 12/06/2017 11:45 AM, Jan Beulich wrote:
>>>> On 06.12.17 at 10:47, <julien.grall@linaro.org> wrote:
>> I guess I have been CCed because you would like this patch is fixing the
>> regression you mentioned on IRC?
> 
> Yes, but first of all we need to see whether the issue goes away in
> master once the patch is in.

Would reverting the offending patch in Xen 4.10 be solution?

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Jan Beulich]

>>> On 06.12.17 at 12:47, <julien.grall@linaro.org> wrote:
> On 12/06/2017 11:45 AM, Jan Beulich wrote:
>>>>> On 06.12.17 at 10:47, <julien.grall@linaro.org> wrote:
>>> I guess I have been CCed because you would like this patch is fixing the
>>> regression you mentioned on IRC?
>> 
>> Yes, but first of all we need to see whether the issue goes away in
>> master once the patch is in.
> 
> Would reverting the offending patch in Xen 4.10 be solution?

Probably yes, at the price of re-introducing the issue that change
did fix. But reverting wouldn't feel right: The change, after all,
was not buggy - it merely uncovered the other issue, as far as we
can tell.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Julien Grall]

Hi Jan,

On 12/06/2017 12:58 PM, Jan Beulich wrote:
>>>> On 06.12.17 at 12:47, <julien.grall@linaro.org> wrote:
>> On 12/06/2017 11:45 AM, Jan Beulich wrote:
>>>>>> On 06.12.17 at 10:47, <julien.grall@linaro.org> wrote:
>>>> I guess I have been CCed because you would like this patch is fixing the
>>>> regression you mentioned on IRC?
>>>
>>> Yes, but first of all we need to see whether the issue goes away in
>>> master once the patch is in.
>>
>> Would reverting the offending patch in Xen 4.10 be solution?
> 
> Probably yes, at the price of re-introducing the issue that change
> did fix. But reverting wouldn't feel right: The change, after all,
> was not buggy - it merely uncovered the other issue, as far as we
> can tell.

I understand. I have seen you pushed the fixes in master today. Let see 
how it perform and decide tomorrow what to do.

Cheers,

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Andrew Cooper]

On 06/12/17 13:04, Julien Grall wrote:
> Hi Jan,
>
> On 12/06/2017 12:58 PM, Jan Beulich wrote:
>>>>> On 06.12.17 at 12:47, <julien.grall@linaro.org> wrote:
>>> On 12/06/2017 11:45 AM, Jan Beulich wrote:
>>>>>>> On 06.12.17 at 10:47, <julien.grall@linaro.org> wrote:
>>>>> I guess I have been CCed because you would like this patch is
>>>>> fixing the
>>>>> regression you mentioned on IRC?
>>>>
>>>> Yes, but first of all we need to see whether the issue goes away in
>>>> master once the patch is in.
>>>
>>> Would reverting the offending patch in Xen 4.10 be solution?
>>
>> Probably yes, at the price of re-introducing the issue that change
>> did fix. But reverting wouldn't feel right: The change, after all,
>> was not buggy - it merely uncovered the other issue, as far as we
>> can tell.
>
> I understand. I have seen you pushed the fixes in master today. Let
> see how it perform and decide tomorrow what to do.

XenServer testing has identified this bug, and shown the bug to be fixed
with this patch in place.

Therefore, a retroactive Tested-by: Andrew Cooper
<andrew.cooper3@citrix.com>

No other emulation issues seen running windows on this particular piece
of old AMD hardware.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/HVM: don't retain emulated insn cache when exiting back to guest

[Julien Grall]

Hi Andrew,

On 12/06/2017 03:08 PM, Andrew Cooper wrote:
> On 06/12/17 13:04, Julien Grall wrote:
>> Hi Jan,
>>
>> On 12/06/2017 12:58 PM, Jan Beulich wrote:
>>>>>> On 06.12.17 at 12:47, <julien.grall@linaro.org> wrote:
>>>> On 12/06/2017 11:45 AM, Jan Beulich wrote:
>>>>>>>> On 06.12.17 at 10:47, <julien.grall@linaro.org> wrote:
>>>>>> I guess I have been CCed because you would like this patch is
>>>>>> fixing the
>>>>>> regression you mentioned on IRC?
>>>>>
>>>>> Yes, but first of all we need to see whether the issue goes away in
>>>>> master once the patch is in.
>>>>
>>>> Would reverting the offending patch in Xen 4.10 be solution?
>>>
>>> Probably yes, at the price of re-introducing the issue that change
>>> did fix. But reverting wouldn't feel right: The change, after all,
>>> was not buggy - it merely uncovered the other issue, as far as we
>>> can tell.
>>
>> I understand. I have seen you pushed the fixes in master today. Let
>> see how it perform and decide tomorrow what to do.
> 
> XenServer testing has identified this bug, and shown the bug to be fixed
> with this patch in place.
> 
> Therefore, a retroactive Tested-by: Andrew Cooper
> <andrew.cooper3@citrix.com>
> 
> No other emulation issues seen running windows on this particular piece
> of old AMD hardware.

Thank you for doing regresison tests! On that basis:

Release-acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel