xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] x86/HVM: avoid full linear->phys translations more frequently
@ 2016-06-08 13:04 Jan Beulich
  2016-06-08 13:09 ` [PATCH 1/2] x86/HVM: latch linear->phys translation results Jan Beulich
  2016-06-08 13:10 ` [PATCH 2/2] x86/HVM: use available linear->phys translations in REP MOVS/STOS handling Jan Beulich
  0 siblings, 2 replies; 10+ messages in thread
From: Jan Beulich @ 2016-06-08 13:04 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant

1: latch linear->phys translation results
2: use available linear->phys translations in REP MOVS/STOS handling

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


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/2] x86/HVM: latch linear->phys translation results
  2016-06-08 13:04 [PATCH 0/2] x86/HVM: avoid full linear->phys translations more frequently Jan Beulich
@ 2016-06-08 13:09 ` Jan Beulich
  2016-06-09 11:54   ` Andrew Cooper
  2016-06-10 15:17   ` Paul Durrant
  2016-06-08 13:10 ` [PATCH 2/2] x86/HVM: use available linear->phys translations in REP MOVS/STOS handling Jan Beulich
  1 sibling, 2 replies; 10+ messages in thread
From: Jan Beulich @ 2016-06-08 13:09 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant

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

... to avoid re-doing the same translation later again (in a retry, for
example). This doesn't help very often according to my testing, but
it's pretty cheap to have, and will be of further use subsequently.

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

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -678,6 +678,19 @@ static struct hvm_mmio_cache *hvmemul_fi
     return cache;
 }
 
+static void latch_linear_to_phys(struct hvm_vcpu_io *vio, unsigned long gla,
+                                 unsigned long gpa, bool_t write)
+{
+    if ( vio->mmio_access.gla_valid )
+        return;
+
+    vio->mmio_gva = gla & PAGE_MASK;
+    vio->mmio_gpfn = PFN_DOWN(gpa);
+    vio->mmio_access = (struct npfec){ .gla_valid = 1,
+                                       .read_access = 1,
+                                       .write_access = write };
+}
+
 static int hvmemul_linear_mmio_access(
     unsigned long gla, unsigned int size, uint8_t dir, void *buffer,
     uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt, bool_t known_gpfn)
@@ -703,6 +716,8 @@ static int hvmemul_linear_mmio_access(
                                     hvmemul_ctxt);
         if ( rc != X86EMUL_OKAY )
             return rc;
+
+        latch_linear_to_phys(vio, gla, gpa, dir == IOREQ_WRITE);
     }
 
     for ( ;; )




[-- Attachment #2: x86-HVM-emul-store-addresses.patch --]
[-- Type: text/plain, Size: 1413 bytes --]

x86/HVM: latch linear->phys translation results

... to avoid re-doing the same translation later again (in a retry, for
example). This doesn't help very often according to my testing, but
it's pretty cheap to have, and will be of further use subsequently.

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

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -678,6 +678,19 @@ static struct hvm_mmio_cache *hvmemul_fi
     return cache;
 }
 
+static void latch_linear_to_phys(struct hvm_vcpu_io *vio, unsigned long gla,
+                                 unsigned long gpa, bool_t write)
+{
+    if ( vio->mmio_access.gla_valid )
+        return;
+
+    vio->mmio_gva = gla & PAGE_MASK;
+    vio->mmio_gpfn = PFN_DOWN(gpa);
+    vio->mmio_access = (struct npfec){ .gla_valid = 1,
+                                       .read_access = 1,
+                                       .write_access = write };
+}
+
 static int hvmemul_linear_mmio_access(
     unsigned long gla, unsigned int size, uint8_t dir, void *buffer,
     uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt, bool_t known_gpfn)
@@ -703,6 +716,8 @@ static int hvmemul_linear_mmio_access(
                                     hvmemul_ctxt);
         if ( rc != X86EMUL_OKAY )
             return rc;
+
+        latch_linear_to_phys(vio, gla, gpa, dir == IOREQ_WRITE);
     }
 
     for ( ;; )

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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] x86/HVM: use available linear->phys translations in REP MOVS/STOS handling
  2016-06-08 13:04 [PATCH 0/2] x86/HVM: avoid full linear->phys translations more frequently Jan Beulich
  2016-06-08 13:09 ` [PATCH 1/2] x86/HVM: latch linear->phys translation results Jan Beulich
@ 2016-06-08 13:10 ` Jan Beulich
  2016-06-10 15:17   ` Paul Durrant
  1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2016-06-08 13:10 UTC (permalink / raw)
  To: xen-devel; +Cc: Paul Durrant

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

If we have the translation result available already, we should also use
is here. In my tests with Linux guests this eliminates all calls to
hvmemul_linear_to_phys() out of the two functions being changed.

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

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1156,6 +1156,7 @@ static int hvmemul_rep_movs(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
     unsigned long saddr, daddr, bytes;
     paddr_t sgpa, dgpa;
     uint32_t pfec = PFEC_page_present;
@@ -1178,16 +1179,43 @@ static int hvmemul_rep_movs(
     if ( hvmemul_ctxt->seg_reg[x86_seg_ss].attr.fields.dpl == 3 )
         pfec |= PFEC_user_mode;
 
-    rc = hvmemul_linear_to_phys(
-        saddr, &sgpa, bytes_per_rep, reps, pfec, hvmemul_ctxt);
-    if ( rc != X86EMUL_OKAY )
-        return rc;
+    bytes = PAGE_SIZE - (saddr & ~PAGE_MASK);
+    if ( vio->mmio_access.read_access &&
+         (vio->mmio_gva == (saddr & PAGE_MASK)) &&
+         bytes >= bytes_per_rep )
+    {
+        sgpa = pfn_to_paddr(vio->mmio_gpfn) | (saddr & ~PAGE_MASK);
+        if ( *reps * bytes_per_rep > bytes )
+            *reps = bytes / bytes_per_rep;
+    }
+    else
+    {
+        rc = hvmemul_linear_to_phys(saddr, &sgpa, bytes_per_rep, reps, pfec,
+                                    hvmemul_ctxt);
+        if ( rc != X86EMUL_OKAY )
+            return rc;
 
-    rc = hvmemul_linear_to_phys(
-        daddr, &dgpa, bytes_per_rep, reps,
-        pfec | PFEC_write_access, hvmemul_ctxt);
-    if ( rc != X86EMUL_OKAY )
-        return rc;
+        latch_linear_to_phys(vio, saddr, sgpa, 0);
+    }
+
+    bytes = PAGE_SIZE - (daddr & ~PAGE_MASK);
+    if ( vio->mmio_access.write_access &&
+         (vio->mmio_gva == (daddr & PAGE_MASK)) &&
+         bytes >= bytes_per_rep )
+    {
+        dgpa = pfn_to_paddr(vio->mmio_gpfn) | (daddr & ~PAGE_MASK);
+        if ( *reps * bytes_per_rep > bytes )
+            *reps = bytes / bytes_per_rep;
+    }
+    else
+    {
+        rc = hvmemul_linear_to_phys(daddr, &dgpa, bytes_per_rep, reps,
+                                    pfec | PFEC_write_access, hvmemul_ctxt);
+        if ( rc != X86EMUL_OKAY )
+            return rc;
+
+        latch_linear_to_phys(vio, daddr, dgpa, 1);
+    }
 
     /* Check for MMIO ops */
     (void) get_gfn_query_unlocked(current->domain, sgpa >> PAGE_SHIFT, &sp2mt);
@@ -1279,25 +1307,40 @@ static int hvmemul_rep_stos(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
-    unsigned long addr;
+    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
+    unsigned long addr, bytes;
     paddr_t gpa;
     p2m_type_t p2mt;
     bool_t df = !!(ctxt->regs->eflags & X86_EFLAGS_DF);
     int rc = hvmemul_virtual_to_linear(seg, offset, bytes_per_rep, reps,
                                        hvm_access_write, hvmemul_ctxt, &addr);
 
-    if ( rc == X86EMUL_OKAY )
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    bytes = PAGE_SIZE - (addr & ~PAGE_MASK);
+    if ( vio->mmio_access.write_access &&
+         (vio->mmio_gva == (addr & PAGE_MASK)) &&
+         bytes >= bytes_per_rep )
+    {
+        gpa = pfn_to_paddr(vio->mmio_gpfn) | (addr & ~PAGE_MASK);
+        if ( *reps * bytes_per_rep > bytes )
+            *reps = bytes / bytes_per_rep;
+    }
+    else
     {
         uint32_t pfec = PFEC_page_present | PFEC_write_access;
 
         if ( hvmemul_ctxt->seg_reg[x86_seg_ss].attr.fields.dpl == 3 )
             pfec |= PFEC_user_mode;
 
-        rc = hvmemul_linear_to_phys(
-            addr, &gpa, bytes_per_rep, reps, pfec, hvmemul_ctxt);
+        rc = hvmemul_linear_to_phys(addr, &gpa, bytes_per_rep, reps, pfec,
+                                    hvmemul_ctxt);
+        if ( rc != X86EMUL_OKAY )
+            return rc;
+
+        latch_linear_to_phys(vio, addr, gpa, 1);
     }
-    if ( rc != X86EMUL_OKAY )
-        return rc;
 
     /* Check for MMIO op */
     (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT, &p2mt);



[-- Attachment #2: x86-HVM-emul-REP-use-addresses.patch --]
[-- Type: text/plain, Size: 4361 bytes --]

x86/HVM: use available linear->phys translations in REP MOVS/STOS handling

If we have the translation result available already, we should also use
is here. In my tests with Linux guests this eliminates all calls to
hvmemul_linear_to_phys() out of the two functions being changed.

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

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1156,6 +1156,7 @@ static int hvmemul_rep_movs(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
     unsigned long saddr, daddr, bytes;
     paddr_t sgpa, dgpa;
     uint32_t pfec = PFEC_page_present;
@@ -1178,16 +1179,43 @@ static int hvmemul_rep_movs(
     if ( hvmemul_ctxt->seg_reg[x86_seg_ss].attr.fields.dpl == 3 )
         pfec |= PFEC_user_mode;
 
-    rc = hvmemul_linear_to_phys(
-        saddr, &sgpa, bytes_per_rep, reps, pfec, hvmemul_ctxt);
-    if ( rc != X86EMUL_OKAY )
-        return rc;
+    bytes = PAGE_SIZE - (saddr & ~PAGE_MASK);
+    if ( vio->mmio_access.read_access &&
+         (vio->mmio_gva == (saddr & PAGE_MASK)) &&
+         bytes >= bytes_per_rep )
+    {
+        sgpa = pfn_to_paddr(vio->mmio_gpfn) | (saddr & ~PAGE_MASK);
+        if ( *reps * bytes_per_rep > bytes )
+            *reps = bytes / bytes_per_rep;
+    }
+    else
+    {
+        rc = hvmemul_linear_to_phys(saddr, &sgpa, bytes_per_rep, reps, pfec,
+                                    hvmemul_ctxt);
+        if ( rc != X86EMUL_OKAY )
+            return rc;
 
-    rc = hvmemul_linear_to_phys(
-        daddr, &dgpa, bytes_per_rep, reps,
-        pfec | PFEC_write_access, hvmemul_ctxt);
-    if ( rc != X86EMUL_OKAY )
-        return rc;
+        latch_linear_to_phys(vio, saddr, sgpa, 0);
+    }
+
+    bytes = PAGE_SIZE - (daddr & ~PAGE_MASK);
+    if ( vio->mmio_access.write_access &&
+         (vio->mmio_gva == (daddr & PAGE_MASK)) &&
+         bytes >= bytes_per_rep )
+    {
+        dgpa = pfn_to_paddr(vio->mmio_gpfn) | (daddr & ~PAGE_MASK);
+        if ( *reps * bytes_per_rep > bytes )
+            *reps = bytes / bytes_per_rep;
+    }
+    else
+    {
+        rc = hvmemul_linear_to_phys(daddr, &dgpa, bytes_per_rep, reps,
+                                    pfec | PFEC_write_access, hvmemul_ctxt);
+        if ( rc != X86EMUL_OKAY )
+            return rc;
+
+        latch_linear_to_phys(vio, daddr, dgpa, 1);
+    }
 
     /* Check for MMIO ops */
     (void) get_gfn_query_unlocked(current->domain, sgpa >> PAGE_SHIFT, &sp2mt);
@@ -1279,25 +1307,40 @@ static int hvmemul_rep_stos(
 {
     struct hvm_emulate_ctxt *hvmemul_ctxt =
         container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
-    unsigned long addr;
+    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
+    unsigned long addr, bytes;
     paddr_t gpa;
     p2m_type_t p2mt;
     bool_t df = !!(ctxt->regs->eflags & X86_EFLAGS_DF);
     int rc = hvmemul_virtual_to_linear(seg, offset, bytes_per_rep, reps,
                                        hvm_access_write, hvmemul_ctxt, &addr);
 
-    if ( rc == X86EMUL_OKAY )
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    bytes = PAGE_SIZE - (addr & ~PAGE_MASK);
+    if ( vio->mmio_access.write_access &&
+         (vio->mmio_gva == (addr & PAGE_MASK)) &&
+         bytes >= bytes_per_rep )
+    {
+        gpa = pfn_to_paddr(vio->mmio_gpfn) | (addr & ~PAGE_MASK);
+        if ( *reps * bytes_per_rep > bytes )
+            *reps = bytes / bytes_per_rep;
+    }
+    else
     {
         uint32_t pfec = PFEC_page_present | PFEC_write_access;
 
         if ( hvmemul_ctxt->seg_reg[x86_seg_ss].attr.fields.dpl == 3 )
             pfec |= PFEC_user_mode;
 
-        rc = hvmemul_linear_to_phys(
-            addr, &gpa, bytes_per_rep, reps, pfec, hvmemul_ctxt);
+        rc = hvmemul_linear_to_phys(addr, &gpa, bytes_per_rep, reps, pfec,
+                                    hvmemul_ctxt);
+        if ( rc != X86EMUL_OKAY )
+            return rc;
+
+        latch_linear_to_phys(vio, addr, gpa, 1);
     }
-    if ( rc != X86EMUL_OKAY )
-        return rc;
 
     /* Check for MMIO op */
     (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT, &p2mt);

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

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] x86/HVM: latch linear->phys translation results
  2016-06-08 13:09 ` [PATCH 1/2] x86/HVM: latch linear->phys translation results Jan Beulich
@ 2016-06-09 11:54   ` Andrew Cooper
  2016-06-09 12:13     ` Jan Beulich
  2016-06-20 13:12     ` Tim Deegan
  2016-06-10 15:17   ` Paul Durrant
  1 sibling, 2 replies; 10+ messages in thread
From: Andrew Cooper @ 2016-06-09 11:54 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Paul Durrant

On 08/06/16 14:09, Jan Beulich wrote:
> ... to avoid re-doing the same translation later again (in a retry, for
> example). This doesn't help very often according to my testing, but
> it's pretty cheap to have, and will be of further use subsequently.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -678,6 +678,19 @@ static struct hvm_mmio_cache *hvmemul_fi
>      return cache;
>  }
>  
> +static void latch_linear_to_phys(struct hvm_vcpu_io *vio, unsigned long gla,
> +                                 unsigned long gpa, bool_t write)
> +{
> +    if ( vio->mmio_access.gla_valid )
> +        return;
> +
> +    vio->mmio_gva = gla & PAGE_MASK;

This suggest that mmio_vga is mis-named.

Looking at the uses, handle_mmio_with_translation() is used
inconsistently, with virtual addresses from the shadow code, but linear
addresses from nested hap code.

Clearly one of these two users are buggy for guests running in a non
flat way, and it looks to be the shadow side which is buggy.


Following my recent fun with invlpg handling, I am much more conscious
about the distinction between virtual and linear addresses.  I wonder if
we want to go so far as to have a TYPE_SAFE() for it, to try and avoid
further misuse?

~Andrew

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] x86/HVM: latch linear->phys translation results
  2016-06-09 11:54   ` Andrew Cooper
@ 2016-06-09 12:13     ` Jan Beulich
  2016-06-14 10:29       ` Andrew Cooper
  2016-06-20 13:12     ` Tim Deegan
  1 sibling, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2016-06-09 12:13 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Paul Durrant

>>> On 09.06.16 at 13:54, <andrew.cooper3@citrix.com> wrote:
> On 08/06/16 14:09, Jan Beulich wrote:
>> ... to avoid re-doing the same translation later again (in a retry, for
>> example). This doesn't help very often according to my testing, but
>> it's pretty cheap to have, and will be of further use subsequently.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>
>> --- a/xen/arch/x86/hvm/emulate.c
>> +++ b/xen/arch/x86/hvm/emulate.c
>> @@ -678,6 +678,19 @@ static struct hvm_mmio_cache *hvmemul_fi
>>      return cache;
>>  }
>>  
>> +static void latch_linear_to_phys(struct hvm_vcpu_io *vio, unsigned long gla,
>> +                                 unsigned long gpa, bool_t write)
>> +{
>> +    if ( vio->mmio_access.gla_valid )
>> +        return;
>> +
>> +    vio->mmio_gva = gla & PAGE_MASK;
> 
> This suggest that mmio_vga is mis-named.
> 
> Looking at the uses, handle_mmio_with_translation() is used
> inconsistently, with virtual addresses from the shadow code, but linear
> addresses from nested hap code.
> 
> Clearly one of these two users are buggy for guests running in a non
> flat way, and it looks to be the shadow side which is buggy.

Right - this field is certainly meant to be a linear address (as all
segment information is gone by the time we get here). But I can't
seem to see an issue with the shadow instance: The "virtual"
address here is the CR2 value of a #PF, which clearly is a linear
address.

And anyway all you say is orthogonal to the change here.

> Following my recent fun with invlpg handling, I am much more conscious
> about the distinction between virtual and linear addresses.  I wonder if
> we want to go so far as to have a TYPE_SAFE() for it, to try and avoid
> further misuse?

Not sure; if you're up to it, I wouldn't mind, but maybe an
alternative would be to have a (segment,offset) container
instead for virtual addresses, such that only linear addresses
can get passed as plain number?

Jan


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] x86/HVM: latch linear->phys translation results
  2016-06-08 13:09 ` [PATCH 1/2] x86/HVM: latch linear->phys translation results Jan Beulich
  2016-06-09 11:54   ` Andrew Cooper
@ 2016-06-10 15:17   ` Paul Durrant
  1 sibling, 0 replies; 10+ messages in thread
From: Paul Durrant @ 2016-06-10 15:17 UTC (permalink / raw)
  To: Jan Beulich, xen-devel

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 08 June 2016 14:10
> To: xen-devel
> Cc: Paul Durrant
> Subject: [PATCH 1/2] x86/HVM: latch linear->phys translation results
> 
> ... to avoid re-doing the same translation later again (in a retry, for
> example). This doesn't help very often according to my testing, but
> it's pretty cheap to have, and will be of further use subsequently.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -678,6 +678,19 @@ static struct hvm_mmio_cache *hvmemul_fi
>      return cache;
>  }
> 
> +static void latch_linear_to_phys(struct hvm_vcpu_io *vio, unsigned long
> gla,
> +                                 unsigned long gpa, bool_t write)
> +{
> +    if ( vio->mmio_access.gla_valid )
> +        return;
> +
> +    vio->mmio_gva = gla & PAGE_MASK;
> +    vio->mmio_gpfn = PFN_DOWN(gpa);
> +    vio->mmio_access = (struct npfec){ .gla_valid = 1,
> +                                       .read_access = 1,
> +                                       .write_access = write };
> +}
> +
>  static int hvmemul_linear_mmio_access(
>      unsigned long gla, unsigned int size, uint8_t dir, void *buffer,
>      uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt, bool_t
> known_gpfn)
> @@ -703,6 +716,8 @@ static int hvmemul_linear_mmio_access(
>                                      hvmemul_ctxt);
>          if ( rc != X86EMUL_OKAY )
>              return rc;
> +
> +        latch_linear_to_phys(vio, gla, gpa, dir == IOREQ_WRITE);
>      }
> 
>      for ( ;; )
> 
> 


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] x86/HVM: use available linear->phys translations in REP MOVS/STOS handling
  2016-06-08 13:10 ` [PATCH 2/2] x86/HVM: use available linear->phys translations in REP MOVS/STOS handling Jan Beulich
@ 2016-06-10 15:17   ` Paul Durrant
  0 siblings, 0 replies; 10+ messages in thread
From: Paul Durrant @ 2016-06-10 15:17 UTC (permalink / raw)
  To: Jan Beulich, xen-devel

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 08 June 2016 14:10
> To: xen-devel
> Cc: Paul Durrant
> Subject: [PATCH 2/2] x86/HVM: use available linear->phys translations in REP
> MOVS/STOS handling
> 
> If we have the translation result available already, we should also use
> is here. In my tests with Linux guests this eliminates all calls to
> hvmemul_linear_to_phys() out of the two functions being changed.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1156,6 +1156,7 @@ static int hvmemul_rep_movs(
>  {
>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> +    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
>      unsigned long saddr, daddr, bytes;
>      paddr_t sgpa, dgpa;
>      uint32_t pfec = PFEC_page_present;
> @@ -1178,16 +1179,43 @@ static int hvmemul_rep_movs(
>      if ( hvmemul_ctxt->seg_reg[x86_seg_ss].attr.fields.dpl == 3 )
>          pfec |= PFEC_user_mode;
> 
> -    rc = hvmemul_linear_to_phys(
> -        saddr, &sgpa, bytes_per_rep, reps, pfec, hvmemul_ctxt);
> -    if ( rc != X86EMUL_OKAY )
> -        return rc;
> +    bytes = PAGE_SIZE - (saddr & ~PAGE_MASK);
> +    if ( vio->mmio_access.read_access &&
> +         (vio->mmio_gva == (saddr & PAGE_MASK)) &&
> +         bytes >= bytes_per_rep )
> +    {
> +        sgpa = pfn_to_paddr(vio->mmio_gpfn) | (saddr & ~PAGE_MASK);
> +        if ( *reps * bytes_per_rep > bytes )
> +            *reps = bytes / bytes_per_rep;
> +    }
> +    else
> +    {
> +        rc = hvmemul_linear_to_phys(saddr, &sgpa, bytes_per_rep, reps, pfec,
> +                                    hvmemul_ctxt);
> +        if ( rc != X86EMUL_OKAY )
> +            return rc;
> 
> -    rc = hvmemul_linear_to_phys(
> -        daddr, &dgpa, bytes_per_rep, reps,
> -        pfec | PFEC_write_access, hvmemul_ctxt);
> -    if ( rc != X86EMUL_OKAY )
> -        return rc;
> +        latch_linear_to_phys(vio, saddr, sgpa, 0);
> +    }
> +
> +    bytes = PAGE_SIZE - (daddr & ~PAGE_MASK);
> +    if ( vio->mmio_access.write_access &&
> +         (vio->mmio_gva == (daddr & PAGE_MASK)) &&
> +         bytes >= bytes_per_rep )
> +    {
> +        dgpa = pfn_to_paddr(vio->mmio_gpfn) | (daddr & ~PAGE_MASK);
> +        if ( *reps * bytes_per_rep > bytes )
> +            *reps = bytes / bytes_per_rep;
> +    }
> +    else
> +    {
> +        rc = hvmemul_linear_to_phys(daddr, &dgpa, bytes_per_rep, reps,
> +                                    pfec | PFEC_write_access, hvmemul_ctxt);
> +        if ( rc != X86EMUL_OKAY )
> +            return rc;
> +
> +        latch_linear_to_phys(vio, daddr, dgpa, 1);
> +    }
> 
>      /* Check for MMIO ops */
>      (void) get_gfn_query_unlocked(current->domain, sgpa >> PAGE_SHIFT,
> &sp2mt);
> @@ -1279,25 +1307,40 @@ static int hvmemul_rep_stos(
>  {
>      struct hvm_emulate_ctxt *hvmemul_ctxt =
>          container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> -    unsigned long addr;
> +    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
> +    unsigned long addr, bytes;
>      paddr_t gpa;
>      p2m_type_t p2mt;
>      bool_t df = !!(ctxt->regs->eflags & X86_EFLAGS_DF);
>      int rc = hvmemul_virtual_to_linear(seg, offset, bytes_per_rep, reps,
>                                         hvm_access_write, hvmemul_ctxt, &addr);
> 
> -    if ( rc == X86EMUL_OKAY )
> +    if ( rc != X86EMUL_OKAY )
> +        return rc;
> +
> +    bytes = PAGE_SIZE - (addr & ~PAGE_MASK);
> +    if ( vio->mmio_access.write_access &&
> +         (vio->mmio_gva == (addr & PAGE_MASK)) &&
> +         bytes >= bytes_per_rep )
> +    {
> +        gpa = pfn_to_paddr(vio->mmio_gpfn) | (addr & ~PAGE_MASK);
> +        if ( *reps * bytes_per_rep > bytes )
> +            *reps = bytes / bytes_per_rep;
> +    }
> +    else
>      {
>          uint32_t pfec = PFEC_page_present | PFEC_write_access;
> 
>          if ( hvmemul_ctxt->seg_reg[x86_seg_ss].attr.fields.dpl == 3 )
>              pfec |= PFEC_user_mode;
> 
> -        rc = hvmemul_linear_to_phys(
> -            addr, &gpa, bytes_per_rep, reps, pfec, hvmemul_ctxt);
> +        rc = hvmemul_linear_to_phys(addr, &gpa, bytes_per_rep, reps, pfec,
> +                                    hvmemul_ctxt);
> +        if ( rc != X86EMUL_OKAY )
> +            return rc;
> +
> +        latch_linear_to_phys(vio, addr, gpa, 1);
>      }
> -    if ( rc != X86EMUL_OKAY )
> -        return rc;
> 
>      /* Check for MMIO op */
>      (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT,
> &p2mt);
> 


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] x86/HVM: latch linear->phys translation results
  2016-06-09 12:13     ` Jan Beulich
@ 2016-06-14 10:29       ` Andrew Cooper
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2016-06-14 10:29 UTC (permalink / raw)
  To: Jan Beulich; +Cc: xen-devel, Paul Durrant

On 09/06/16 13:13, Jan Beulich wrote:
>>>> On 09.06.16 at 13:54, <andrew.cooper3@citrix.com> wrote:
>> On 08/06/16 14:09, Jan Beulich wrote:
>>> ... to avoid re-doing the same translation later again (in a retry, for
>>> example). This doesn't help very often according to my testing, but
>>> it's pretty cheap to have, and will be of further use subsequently.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -678,6 +678,19 @@ static struct hvm_mmio_cache *hvmemul_fi
>>>      return cache;
>>>  }
>>>  
>>> +static void latch_linear_to_phys(struct hvm_vcpu_io *vio, unsigned long gla,
>>> +                                 unsigned long gpa, bool_t write)
>>> +{
>>> +    if ( vio->mmio_access.gla_valid )
>>> +        return;
>>> +
>>> +    vio->mmio_gva = gla & PAGE_MASK;
>> This suggest that mmio_vga is mis-named.
>>
>> Looking at the uses, handle_mmio_with_translation() is used
>> inconsistently, with virtual addresses from the shadow code, but linear
>> addresses from nested hap code.
>>
>> Clearly one of these two users are buggy for guests running in a non
>> flat way, and it looks to be the shadow side which is buggy.
> Right - this field is certainly meant to be a linear address (as all
> segment information is gone by the time we get here). But I can't
> seem to see an issue with the shadow instance: The "virtual"
> address here is the CR2 value of a #PF, which clearly is a linear
> address.
>
> And anyway all you say is orthogonal to the change here.

Right.  Still, given that there are 4 instances of mmio_gva, renaming it
to mmio_gla for correctness wouldn't be difficult.

That or it can be deferred to a cleanup patch.

Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
>> Following my recent fun with invlpg handling, I am much more conscious
>> about the distinction between virtual and linear addresses.  I wonder if
>> we want to go so far as to have a TYPE_SAFE() for it, to try and avoid
>> further misuse?
> Not sure; if you're up to it, I wouldn't mind, but maybe an
> alternative would be to have a (segment,offset) container
> instead for virtual addresses, such that only linear addresses
> can get passed as plain number?

That is a much better idea.  There is a whole lot of cleanup needed,
relating to this.

~Andrew


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] x86/HVM: latch linear->phys translation results
  2016-06-09 11:54   ` Andrew Cooper
  2016-06-09 12:13     ` Jan Beulich
@ 2016-06-20 13:12     ` Tim Deegan
  2016-06-20 13:44       ` Andrew Cooper
  1 sibling, 1 reply; 10+ messages in thread
From: Tim Deegan @ 2016-06-20 13:12 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: xen-devel, Paul Durrant, Jan Beulich

At 12:54 +0100 on 09 Jun (1465476894), Andrew Cooper wrote:
> On 08/06/16 14:09, Jan Beulich wrote:
> > ... to avoid re-doing the same translation later again (in a retry, for
> > example). This doesn't help very often according to my testing, but
> > it's pretty cheap to have, and will be of further use subsequently.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >
> > --- a/xen/arch/x86/hvm/emulate.c
> > +++ b/xen/arch/x86/hvm/emulate.c
> > @@ -678,6 +678,19 @@ static struct hvm_mmio_cache *hvmemul_fi
> >      return cache;
> >  }
> >  
> > +static void latch_linear_to_phys(struct hvm_vcpu_io *vio, unsigned long gla,
> > +                                 unsigned long gpa, bool_t write)
> > +{
> > +    if ( vio->mmio_access.gla_valid )
> > +        return;
> > +
> > +    vio->mmio_gva = gla & PAGE_MASK;
> 
> This suggest that mmio_vga is mis-named.
> 
> Looking at the uses, handle_mmio_with_translation() is used
> inconsistently, with virtual addresses from the shadow code, but linear
> addresses from nested hap code.
> 
> Clearly one of these two users are buggy for guests running in a non
> flat way, and it looks to be the shadow side which is buggy.

Yes, the naming in the shadow code is incorrect.  Shadow code, along
with a lot of Xen code, uses "virtual" to refer to what the manuals
call linear addresses, i.e. the inputs to paging.  IIRC it was only
with the introduction of HAP hardware interfaces that we started using
the term "linear" widely in Xen code.

I will ack a mechanical renaming if you like, though beware of public
interfaces with the old name, and common code ("linear" being an x86
term).

Tim.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] x86/HVM: latch linear->phys translation results
  2016-06-20 13:12     ` Tim Deegan
@ 2016-06-20 13:44       ` Andrew Cooper
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2016-06-20 13:44 UTC (permalink / raw)
  To: Tim Deegan; +Cc: xen-devel, Paul Durrant, Jan Beulich

On 20/06/16 14:12, Tim Deegan wrote:
> At 12:54 +0100 on 09 Jun (1465476894), Andrew Cooper wrote:
>> On 08/06/16 14:09, Jan Beulich wrote:
>>> ... to avoid re-doing the same translation later again (in a retry, for
>>> example). This doesn't help very often according to my testing, but
>>> it's pretty cheap to have, and will be of further use subsequently.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> --- a/xen/arch/x86/hvm/emulate.c
>>> +++ b/xen/arch/x86/hvm/emulate.c
>>> @@ -678,6 +678,19 @@ static struct hvm_mmio_cache *hvmemul_fi
>>>      return cache;
>>>  }
>>>  
>>> +static void latch_linear_to_phys(struct hvm_vcpu_io *vio, unsigned long gla,
>>> +                                 unsigned long gpa, bool_t write)
>>> +{
>>> +    if ( vio->mmio_access.gla_valid )
>>> +        return;
>>> +
>>> +    vio->mmio_gva = gla & PAGE_MASK;
>> This suggest that mmio_vga is mis-named.
>>
>> Looking at the uses, handle_mmio_with_translation() is used
>> inconsistently, with virtual addresses from the shadow code, but linear
>> addresses from nested hap code.
>>
>> Clearly one of these two users are buggy for guests running in a non
>> flat way, and it looks to be the shadow side which is buggy.
> Yes, the naming in the shadow code is incorrect.  Shadow code, along
> with a lot of Xen code, uses "virtual" to refer to what the manuals
> call linear addresses, i.e. the inputs to paging.  IIRC it was only
> with the introduction of HAP hardware interfaces that we started using
> the term "linear" widely in Xen code.
>
> I will ack a mechanical renaming if you like, though beware of public
> interfaces with the old name, and common code ("linear" being an x86
> term).

I will be doing some cleanup in due course, although I don't have enough
time to do this right now.

~Andrew

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2016-06-20 13:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-08 13:04 [PATCH 0/2] x86/HVM: avoid full linear->phys translations more frequently Jan Beulich
2016-06-08 13:09 ` [PATCH 1/2] x86/HVM: latch linear->phys translation results Jan Beulich
2016-06-09 11:54   ` Andrew Cooper
2016-06-09 12:13     ` Jan Beulich
2016-06-14 10:29       ` Andrew Cooper
2016-06-20 13:12     ` Tim Deegan
2016-06-20 13:44       ` Andrew Cooper
2016-06-10 15:17   ` Paul Durrant
2016-06-08 13:10 ` [PATCH 2/2] x86/HVM: use available linear->phys translations in REP MOVS/STOS handling Jan Beulich
2016-06-10 15:17   ` Paul Durrant

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).