RE: [Xen-changelog] Fix MOVS instruction emulation for HVM MMIO.

RE: [Xen-changelog] Fix MOVS instruction emulation for HVM MMIO.

[Petersson, Mats]

> -----Original Message-----
> From: xen-changelog-bounces@lists.xensource.com 
> [mailto:xen-changelog-bounces@lists.xensource.com] On Behalf 
> Of Xen patchbot-unstable
> Sent: 16 May 2006 22:30
> To: xen-changelog@lists.xensource.com
> Subject: [Xen-changelog] Fix MOVS instruction emulation for HVM MMIO.
> 
> # HG changeset patch
> # User kaf24@firebug.cl.cam.ac.uk
> # Node ID 7fdc4a8b782b1e17fc473d418236ab44cc31b35f
> # Parent  aab3cd33d2ba65340d355ffbfc859ef6e7b64df3
> Fix MOVS instruction emulation for HVM MMIO.
> From: Gerd Hoffman
> Signed-off-by: Keir Fraser <keir@xensource.com>
> ---
>  xen/arch/x86/hvm/platform.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff -r aab3cd33d2ba -r 7fdc4a8b782b xen/arch/x86/hvm/platform.c
> --- a/xen/arch/x86/hvm/platform.c	Tue May 16 16:34:27 2006 +0100
> +++ b/xen/arch/x86/hvm/platform.c	Tue May 16 19:50:23 2006 +0100
> @@ -865,7 +865,7 @@ void handle_mmio(unsigned long va, unsig
>           * copy ourself. After this copy succeeds, "rep 
> movs" is executed
>           * again.
>           */
> -        if ((addr & PAGE_MASK) != ((addr + size - 1) & PAGE_MASK)) {
> +        if ((addr & PAGE_MASK) != ((addr + sign * (size - 1)) & 
> + PAGE_MASK)) {

With the risk of being wrong (again), I'd say this is incorrect: The
MOVS instruction will start reading at ESI, and write at the address
indicated by EDI and write with size bytes, even when it's copying
backwards. So there should be no multiplication of sign on this line. 

I'm sorry I missed this the first time I looked at the patch the first
time and said "That looks right". 

The rest of the patch looks fine to me.

--
Mats
>              unsigned long value = 0;
>  
>              mmio_opp->flags |= OVERLAP; @@ -876,7 +876,7 @@ 
> void handle_mmio(unsigned long va, unsig
>                  hvm_copy(&value, addr, size, HVM_COPY_IN);
>              send_mmio_req(IOREQ_TYPE_COPY, gpa, 1, size, 
> value, dir, 0);
>          } else {
> -            if ((addr & PAGE_MASK) != ((addr + count * size 
> - 1) & PAGE_MASK)) {
> +            if ((addr & PAGE_MASK) != ((addr + sign * (count 
> * size - 
> + 1)) & PAGE_MASK)) {
>                  regs->eip -= inst_len; /* do not advance %eip */
>  
>                  if (sign > 0)
> 
> _______________________________________________
> Xen-changelog mailing list
> Xen-changelog@lists.xensource.com
> http://lists.xensource.com/xen-changelog
> 
> 

Re: RE: [Xen-changelog] Fix MOVS instruction emulation for HVM MMIO.

[Gerd Hoffmann]

Petersson, Mats wrote:
>> Subject: [Xen-changelog] Fix MOVS instruction emulation for HVM MMIO.
>>
>> diff -r aab3cd33d2ba -r 7fdc4a8b782b xen/arch/x86/hvm/platform.c
>> --- a/xen/arch/x86/hvm/platform.c	Tue May 16 16:34:27 2006 +0100
>> +++ b/xen/arch/x86/hvm/platform.c	Tue May 16 19:50:23 2006 +0100
>> @@ -865,7 +865,7 @@ void handle_mmio(unsigned long va, unsig
>>           * copy ourself. After this copy succeeds, "rep 
>> movs" is executed
>>           * again.
>>           */
>> -        if ((addr & PAGE_MASK) != ((addr + size - 1) & PAGE_MASK)) {
>> +        if ((addr & PAGE_MASK) != ((addr + sign * (size - 1)) & 
>> + PAGE_MASK)) {
> 
> With the risk of being wrong (again), I'd say this is incorrect: The
> MOVS instruction will start reading at ESI, and write at the address
> indicated by EDI and write with size bytes, even when it's copying
> backwards. So there should be no multiplication of sign on this line. 

I still think this is correct.  If I understand things correctly the
point of the test is to figure whenever the _next_ repz movs interation
will access another page (and if so copy just one data word and let the
emulator kick in again for the remaining data on the page above/below).

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
Erst mal heiraten, ein, zwei Kinder, und wenn alles läuft
geh' ich nach drei Jahren mit der Familie an die Börse.
http://www.suse.de/~kraxel/julika-dora.jpeg

RE: RE: [Xen-changelog] Fix MOVS instruction emulation for HVM MMIO.

[Petersson, Mats]

> -----Original Message-----
> From: xen-devel-bounces@lists.xensource.com 
> [mailto:xen-devel-bounces@lists.xensource.com] On Behalf Of 
> Gerd Hoffmann
> Sent: 17 May 2006 07:43
> To: Petersson, Mats
> Cc: xen-devel@lists.xensource.com; xen-changelog@lists.xensource.com
> Subject: Re: [Xen-devel] RE: [Xen-changelog] Fix MOVS 
> instruction emulation for HVM MMIO.
> 
> Petersson, Mats wrote:
> >> Subject: [Xen-changelog] Fix MOVS instruction emulation 
> for HVM MMIO.
> >>
> >> diff -r aab3cd33d2ba -r 7fdc4a8b782b xen/arch/x86/hvm/platform.c
> >> --- a/xen/arch/x86/hvm/platform.c	Tue May 16 16:34:27 2006 +0100
> >> +++ b/xen/arch/x86/hvm/platform.c	Tue May 16 19:50:23 2006 +0100
> >> @@ -865,7 +865,7 @@ void handle_mmio(unsigned long va, unsig
> >>           * copy ourself. After this copy succeeds, "rep movs" is 
> >> executed
> >>           * again.
> >>           */
> >> -        if ((addr & PAGE_MASK) != ((addr + size - 1) & 
> PAGE_MASK)) {
> >> +        if ((addr & PAGE_MASK) != ((addr + sign * (size - 1)) &
> >> + PAGE_MASK)) {
> > 
> > With the risk of being wrong (again), I'd say this is 
> incorrect: The 
> > MOVS instruction will start reading at ESI, and write at 
> the address 
> > indicated by EDI and write with size bytes, even when it's copying 
> > backwards. So there should be no multiplication of sign on 
> this line.
> 
> I still think this is correct.  If I understand things 
> correctly the point of the test is to figure whenever the 
> _next_ repz movs interation will access another page (and if 
> so copy just one data word and let the emulator kick in again 
> for the remaining data on the page above/below).

No, the test is for if the first (current) operation is crossing a page boundary, not if the NEXT one is... That's why the code in this condition is using hvm_copy to fetch the data that is being accessed... 

But leave it as is right now, I'm still working on getting a piece of test-code to do MOVS in various ways... 

--
Mats
> 
> cheers,
> 
>   Gerd
> 
> --
> Gerd Hoffmann <kraxel@suse.de>
> Erst mal heiraten, ein, zwei Kinder, und wenn alles läuft 
> geh' ich nach drei Jahren mit der Familie an die Börse.
> http://www.suse.de/~kraxel/julika-dora.jpeg
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
> 

Re: RE: [Xen-changelog] Fix MOVS instruction emulation for HVM MMIO.

[Gerd Hoffmann]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

  Hi,

> No, the test is for if the first (current) operation is crossing a
> page boundary, not if the NEXT one is... That's why the code in this
> condition is using hvm_copy to fetch the data that is being
> accessed...

Ok, point taken.  But that also means that the access range calculation
isn't that simple, it's quite different for the two directions ...

Patch against 9681 (testing repository) attached.

cheers,

  Gerd

-- 
Gerd Hoffmann <kraxel@suse.de>
Erst mal heiraten, ein, zwei Kinder, und wenn alles läuft
geh' ich nach drei Jahren mit der Familie an die Börse.
http://www.suse.de/~kraxel/julika-dora.jpeg

[-- Attachment #2: fix --]
[-- Type: text/plain, Size: 2130 bytes --]

diff -r 42eee0575ab7 xen/arch/x86/hvm/platform.c
--- a/xen/arch/x86/hvm/platform.c	Tue May 16 18:52:53 2006
+++ b/xen/arch/x86/hvm/platform.c	Thu May 18 14:09:36 2006
@@ -830,7 +830,7 @@
         unsigned long count = GET_REPEAT_COUNT();
         unsigned long size = mmio_inst.op_size;
         int sign = regs->eflags & EF_DF ? -1 : 1;
-        unsigned long addr = 0;
+        unsigned long addr, high_addr, low_addr;
         int dir;
 
         /* determine non-MMIO address */
@@ -851,6 +851,13 @@
                 addr = regs->edi;
             }
         }
+	if (sign > 0) {
+	    high_addr = addr + count * size - 1;
+	    low_addr  = addr;
+	} else {
+	    high_addr = addr + size - 1;
+	    low_addr  = addr - (count-1) * size;
+	}
 
         mmio_opp->flags = mmio_inst.flags;
         mmio_opp->instr = mmio_inst.instr;
@@ -865,7 +872,8 @@
          * copy ourself. After this copy succeeds, "rep movs" is executed
          * again.
          */
-        if ((addr & PAGE_MASK) != ((addr + sign * (size - 1)) & PAGE_MASK)) {
+        if ((addr & PAGE_MASK) != ((addr + size - 1) & PAGE_MASK)) {
+	    /* one movs crosses page border */
             unsigned long value = 0;
 
             mmio_opp->flags |= OVERLAP;
@@ -876,15 +884,14 @@
                 hvm_copy(&value, addr, size, HVM_COPY_IN);
             send_mmio_req(IOREQ_TYPE_COPY, gpa, 1, size, value, dir, 0);
         } else {
-            if ((addr & PAGE_MASK) != ((addr + sign * (count * size - 1)) & PAGE_MASK)) {
+	    if ((high_addr & PAGE_MASK) != (low_addr & PAGE_MASK)) {
+                /* $count movs will cross page border */
                 regs->eip -= inst_len; /* do not advance %eip */
-
-                if (sign > 0)
-                    count = (PAGE_SIZE - (addr & ~PAGE_MASK)) / size;
-                else
-                    count = (addr & ~PAGE_MASK) / size;
+		if (sign > 0)
+		    count = (PAGE_SIZE - (addr & ~PAGE_MASK)) / size;
+		else
+		    count = ((high_addr + size - 1) & ~PAGE_MASK) / size;
             }
-
             send_mmio_req(IOREQ_TYPE_COPY, gpa, count, size, addr, dir, 1);
         }
         break;

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel