[PATCH v3 1/2] x86/hvm: split all linear reads and writes at page boundary

[PATCH v3 1/2] x86/hvm: split all linear reads and writes at page boundary

[Igor Druzhinin]

Ruling out page straddling at linear level makes it easier to
distinguish chunks that require proper handling as MMIO access
and not complete them as page straddling memory transactions
prematurely. This doesn't change the general behavior.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
Changes in v3:
* new patch in v3 to address the concern of P2M type change along with
  page straddling
---
 xen/arch/x86/hvm/emulate.c | 72 ++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 34 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 754baf6..4879ccb 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -1089,12 +1089,25 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
                        uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt)
 {
     pagefault_info_t pfinfo;
-    int rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
+    unsigned int offset = addr & ~PAGE_MASK;
+    int rc;
 
-    switch ( rc )
+    if ( offset + bytes > PAGE_SIZE )
     {
-        unsigned int offset, part1;
+        unsigned int part1 = PAGE_SIZE - offset;
+
+        /* Split the access at the page boundary. */
+        rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt);
+        if ( rc == X86EMUL_OKAY )
+            rc = linear_read(addr + part1, bytes - part1, p_data + part1,
+                             pfec, hvmemul_ctxt);
+        return rc;
+    }
+
+    rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
 
+    switch ( rc )
+    {
     case HVMTRANS_okay:
         return X86EMUL_OKAY;
 
@@ -1106,20 +1119,9 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
         if ( pfec & PFEC_insn_fetch )
             return X86EMUL_UNHANDLEABLE;
 
-        offset = addr & ~PAGE_MASK;
-        if ( offset + bytes <= PAGE_SIZE )
-            return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec,
-                                            hvmemul_ctxt,
-                                            known_gla(addr, bytes, pfec));
-
-        /* Split the access at the page boundary. */
-        part1 = PAGE_SIZE - offset;
-        rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt);
-        if ( rc == X86EMUL_OKAY )
-            rc = linear_read(addr + part1, bytes - part1, p_data + part1,
-                             pfec, hvmemul_ctxt);
-        return rc;
-
+        return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec,
+                                        hvmemul_ctxt,
+                                        known_gla(addr, bytes, pfec));
     case HVMTRANS_gfn_paged_out:
     case HVMTRANS_gfn_shared:
         return X86EMUL_RETRY;
@@ -1132,12 +1134,25 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data,
                         uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt)
 {
     pagefault_info_t pfinfo;
-    int rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo);
+    unsigned int offset = addr & ~PAGE_MASK;
+    int rc;
 
-    switch ( rc )
+    if ( offset + bytes > PAGE_SIZE )
     {
-        unsigned int offset, part1;
+        unsigned int part1 = PAGE_SIZE - offset;
 
+        /* Split the access at the page boundary. */
+        rc = linear_write(addr, part1, p_data, pfec, hvmemul_ctxt);
+        if ( rc == X86EMUL_OKAY )
+            rc = linear_write(addr + part1, bytes - part1, p_data + part1,
+                              pfec, hvmemul_ctxt);
+        return rc;
+    }
+
+    rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo);
+
+    switch ( rc )
+    {
     case HVMTRANS_okay:
         return X86EMUL_OKAY;
 
@@ -1146,20 +1161,9 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data,
         return X86EMUL_EXCEPTION;
 
     case HVMTRANS_bad_gfn_to_mfn:
-        offset = addr & ~PAGE_MASK;
-        if ( offset + bytes <= PAGE_SIZE )
-            return hvmemul_linear_mmio_write(addr, bytes, p_data, pfec,
-                                             hvmemul_ctxt,
-                                             known_gla(addr, bytes, pfec));
-
-        /* Split the access at the page boundary. */
-        part1 = PAGE_SIZE - offset;
-        rc = linear_write(addr, part1, p_data, pfec, hvmemul_ctxt);
-        if ( rc == X86EMUL_OKAY )
-            rc = linear_write(addr + part1, bytes - part1, p_data + part1,
-                              pfec, hvmemul_ctxt);
-        return rc;
-
+        return hvmemul_linear_mmio_write(addr, bytes, p_data, pfec,
+                                         hvmemul_ctxt,
+                                         known_gla(addr, bytes, pfec));
     case HVMTRANS_gfn_paged_out:
     case HVMTRANS_gfn_shared:
         return X86EMUL_RETRY;
-- 
2.7.4


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

[PATCH v3 2/2] x86/hvm: finish IOREQs correctly on completion path

[Igor Druzhinin]

Since the introduction of linear_{read,write}() helpers in 3bdec530a5
(x86/HVM: split page straddling emulated accesses in more cases) the
completion path for IOREQs has been broken: if there is an IOREQ in
progress but hvm_copy_{to,from}_guest_linear() returns HVMTRANS_okay
(e.g. when P2M type of source/destination has been changed by IOREQ
handler) the execution will never re-enter hvmemul_do_io() where
IOREQs are completed. This usually results in a domain crash upon
the execution of the next IOREQ entering hvmemul_do_io() and finding
the remnants of the previous IOREQ in the state machine.

This particular issue has been discovered in relation to p2m_ioreq_server
type where an emulator changed the memory type between p2m_ioreq_server
and p2m_ram_rw in process of responding to IOREQ which made
hvm_copy_..() to behave differently on the way back.

Fix it for now by checking if IOREQ completion is required (which
can be identified by quering MMIO cache) before trying to finish
a memory access immediately through hvm_copy_..(), re-enter
hvmemul_do_io() otherwise. This change alone addresses IOREQ
completion issue where P2M type is modified in the middle of emulation
but is not enough for a more general case where machine state
arbitrarely changes behind our back.

Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
---
Changes in v3:
* made it more clear that it's still a partial fix in the commit description
* other minor suggestions
---
 xen/arch/x86/hvm/emulate.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 4879ccb..92a9b82 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -952,7 +952,7 @@ static int hvmemul_phys_mmio_access(
  * cache indexed by linear MMIO address.
  */
 static struct hvm_mmio_cache *hvmemul_find_mmio_cache(
-    struct hvm_vcpu_io *vio, unsigned long gla, uint8_t dir)
+    struct hvm_vcpu_io *vio, unsigned long gla, uint8_t dir, bool create)
 {
     unsigned int i;
     struct hvm_mmio_cache *cache;
@@ -966,6 +966,9 @@ static struct hvm_mmio_cache *hvmemul_find_mmio_cache(
             return cache;
     }
 
+    if ( !create )
+        return NULL;
+
     i = vio->mmio_cache_count;
     if( i == ARRAY_SIZE(vio->mmio_cache) )
         return NULL;
@@ -1000,7 +1003,7 @@ static int hvmemul_linear_mmio_access(
 {
     struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
     unsigned long offset = gla & ~PAGE_MASK;
-    struct hvm_mmio_cache *cache = hvmemul_find_mmio_cache(vio, gla, dir);
+    struct hvm_mmio_cache *cache = hvmemul_find_mmio_cache(vio, gla, dir, true);
     unsigned int chunk, buffer_offset = 0;
     paddr_t gpa;
     unsigned long one_rep = 1;
@@ -1089,8 +1092,9 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
                        uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt)
 {
     pagefault_info_t pfinfo;
+    struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
     unsigned int offset = addr & ~PAGE_MASK;
-    int rc;
+    int rc = HVMTRANS_bad_gfn_to_mfn;
 
     if ( offset + bytes > PAGE_SIZE )
     {
@@ -1104,7 +1108,14 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
         return rc;
     }
 
-    rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
+    /*
+     * If there is an MMIO cache entry for that access then we must be re-issuing
+     * an access that was previously handled as MMIO. Thus it is imperative that
+     * we handle this access in the same way to guarantee completion and hence
+     * clean up any interim state.
+     */
+    if ( !hvmemul_find_mmio_cache(vio, addr, IOREQ_READ, false) )
+        rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
 
     switch ( rc )
     {
@@ -1134,8 +1145,9 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data,
                         uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt)
 {
     pagefault_info_t pfinfo;
+    struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
     unsigned int offset = addr & ~PAGE_MASK;
-    int rc;
+    int rc = HVMTRANS_bad_gfn_to_mfn;
 
     if ( offset + bytes > PAGE_SIZE )
     {
@@ -1149,7 +1161,14 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data,
         return rc;
     }
 
-    rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo);
+    /*
+     * If there is an MMIO cache entry for that acces then we must be re-issuing
+     * an access that was previously handled as MMIO. Thus it is imperative that
+     * we handle this access in the same way to guarantee completion and hence
+     * clean up any interim state.
+     */
+    if ( !hvmemul_find_mmio_cache(vio, addr, IOREQ_WRITE, false) )
+        rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo);
 
     switch ( rc )
     {
-- 
2.7.4


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

Re: [PATCH v3 1/2] x86/hvm: split all linear reads and writes at page boundary

[Paul Durrant]

> -----Original Message-----
> From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> Sent: 14 March 2019 22:31
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> Igor Druzhinin <igor.druzhinin@citrix.com>
> Subject: [PATCH v3 1/2] x86/hvm: split all linear reads and writes at page boundary
> 
> Ruling out page straddling at linear level makes it easier to
> distinguish chunks that require proper handling as MMIO access
> and not complete them as page straddling memory transactions
> prematurely. This doesn't change the general behavior.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

I think this makes things easier to reason about.

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

> ---
> Changes in v3:
> * new patch in v3 to address the concern of P2M type change along with
>   page straddling
> ---
>  xen/arch/x86/hvm/emulate.c | 72 ++++++++++++++++++++++++----------------------
>  1 file changed, 38 insertions(+), 34 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 754baf6..4879ccb 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -1089,12 +1089,25 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
>                         uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
>      pagefault_info_t pfinfo;
> -    int rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
> +    unsigned int offset = addr & ~PAGE_MASK;
> +    int rc;
> 
> -    switch ( rc )
> +    if ( offset + bytes > PAGE_SIZE )
>      {
> -        unsigned int offset, part1;
> +        unsigned int part1 = PAGE_SIZE - offset;
> +
> +        /* Split the access at the page boundary. */
> +        rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt);
> +        if ( rc == X86EMUL_OKAY )
> +            rc = linear_read(addr + part1, bytes - part1, p_data + part1,
> +                             pfec, hvmemul_ctxt);
> +        return rc;
> +    }
> +
> +    rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
> 
> +    switch ( rc )
> +    {
>      case HVMTRANS_okay:
>          return X86EMUL_OKAY;
> 
> @@ -1106,20 +1119,9 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
>          if ( pfec & PFEC_insn_fetch )
>              return X86EMUL_UNHANDLEABLE;
> 
> -        offset = addr & ~PAGE_MASK;
> -        if ( offset + bytes <= PAGE_SIZE )
> -            return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec,
> -                                            hvmemul_ctxt,
> -                                            known_gla(addr, bytes, pfec));
> -
> -        /* Split the access at the page boundary. */
> -        part1 = PAGE_SIZE - offset;
> -        rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt);
> -        if ( rc == X86EMUL_OKAY )
> -            rc = linear_read(addr + part1, bytes - part1, p_data + part1,
> -                             pfec, hvmemul_ctxt);
> -        return rc;
> -
> +        return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec,
> +                                        hvmemul_ctxt,
> +                                        known_gla(addr, bytes, pfec));
>      case HVMTRANS_gfn_paged_out:
>      case HVMTRANS_gfn_shared:
>          return X86EMUL_RETRY;
> @@ -1132,12 +1134,25 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data,
>                          uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
>      pagefault_info_t pfinfo;
> -    int rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo);
> +    unsigned int offset = addr & ~PAGE_MASK;
> +    int rc;
> 
> -    switch ( rc )
> +    if ( offset + bytes > PAGE_SIZE )
>      {
> -        unsigned int offset, part1;
> +        unsigned int part1 = PAGE_SIZE - offset;
> 
> +        /* Split the access at the page boundary. */
> +        rc = linear_write(addr, part1, p_data, pfec, hvmemul_ctxt);
> +        if ( rc == X86EMUL_OKAY )
> +            rc = linear_write(addr + part1, bytes - part1, p_data + part1,
> +                              pfec, hvmemul_ctxt);
> +        return rc;
> +    }
> +
> +    rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo);
> +
> +    switch ( rc )
> +    {
>      case HVMTRANS_okay:
>          return X86EMUL_OKAY;
> 
> @@ -1146,20 +1161,9 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data,
>          return X86EMUL_EXCEPTION;
> 
>      case HVMTRANS_bad_gfn_to_mfn:
> -        offset = addr & ~PAGE_MASK;
> -        if ( offset + bytes <= PAGE_SIZE )
> -            return hvmemul_linear_mmio_write(addr, bytes, p_data, pfec,
> -                                             hvmemul_ctxt,
> -                                             known_gla(addr, bytes, pfec));
> -
> -        /* Split the access at the page boundary. */
> -        part1 = PAGE_SIZE - offset;
> -        rc = linear_write(addr, part1, p_data, pfec, hvmemul_ctxt);
> -        if ( rc == X86EMUL_OKAY )
> -            rc = linear_write(addr + part1, bytes - part1, p_data + part1,
> -                              pfec, hvmemul_ctxt);
> -        return rc;
> -
> +        return hvmemul_linear_mmio_write(addr, bytes, p_data, pfec,
> +                                         hvmemul_ctxt,
> +                                         known_gla(addr, bytes, pfec));
>      case HVMTRANS_gfn_paged_out:
>      case HVMTRANS_gfn_shared:
>          return X86EMUL_RETRY;
> --
> 2.7.4


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

Re: [PATCH v3 2/2] x86/hvm: finish IOREQs correctly on completion path

[Paul Durrant]

> -----Original Message-----
> From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> Sent: 14 March 2019 22:31
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; jbeulich@suse.com; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne <roger.pau@citrix.com>;
> Igor Druzhinin <igor.druzhinin@citrix.com>
> Subject: [PATCH v3 2/2] x86/hvm: finish IOREQs correctly on completion path
> 
> Since the introduction of linear_{read,write}() helpers in 3bdec530a5
> (x86/HVM: split page straddling emulated accesses in more cases) the
> completion path for IOREQs has been broken: if there is an IOREQ in
> progress but hvm_copy_{to,from}_guest_linear() returns HVMTRANS_okay
> (e.g. when P2M type of source/destination has been changed by IOREQ
> handler) the execution will never re-enter hvmemul_do_io() where
> IOREQs are completed. This usually results in a domain crash upon
> the execution of the next IOREQ entering hvmemul_do_io() and finding
> the remnants of the previous IOREQ in the state machine.
> 
> This particular issue has been discovered in relation to p2m_ioreq_server
> type where an emulator changed the memory type between p2m_ioreq_server
> and p2m_ram_rw in process of responding to IOREQ which made
> hvm_copy_..() to behave differently on the way back.
> 
> Fix it for now by checking if IOREQ completion is required (which
> can be identified by quering MMIO cache) before trying to finish

^ querying

> a memory access immediately through hvm_copy_..(), re-enter
> hvmemul_do_io() otherwise. This change alone addresses IOREQ
> completion issue where P2M type is modified in the middle of emulation
> but is not enough for a more general case where machine state
> arbitrarely changes behind our back.

^ arbitrarily

> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
> ---
> Changes in v3:
> * made it more clear that it's still a partial fix in the commit description
> * other minor suggestions
> ---
>  xen/arch/x86/hvm/emulate.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index 4879ccb..92a9b82 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -952,7 +952,7 @@ static int hvmemul_phys_mmio_access(
>   * cache indexed by linear MMIO address.
>   */
>  static struct hvm_mmio_cache *hvmemul_find_mmio_cache(
> -    struct hvm_vcpu_io *vio, unsigned long gla, uint8_t dir)
> +    struct hvm_vcpu_io *vio, unsigned long gla, uint8_t dir, bool create)
>  {
>      unsigned int i;
>      struct hvm_mmio_cache *cache;
> @@ -966,6 +966,9 @@ static struct hvm_mmio_cache *hvmemul_find_mmio_cache(
>              return cache;
>      }
> 
> +    if ( !create )
> +        return NULL;
> +
>      i = vio->mmio_cache_count;
>      if( i == ARRAY_SIZE(vio->mmio_cache) )
>          return NULL;
> @@ -1000,7 +1003,7 @@ static int hvmemul_linear_mmio_access(
>  {
>      struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
>      unsigned long offset = gla & ~PAGE_MASK;
> -    struct hvm_mmio_cache *cache = hvmemul_find_mmio_cache(vio, gla, dir);
> +    struct hvm_mmio_cache *cache = hvmemul_find_mmio_cache(vio, gla, dir, true);
>      unsigned int chunk, buffer_offset = 0;
>      paddr_t gpa;
>      unsigned long one_rep = 1;
> @@ -1089,8 +1092,9 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
>                         uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
>      pagefault_info_t pfinfo;
> +    struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
>      unsigned int offset = addr & ~PAGE_MASK;
> -    int rc;
> +    int rc = HVMTRANS_bad_gfn_to_mfn;
> 
>      if ( offset + bytes > PAGE_SIZE )
>      {
> @@ -1104,7 +1108,14 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
>          return rc;
>      }
> 
> -    rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
> +    /*
> +     * If there is an MMIO cache entry for that access then we must be re-issuing

^ s/that/the

> +     * an access that was previously handled as MMIO. Thus it is imperative that
> +     * we handle this access in the same way to guarantee completion and hence
> +     * clean up any interim state.
> +     */
> +    if ( !hvmemul_find_mmio_cache(vio, addr, IOREQ_READ, false) )
> +        rc = hvm_copy_from_guest_linear(p_data, addr, bytes, pfec, &pfinfo);
> 
>      switch ( rc )
>      {
> @@ -1134,8 +1145,9 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data,
>                          uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt)
>  {
>      pagefault_info_t pfinfo;
> +    struct hvm_vcpu_io *vio = &current->arch.hvm.hvm_io;
>      unsigned int offset = addr & ~PAGE_MASK;
> -    int rc;
> +    int rc = HVMTRANS_bad_gfn_to_mfn;
> 
>      if ( offset + bytes > PAGE_SIZE )
>      {
> @@ -1149,7 +1161,14 @@ static int linear_write(unsigned long addr, unsigned int bytes, void *p_data,
>          return rc;
>      }
> 
> -    rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo);
> +    /*
> +     * If there is an MMIO cache entry for that acces then we must be re-issuing

Same here.

With these fixed...

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


> +     * an access that was previously handled as MMIO. Thus it is imperative that
> +     * we handle this access in the same way to guarantee completion and hence
> +     * clean up any interim state.
> +     */
> +    if ( !hvmemul_find_mmio_cache(vio, addr, IOREQ_WRITE, false) )
> +        rc = hvm_copy_to_guest_linear(addr, p_data, bytes, pfec, &pfinfo);
> 
>      switch ( rc )
>      {
> --
> 2.7.4


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

Re: [PATCH v3 1/2] x86/hvm: split all linear reads and writes at page boundary

[Jan Beulich]

>>> On 14.03.19 at 23:30, <igor.druzhinin@citrix.com> wrote:
> Ruling out page straddling at linear level makes it easier to
> distinguish chunks that require proper handling as MMIO access
> and not complete them as page straddling memory transactions
> prematurely. This doesn't change the general behavior.
> 
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one cosmetic aspect taken care of (can be done while
committing):

> @@ -1106,20 +1119,9 @@ static int linear_read(unsigned long addr, unsigned int bytes, void *p_data,
>          if ( pfec & PFEC_insn_fetch )
>              return X86EMUL_UNHANDLEABLE;
>  
> -        offset = addr & ~PAGE_MASK;
> -        if ( offset + bytes <= PAGE_SIZE )
> -            return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec,
> -                                            hvmemul_ctxt,
> -                                            known_gla(addr, bytes, pfec));
> -
> -        /* Split the access at the page boundary. */
> -        part1 = PAGE_SIZE - offset;
> -        rc = linear_read(addr, part1, p_data, pfec, hvmemul_ctxt);
> -        if ( rc == X86EMUL_OKAY )
> -            rc = linear_read(addr + part1, bytes - part1, p_data + part1,
> -                             pfec, hvmemul_ctxt);
> -        return rc;
> -
> +        return hvmemul_linear_mmio_read(addr, bytes, p_data, pfec,
> +                                        hvmemul_ctxt,
> +                                        known_gla(addr, bytes, pfec));
>      case HVMTRANS_gfn_paged_out:

Please retain the blank line above here (and also in the write case).

I notice that sadly the change doesn't allow removing the respective
logic from hvmemul_linear_mmio_access() yet, due to its use by
hvmemul_cmpxchg().

Jan



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

Re: [PATCH v3 2/2] x86/hvm: finish IOREQs correctly on completion path

[Jan Beulich]

>>> On 14.03.19 at 23:30, <igor.druzhinin@citrix.com> wrote:
> Since the introduction of linear_{read,write}() helpers in 3bdec530a5
> (x86/HVM: split page straddling emulated accesses in more cases) the
> completion path for IOREQs has been broken: if there is an IOREQ in
> progress but hvm_copy_{to,from}_guest_linear() returns HVMTRANS_okay
> (e.g. when P2M type of source/destination has been changed by IOREQ
> handler) the execution will never re-enter hvmemul_do_io() where
> IOREQs are completed. This usually results in a domain crash upon
> the execution of the next IOREQ entering hvmemul_do_io() and finding
> the remnants of the previous IOREQ in the state machine.

From an archeological pov I'm not sure you point at the offending
commit: I'd rather expect d7bff2bc00 ("x86/HVM: __hvm_copy()
should not write to p2m_ioreq_server pages") to be the culprit,
which went in two months later.

> This particular issue has been discovered in relation to p2m_ioreq_server
> type where an emulator changed the memory type between p2m_ioreq_server
> and p2m_ram_rw in process of responding to IOREQ which made
> hvm_copy_..() to behave differently on the way back.
> 
> Fix it for now by checking if IOREQ completion is required (which
> can be identified by quering MMIO cache) before trying to finish
> a memory access immediately through hvm_copy_..(), re-enter
> hvmemul_do_io() otherwise. This change alone addresses IOREQ
> completion issue where P2M type is modified in the middle of emulation
> but is not enough for a more general case where machine state
> arbitrarely changes behind our back.

I'm afraid this still claims to address cases which don't get fixed
here. For example, take a page changing _to_ p2m_ioreq_server
behind our backs: You won't find an MMIO cache entry for it,
hvm_copy_to_guest_linear() will fail, and you'll try to issue an
MMIO write when in reality the write was already done (emulated
for whatever other reason, e.g. introspection). This example
may be pretty contrived, but Andrew's ballooning scenario really
applies both ways (balloon-in and balloon-out), while the change
deals only with the balloon-in case.

So while I'm fine with the code change, I'd still like to ask to
further refine the description.

Jan



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

Re: [PATCH v3 2/2] x86/hvm: finish IOREQs correctly on completion path

[Igor Druzhinin]

On 15/03/2019 12:27, Jan Beulich wrote:
>>>> On 14.03.19 at 23:30, <igor.druzhinin@citrix.com> wrote:
>> Since the introduction of linear_{read,write}() helpers in 3bdec530a5
>> (x86/HVM: split page straddling emulated accesses in more cases) the
>> completion path for IOREQs has been broken: if there is an IOREQ in
>> progress but hvm_copy_{to,from}_guest_linear() returns HVMTRANS_okay
>> (e.g. when P2M type of source/destination has been changed by IOREQ
>> handler) the execution will never re-enter hvmemul_do_io() where
>> IOREQs are completed. This usually results in a domain crash upon
>> the execution of the next IOREQ entering hvmemul_do_io() and finding
>> the remnants of the previous IOREQ in the state machine.
> 
> From an archeological pov I'm not sure you point at the offending
> commit: I'd rather expect d7bff2bc00 ("x86/HVM: __hvm_copy()
> should not write to p2m_ioreq_server pages") to be the culprit,
> which went in two months later.
> 
>> This particular issue has been discovered in relation to p2m_ioreq_server
>> type where an emulator changed the memory type between p2m_ioreq_server
>> and p2m_ram_rw in process of responding to IOREQ which made
>> hvm_copy_..() to behave differently on the way back.
>>
>> Fix it for now by checking if IOREQ completion is required (which
>> can be identified by quering MMIO cache) before trying to finish
>> a memory access immediately through hvm_copy_..(), re-enter
>> hvmemul_do_io() otherwise. This change alone addresses IOREQ
>> completion issue where P2M type is modified in the middle of emulation
>> but is not enough for a more general case where machine state
>> arbitrarely changes behind our back.
> 
> I'm afraid this still claims to address cases which don't get fixed
> here. For example, take a page changing _to_ p2m_ioreq_server
> behind our backs: You won't find an MMIO cache entry for it,
> hvm_copy_to_guest_linear() will fail, and you'll try to issue an
> MMIO write when in reality the write was already done (emulated
> for whatever other reason, e.g. introspection). This example
> may be pretty contrived, but Andrew's ballooning scenario really
> applies both ways (balloon-in and balloon-out), while the change
> deals only with the balloon-in case.
> 
> So while I'm fine with the code change, I'd still like to ask to
> further refine the description.

Thanks for clarification. I discussed with Paul - there is definitely
still a hole in general case where 1st half of the instruction is memory
and 2nd half is MMIO and the 1st half is changed *to* MMIO. But it's
hard to deal with these types of accesses without a complete re-write of
MMIO cache into general insn access cache - so to lift it up to
linear_{read,write} layer. I hope my understanding is now correct and
I'll put into the description. Until then the fix should do fine with
scenarios we're seeing.

Igor

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

Re: [PATCH v3 2/2] x86/hvm: finish IOREQs correctly on completion path

[Jan Beulich]

>>> On 15.03.19 at 14:05, <igor.druzhinin@citrix.com> wrote:
> Thanks for clarification. I discussed with Paul - there is definitely
> still a hole in general case where 1st half of the instruction is memory
> and 2nd half is MMIO and the 1st half is changed *to* MMIO. But it's
> hard to deal with these types of accesses without a complete re-write of
> MMIO cache into general insn access cache - so to lift it up to
> linear_{read,write} layer. I hope my understanding is now correct and
> I'll put into the description.

Well, mostly. With patch 1 there's no dependency anymore on an
access to be page straddling afaict. I.e. the scenarios I gave
should apply also to aligned accesses.

> Until then the fix should do fine with scenarios we're seeing.

Indeed.

Jan



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