[PATCH 0/3] x86: XSA-112 follow-ups

[PATCH 0/3] x86: XSA-112 follow-ups

[Jan Beulich]

1: also allow REP STOS emulation acceleration
2: HVM: drop pointless parameters from vIOAPIC internal routines
3: HVM: vMSI simplification

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

[PATCH 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

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

While the REP MOVS acceleration appears to have helped qemu-traditional
based guests, qemu-upstream (or really the respective video BIOSes)
doesn't appear to benefit from that. Instead the acceleration added
here provides a visible performance improvement during very early HVM
guest boot.

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

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -731,6 +731,17 @@ static int hvmemul_rep_movs_discard(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_stos_discard(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    return X86EMUL_OKAY;
+}
+
 static int hvmemul_rep_outs_discard(
     enum x86_segment src_seg,
     unsigned long src_offset,
@@ -982,6 +993,110 @@ static int hvmemul_rep_movs(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_stos(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+    unsigned long addr;
+    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 )
+    {
+        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);
+    }
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    /* Check for MMIO op */
+    (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT, &p2mt);
+
+    switch ( p2mt )
+    {
+        unsigned long bytes;
+        void *buf;
+
+    default:
+        /* Allocate temporary buffer. */
+        for ( ; ; )
+        {
+            bytes = *reps * bytes_per_rep;
+            buf = xmalloc_bytes(bytes);
+            if ( buf || *reps <= 1 )
+                break;
+            *reps >>= 1;
+        }
+
+        if ( !buf )
+            buf = p_data;
+        else
+            switch ( bytes_per_rep )
+            {
+#define CASE(bits, suffix)                                      \
+            case (bits) / 8:                                    \
+                asm ( "rep stos" #suffix                        \
+                      :: "a" (*(const uint##bits##_t *)p_data), \
+                         "D" (buf), "c" (*reps)                 \
+                      : "memory" );                             \
+                break
+            CASE(8, b);
+            CASE(16, w);
+            CASE(32, l);
+            CASE(64, q);
+#undef CASE
+
+            default:
+                xfree(buf);
+                ASSERT(!buf);
+                return X86EMUL_UNHANDLEABLE;
+            }
+
+        /* Adjust address for reverse store. */
+        if ( df )
+            gpa -= bytes - bytes_per_rep;
+
+        rc = hvm_copy_to_guest_phys(gpa, buf, bytes);
+
+        if ( buf != p_data )
+            xfree(buf);
+
+        switch ( rc )
+        {
+        case HVMCOPY_gfn_paged_out:
+        case HVMCOPY_gfn_shared:
+            return X86EMUL_RETRY;
+        case HVMCOPY_okay:
+            return X86EMUL_OKAY;
+        }
+
+        gdprintk(XENLOG_WARNING,
+                 "Failed REP STOS: gpa=%"PRIpaddr" reps=%lu bytes_per_rep=%u\n",
+                 gpa, *reps, bytes_per_rep);
+        /* fall through */
+    case p2m_mmio_direct:
+        return X86EMUL_UNHANDLEABLE;
+
+    case p2m_mmio_dm:
+        return hvmemul_do_mmio(gpa, reps, bytes_per_rep, 0, IOREQ_WRITE, df,
+                               p_data);
+    }
+}
+
 static int hvmemul_read_segment(
     enum x86_segment seg,
     struct segment_register *reg,
@@ -1239,6 +1354,7 @@ static const struct x86_emulate_ops hvm_
     .rep_ins       = hvmemul_rep_ins,
     .rep_outs      = hvmemul_rep_outs,
     .rep_movs      = hvmemul_rep_movs,
+    .rep_stos      = hvmemul_rep_stos,
     .read_segment  = hvmemul_read_segment,
     .write_segment = hvmemul_write_segment,
     .read_io       = hvmemul_read_io,
@@ -1264,6 +1380,7 @@ static const struct x86_emulate_ops hvm_
     .rep_ins       = hvmemul_rep_ins_discard,
     .rep_outs      = hvmemul_rep_outs_discard,
     .rep_movs      = hvmemul_rep_movs_discard,
+    .rep_stos      = hvmemul_rep_stos_discard,
     .read_segment  = hvmemul_read_segment,
     .write_segment = hvmemul_write_segment,
     .read_io       = hvmemul_read_io_discard,
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -470,11 +470,11 @@ static int mmio_move(struct hvm_hw_stdvg
     uint64_t addr = p->addr;
     p2m_type_t p2mt;
     struct domain *d = current->domain;
+    int step = p->df ? -p->size : p->size;
 
     if ( p->data_is_ptr )
     {
         uint64_t data = p->data, tmp;
-        int step = p->df ? -p->size : p->size;
 
         if ( p->dir == IOREQ_READ )
         {
@@ -529,13 +529,18 @@ static int mmio_move(struct hvm_hw_stdvg
             }
         }
     }
+    else if ( p->dir == IOREQ_WRITE )
+    {
+        for ( i = 0; i < p->count; i++ )
+        {
+            stdvga_mem_write(addr, p->data, p->size);
+            addr += step;
+        }
+    }
     else
     {
         ASSERT(p->count == 1);
-        if ( p->dir == IOREQ_READ )
-            p->data = stdvga_mem_read(addr, p->size);
-        else
-            stdvga_mem_write(addr, p->data, p->size);
+        p->data = stdvga_mem_read(addr, p->size);
     }
 
     read_data = p->data;
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2568,15 +2568,25 @@ x86_emulate(
     }
 
     case 0xaa ... 0xab: /* stos */ {
-        /* unsigned long max_reps = */get_rep_prefix();
-        dst.type  = OP_MEM;
+        unsigned long nr_reps = get_rep_prefix();
         dst.bytes = (d & ByteOp) ? 1 : op_bytes;
         dst.mem.seg = x86_seg_es;
         dst.mem.off = truncate_ea(_regs.edi);
-        dst.val   = _regs.eax;
+        if ( (nr_reps == 1) || !ops->rep_stos ||
+             ((rc = ops->rep_stos(&_regs.eax,
+                                  dst.mem.seg, dst.mem.off, dst.bytes,
+                                  &nr_reps, ctxt)) == X86EMUL_UNHANDLEABLE) )
+        {
+            dst.val = _regs.eax;
+            dst.type = OP_MEM;
+            nr_reps = 1;
+        }
+        else if ( rc != X86EMUL_OKAY )
+            goto done;
         register_address_increment(
-            _regs.edi, (_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes);
-        put_rep_prefix(1);
+            _regs.edi,
+            nr_reps * ((_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes));
+        put_rep_prefix(nr_reps);
         break;
     }
 
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -241,6 +241,20 @@ struct x86_emulate_ops
         struct x86_emulate_ctxt *ctxt);
 
     /*
+     * rep_stos: Emulate STOS: <*p_data> -> <seg:offset>.
+     *  @bytes_per_rep: [IN ] Bytes transferred per repetition.
+     *  @reps:  [IN ] Maximum repetitions to be emulated.
+     *          [OUT] Number of repetitions actually emulated.
+     */
+    int (*rep_stos)(
+        void *p_data,
+        enum x86_segment seg,
+        unsigned long offset,
+        unsigned int bytes_per_rep,
+        unsigned long *reps,
+        struct x86_emulate_ctxt *ctxt);
+
+    /*
      * read_segment: Emulate a read of full context of a segment register.
      *  @reg:   [OUT] Contents of segment register (visible and hidden state).
      */



[-- Attachment #2: x86emul-rep-stos.patch --]
[-- Type: text/plain, Size: 8195 bytes --]

x86: also allow REP STOS emulation acceleration

While the REP MOVS acceleration appears to have helped qemu-traditional
based guests, qemu-upstream (or really the respective video BIOSes)
doesn't appear to benefit from that. Instead the acceleration added
here provides a visible performance improvement during very early HVM
guest boot.

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

--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -731,6 +731,17 @@ static int hvmemul_rep_movs_discard(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_stos_discard(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    return X86EMUL_OKAY;
+}
+
 static int hvmemul_rep_outs_discard(
     enum x86_segment src_seg,
     unsigned long src_offset,
@@ -982,6 +993,110 @@ static int hvmemul_rep_movs(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_stos(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+    unsigned long addr;
+    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 )
+    {
+        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);
+    }
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    /* Check for MMIO op */
+    (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT, &p2mt);
+
+    switch ( p2mt )
+    {
+        unsigned long bytes;
+        void *buf;
+
+    default:
+        /* Allocate temporary buffer. */
+        for ( ; ; )
+        {
+            bytes = *reps * bytes_per_rep;
+            buf = xmalloc_bytes(bytes);
+            if ( buf || *reps <= 1 )
+                break;
+            *reps >>= 1;
+        }
+
+        if ( !buf )
+            buf = p_data;
+        else
+            switch ( bytes_per_rep )
+            {
+#define CASE(bits, suffix)                                      \
+            case (bits) / 8:                                    \
+                asm ( "rep stos" #suffix                        \
+                      :: "a" (*(const uint##bits##_t *)p_data), \
+                         "D" (buf), "c" (*reps)                 \
+                      : "memory" );                             \
+                break
+            CASE(8, b);
+            CASE(16, w);
+            CASE(32, l);
+            CASE(64, q);
+#undef CASE
+
+            default:
+                xfree(buf);
+                ASSERT(!buf);
+                return X86EMUL_UNHANDLEABLE;
+            }
+
+        /* Adjust address for reverse store. */
+        if ( df )
+            gpa -= bytes - bytes_per_rep;
+
+        rc = hvm_copy_to_guest_phys(gpa, buf, bytes);
+
+        if ( buf != p_data )
+            xfree(buf);
+
+        switch ( rc )
+        {
+        case HVMCOPY_gfn_paged_out:
+        case HVMCOPY_gfn_shared:
+            return X86EMUL_RETRY;
+        case HVMCOPY_okay:
+            return X86EMUL_OKAY;
+        }
+
+        gdprintk(XENLOG_WARNING,
+                 "Failed REP STOS: gpa=%"PRIpaddr" reps=%lu bytes_per_rep=%u\n",
+                 gpa, *reps, bytes_per_rep);
+        /* fall through */
+    case p2m_mmio_direct:
+        return X86EMUL_UNHANDLEABLE;
+
+    case p2m_mmio_dm:
+        return hvmemul_do_mmio(gpa, reps, bytes_per_rep, 0, IOREQ_WRITE, df,
+                               p_data);
+    }
+}
+
 static int hvmemul_read_segment(
     enum x86_segment seg,
     struct segment_register *reg,
@@ -1239,6 +1354,7 @@ static const struct x86_emulate_ops hvm_
     .rep_ins       = hvmemul_rep_ins,
     .rep_outs      = hvmemul_rep_outs,
     .rep_movs      = hvmemul_rep_movs,
+    .rep_stos      = hvmemul_rep_stos,
     .read_segment  = hvmemul_read_segment,
     .write_segment = hvmemul_write_segment,
     .read_io       = hvmemul_read_io,
@@ -1264,6 +1380,7 @@ static const struct x86_emulate_ops hvm_
     .rep_ins       = hvmemul_rep_ins_discard,
     .rep_outs      = hvmemul_rep_outs_discard,
     .rep_movs      = hvmemul_rep_movs_discard,
+    .rep_stos      = hvmemul_rep_stos_discard,
     .read_segment  = hvmemul_read_segment,
     .write_segment = hvmemul_write_segment,
     .read_io       = hvmemul_read_io_discard,
--- a/xen/arch/x86/hvm/stdvga.c
+++ b/xen/arch/x86/hvm/stdvga.c
@@ -470,11 +470,11 @@ static int mmio_move(struct hvm_hw_stdvg
     uint64_t addr = p->addr;
     p2m_type_t p2mt;
     struct domain *d = current->domain;
+    int step = p->df ? -p->size : p->size;
 
     if ( p->data_is_ptr )
     {
         uint64_t data = p->data, tmp;
-        int step = p->df ? -p->size : p->size;
 
         if ( p->dir == IOREQ_READ )
         {
@@ -529,13 +529,18 @@ static int mmio_move(struct hvm_hw_stdvg
             }
         }
     }
+    else if ( p->dir == IOREQ_WRITE )
+    {
+        for ( i = 0; i < p->count; i++ )
+        {
+            stdvga_mem_write(addr, p->data, p->size);
+            addr += step;
+        }
+    }
     else
     {
         ASSERT(p->count == 1);
-        if ( p->dir == IOREQ_READ )
-            p->data = stdvga_mem_read(addr, p->size);
-        else
-            stdvga_mem_write(addr, p->data, p->size);
+        p->data = stdvga_mem_read(addr, p->size);
     }
 
     read_data = p->data;
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -2568,15 +2568,25 @@ x86_emulate(
     }
 
     case 0xaa ... 0xab: /* stos */ {
-        /* unsigned long max_reps = */get_rep_prefix();
-        dst.type  = OP_MEM;
+        unsigned long nr_reps = get_rep_prefix();
         dst.bytes = (d & ByteOp) ? 1 : op_bytes;
         dst.mem.seg = x86_seg_es;
         dst.mem.off = truncate_ea(_regs.edi);
-        dst.val   = _regs.eax;
+        if ( (nr_reps == 1) || !ops->rep_stos ||
+             ((rc = ops->rep_stos(&_regs.eax,
+                                  dst.mem.seg, dst.mem.off, dst.bytes,
+                                  &nr_reps, ctxt)) == X86EMUL_UNHANDLEABLE) )
+        {
+            dst.val = _regs.eax;
+            dst.type = OP_MEM;
+            nr_reps = 1;
+        }
+        else if ( rc != X86EMUL_OKAY )
+            goto done;
         register_address_increment(
-            _regs.edi, (_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes);
-        put_rep_prefix(1);
+            _regs.edi,
+            nr_reps * ((_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes));
+        put_rep_prefix(nr_reps);
         break;
     }
 
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -241,6 +241,20 @@ struct x86_emulate_ops
         struct x86_emulate_ctxt *ctxt);
 
     /*
+     * rep_stos: Emulate STOS: <*p_data> -> <seg:offset>.
+     *  @bytes_per_rep: [IN ] Bytes transferred per repetition.
+     *  @reps:  [IN ] Maximum repetitions to be emulated.
+     *          [OUT] Number of repetitions actually emulated.
+     */
+    int (*rep_stos)(
+        void *p_data,
+        enum x86_segment seg,
+        unsigned long offset,
+        unsigned int bytes_per_rep,
+        unsigned long *reps,
+        struct x86_emulate_ctxt *ctxt);
+
+    /*
      * read_segment: Emulate a read of full context of a segment register.
      *  @reg:   [OUT] Contents of segment register (visible and hidden state).
      */

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

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

[PATCH 2/3] x86/HVM: drop pointless parameters from vIOAPIC internal routines

[Jan Beulich]

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

Also simplify a few other operations (without funtional change).

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

--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -46,11 +46,9 @@
 
 static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq);
 
-static unsigned long vioapic_read_indirect(struct hvm_hw_vioapic *vioapic,
-                                           unsigned long addr,
-                                           unsigned long length)
+static uint32_t vioapic_read_indirect(const struct hvm_hw_vioapic *vioapic)
 {
-    unsigned long result = 0;
+    uint32_t result = 0;
 
     switch ( vioapic->ioregsel )
     {
@@ -77,9 +75,8 @@ static unsigned long vioapic_read_indire
         }
 
         redir_content = vioapic->redirtbl[redir_index].bits;
-        result = (vioapic->ioregsel & 0x1)?
-            (redir_content >> 32) & 0xffffffff :
-            redir_content & 0xffffffff;
+        result = (vioapic->ioregsel & 1) ? (redir_content >> 32)
+                                         : redir_content;
         break;
     }
     }
@@ -91,21 +88,19 @@ static int vioapic_read(
     struct vcpu *v, unsigned long addr,
     unsigned long length, unsigned long *pval)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
+    const struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
     uint32_t result;
 
     HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "addr %lx", addr);
 
-    addr &= 0xff;
-
-    switch ( addr )
+    switch ( addr & 0xff )
     {
     case VIOAPIC_REG_SELECT:
         result = vioapic->ioregsel;
         break;
 
     case VIOAPIC_REG_WINDOW:
-        result = vioapic_read_indirect(vioapic, addr, length);
+        result = vioapic_read_indirect(vioapic);
         break;
 
     default:
@@ -169,7 +164,7 @@ static void vioapic_write_redirent(
 }
 
 static void vioapic_write_indirect(
-    struct hvm_hw_vioapic *vioapic, unsigned long length, unsigned long val)
+    struct hvm_hw_vioapic *vioapic, uint32_t val)
 {
     switch ( vioapic->ioregsel )
     {
@@ -188,8 +183,8 @@ static void vioapic_write_indirect(
     {
         uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
 
-        HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "change redir index %x val %lx",
-                    redir_index, val);
+        HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "rte[%02x].%s = %08x",
+                    redir_index, vioapic->ioregsel & 1 ? "hi" : "lo", val);
 
         if ( redir_index >= VIOAPIC_NUM_PINS )
         {
@@ -211,16 +206,14 @@ static int vioapic_write(
 {
     struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
 
-    addr &= 0xff;
-
-    switch ( addr )
+    switch ( addr & 0xff )
     {
     case VIOAPIC_REG_SELECT:
         vioapic->ioregsel = val;
         break;
 
     case VIOAPIC_REG_WINDOW:
-        vioapic_write_indirect(vioapic, length, val);
+        vioapic_write_indirect(vioapic, val);
         break;
 
 #if VIOAPIC_VERSION_ID >= 0x20




[-- Attachment #2: x86-vIOAPIC-drop-params.patch --]
[-- Type: text/plain, Size: 3113 bytes --]

x86/HVM: drop pointless parameters from vIOAPIC internal routines

Also simplify a few other operations (without funtional change).

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

--- a/xen/arch/x86/hvm/vioapic.c
+++ b/xen/arch/x86/hvm/vioapic.c
@@ -46,11 +46,9 @@
 
 static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq);
 
-static unsigned long vioapic_read_indirect(struct hvm_hw_vioapic *vioapic,
-                                           unsigned long addr,
-                                           unsigned long length)
+static uint32_t vioapic_read_indirect(const struct hvm_hw_vioapic *vioapic)
 {
-    unsigned long result = 0;
+    uint32_t result = 0;
 
     switch ( vioapic->ioregsel )
     {
@@ -77,9 +75,8 @@ static unsigned long vioapic_read_indire
         }
 
         redir_content = vioapic->redirtbl[redir_index].bits;
-        result = (vioapic->ioregsel & 0x1)?
-            (redir_content >> 32) & 0xffffffff :
-            redir_content & 0xffffffff;
+        result = (vioapic->ioregsel & 1) ? (redir_content >> 32)
+                                         : redir_content;
         break;
     }
     }
@@ -91,21 +88,19 @@ static int vioapic_read(
     struct vcpu *v, unsigned long addr,
     unsigned long length, unsigned long *pval)
 {
-    struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
+    const struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
     uint32_t result;
 
     HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "addr %lx", addr);
 
-    addr &= 0xff;
-
-    switch ( addr )
+    switch ( addr & 0xff )
     {
     case VIOAPIC_REG_SELECT:
         result = vioapic->ioregsel;
         break;
 
     case VIOAPIC_REG_WINDOW:
-        result = vioapic_read_indirect(vioapic, addr, length);
+        result = vioapic_read_indirect(vioapic);
         break;
 
     default:
@@ -169,7 +164,7 @@ static void vioapic_write_redirent(
 }
 
 static void vioapic_write_indirect(
-    struct hvm_hw_vioapic *vioapic, unsigned long length, unsigned long val)
+    struct hvm_hw_vioapic *vioapic, uint32_t val)
 {
     switch ( vioapic->ioregsel )
     {
@@ -188,8 +183,8 @@ static void vioapic_write_indirect(
     {
         uint32_t redir_index = (vioapic->ioregsel - 0x10) >> 1;
 
-        HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "change redir index %x val %lx",
-                    redir_index, val);
+        HVM_DBG_LOG(DBG_LEVEL_IOAPIC, "rte[%02x].%s = %08x",
+                    redir_index, vioapic->ioregsel & 1 ? "hi" : "lo", val);
 
         if ( redir_index >= VIOAPIC_NUM_PINS )
         {
@@ -211,16 +206,14 @@ static int vioapic_write(
 {
     struct hvm_hw_vioapic *vioapic = domain_vioapic(v->domain);
 
-    addr &= 0xff;
-
-    switch ( addr )
+    switch ( addr & 0xff )
     {
     case VIOAPIC_REG_SELECT:
         vioapic->ioregsel = val;
         break;
 
     case VIOAPIC_REG_WINDOW:
-        vioapic_write_indirect(vioapic, length, val);
+        vioapic_write_indirect(vioapic, val);
         break;
 
 #if VIOAPIC_VERSION_ID >= 0x20

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

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

[PATCH 3/3] x86/HVM: vMSI simplification

[Jan Beulich]

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

- struct msixtbl_entry's table_len field can be unsigned int, and by
  moving it down a little the structure size can be reduced slightly
- a disjoint xmalloc()/memset() pair can be converted to xzalloc()
- a pointless local variable can be dropped

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

--- unstable.orig/xen/arch/x86/hvm/vmsi.c	2014-11-17 11:40:49.000000000 +0100
+++ unstable/xen/arch/x86/hvm/vmsi.c	2014-11-17 11:43:18.000000000 +0100
@@ -153,9 +153,9 @@ struct msixtbl_entry
     /* TODO: resolve the potential race by destruction of pdev */
     struct pci_dev *pdev;
     unsigned long gtable;       /* gpa of msix table */
-    unsigned long table_len;
     unsigned long table_flags[BITS_TO_LONGS(MAX_MSIX_TABLE_ENTRIES)];
 #define MAX_MSIX_ACC_ENTRIES 3
+    unsigned int table_len;
     struct { 
         uint32_t msi_ad[3];	/* Shadow of address low, high and data */
     } gentries[MAX_MSIX_ACC_ENTRIES];
@@ -380,16 +380,11 @@ static void add_msixtbl_entry(struct dom
                               uint64_t gtable,
                               struct msixtbl_entry *entry)
 {
-    u32 len;
-
-    memset(entry, 0, sizeof(struct msixtbl_entry));
-        
     INIT_LIST_HEAD(&entry->list);
     INIT_RCU_HEAD(&entry->rcu);
     atomic_set(&entry->refcnt, 0);
 
-    len = pci_msix_get_table_len(pdev);
-    entry->table_len = len;
+    entry->table_len = pci_msix_get_table_len(pdev);
     entry->pdev = pdev;
     entry->gtable = (unsigned long) gtable;
 
@@ -426,7 +421,7 @@ int msixtbl_pt_register(struct domain *d
      * xmalloc() with irq_disabled causes the failure of check_lock() 
      * for xenpool->lock. So we allocate an entry beforehand.
      */
-    new_entry = xmalloc(struct msixtbl_entry);
+    new_entry = xzalloc(struct msixtbl_entry);
     if ( !new_entry )
         return -ENOMEM;
 




[-- Attachment #2: x86-vMSI-table-len.patch --]
[-- Type: text/plain, Size: 1914 bytes --]

x86/HVM: vMSI simplification

- struct msixtbl_entry's table_len field can be unsigned int, and by
  moving it down a little the structure size can be reduced slightly
- a disjoint xmalloc()/memset() pair can be converted to xzalloc()
- a pointless local variable can be dropped

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

--- unstable.orig/xen/arch/x86/hvm/vmsi.c	2014-11-17 11:40:49.000000000 +0100
+++ unstable/xen/arch/x86/hvm/vmsi.c	2014-11-17 11:43:18.000000000 +0100
@@ -153,9 +153,9 @@ struct msixtbl_entry
     /* TODO: resolve the potential race by destruction of pdev */
     struct pci_dev *pdev;
     unsigned long gtable;       /* gpa of msix table */
-    unsigned long table_len;
     unsigned long table_flags[BITS_TO_LONGS(MAX_MSIX_TABLE_ENTRIES)];
 #define MAX_MSIX_ACC_ENTRIES 3
+    unsigned int table_len;
     struct { 
         uint32_t msi_ad[3];	/* Shadow of address low, high and data */
     } gentries[MAX_MSIX_ACC_ENTRIES];
@@ -380,16 +380,11 @@ static void add_msixtbl_entry(struct dom
                               uint64_t gtable,
                               struct msixtbl_entry *entry)
 {
-    u32 len;
-
-    memset(entry, 0, sizeof(struct msixtbl_entry));
-        
     INIT_LIST_HEAD(&entry->list);
     INIT_RCU_HEAD(&entry->rcu);
     atomic_set(&entry->refcnt, 0);
 
-    len = pci_msix_get_table_len(pdev);
-    entry->table_len = len;
+    entry->table_len = pci_msix_get_table_len(pdev);
     entry->pdev = pdev;
     entry->gtable = (unsigned long) gtable;
 
@@ -426,7 +421,7 @@ int msixtbl_pt_register(struct domain *d
      * xmalloc() with irq_disabled causes the failure of check_lock() 
      * for xenpool->lock. So we allocate an entry beforehand.
      */
-    new_entry = xmalloc(struct msixtbl_entry);
+    new_entry = xzalloc(struct msixtbl_entry);
     if ( !new_entry )
         return -ENOMEM;
 

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

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

Re: [PATCH 2/3] x86/HVM: drop pointless parameters from vIOAPIC internal routines

[Tim Deegan]

At 15:51 +0000 on 08 Jan (1420728683), Jan Beulich wrote:
> Also simplify a few other operations (without funtional change).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH 3/3] x86/HVM: vMSI simplification

[Tim Deegan]

At 15:52 +0000 on 08 Jan (1420728734), Jan Beulich wrote:
> - struct msixtbl_entry's table_len field can be unsigned int, and by
>   moving it down a little the structure size can be reduced slightly
> - a disjoint xmalloc()/memset() pair can be converted to xzalloc()
> - a pointless local variable can be dropped
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Signed-off-by: Tim Deegan <tim@xen.org>

Re: [PATCH 1/3] x86: also allow REP STOS emulation acceleration

[Tim Deegan]

At 15:50 +0000 on 08 Jan (1420728649), Jan Beulich wrote:
> While the REP MOVS acceleration appears to have helped qemu-traditional
> based guests, qemu-upstream (or really the respective video BIOSes)
> doesn't appear to benefit from that. Instead the acceleration added
> here provides a visible performance improvement during very early HVM
> guest boot.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

If I read this right, it's allocating and memset()ing a buffer and
then copying that buffer to the guest?

Would it be better to map the guest frame and write directly?  Edge
cases where the STOS crosses a frame boundary could happen the old way.

Cheers,

Tim.

Re: [PATCH 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

>>> On 08.01.15 at 17:16, <tim@xen.org> wrote:
> At 15:50 +0000 on 08 Jan (1420728649), Jan Beulich wrote:
>> While the REP MOVS acceleration appears to have helped qemu-traditional
>> based guests, qemu-upstream (or really the respective video BIOSes)
>> doesn't appear to benefit from that. Instead the acceleration added
>> here provides a visible performance improvement during very early HVM
>> guest boot.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> If I read this right, it's allocating and memset()ing a buffer and
> then copying that buffer to the guest?

In the non-MMIO case yes.

> Would it be better to map the guest frame and write directly?  Edge
> cases where the STOS crosses a frame boundary could happen the old way.

This matches the REP MOVS handling, which also allocates a
temporary buffer. I.e. if you wanted this for REP STOS, we should
first make it so for REP MOVS.

Jan

Re: [PATCH 3/3] x86/HVM: vMSI simplification

[Jan Beulich]

>>> On 08.01.15 at 17:14, <tim@xen.org> wrote:
> At 15:52 +0000 on 08 Jan (1420728734), Jan Beulich wrote:
>> - struct msixtbl_entry's table_len field can be unsigned int, and by
>>   moving it down a little the structure size can be reduced slightly
>> - a disjoint xmalloc()/memset() pair can be converted to xzalloc()
>> - a pointless local variable can be dropped
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Signed-off-by: Tim Deegan <tim@xen.org>

DYM Reviewed-by?

Re: [PATCH 3/3] x86/HVM: vMSI simplification

[Tim Deegan]

At 16:30 +0000 on 08 Jan (1420731016), Jan Beulich wrote:
> >>> On 08.01.15 at 17:14, <tim@xen.org> wrote:
> > At 15:52 +0000 on 08 Jan (1420728734), Jan Beulich wrote:
> >> - struct msixtbl_entry's table_len field can be unsigned int, and by
> >>   moving it down a little the structure size can be reduced slightly
> >> - a disjoint xmalloc()/memset() pair can be converted to xzalloc()
> >> - a pointless local variable can be dropped
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Signed-off-by: Tim Deegan <tim@xen.org>
> 
> DYM Reviewed-by?

Yes, I do, sorry.  Although if I'm making simple errors like that
today maybe my review is not worth as much as it might be. :)

Tim.

Re: [PATCH 1/3] x86: also allow REP STOS emulation acceleration

[Tim Deegan]

At 16:29 +0000 on 08 Jan (1420730974), Jan Beulich wrote:
> >>> On 08.01.15 at 17:16, <tim@xen.org> wrote:
> > At 15:50 +0000 on 08 Jan (1420728649), Jan Beulich wrote:
> >> While the REP MOVS acceleration appears to have helped qemu-traditional
> >> based guests, qemu-upstream (or really the respective video BIOSes)
> >> doesn't appear to benefit from that. Instead the acceleration added
> >> here provides a visible performance improvement during very early HVM
> >> guest boot.
> >> 
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > If I read this right, it's allocating and memset()ing a buffer and
> > then copying that buffer to the guest?
> 
> In the non-MMIO case yes.
> 
> > Would it be better to map the guest frame and write directly?  Edge
> > cases where the STOS crosses a frame boundary could happen the old way.
> 
> This matches the REP MOVS handling, which also allocates a
> temporary buffer. I.e. if you wanted this for REP STOS, we should
> first make it so for REP MOVS.

Well, REP MOVS is trickier as it would have to handle page crossings
in both source and destination.

Tim.

Re: [PATCH 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

>>> On 08.01.15 at 17:56, <tim@xen.org> wrote:
> At 16:29 +0000 on 08 Jan (1420730974), Jan Beulich wrote:
>> >>> On 08.01.15 at 17:16, <tim@xen.org> wrote:
>> > At 15:50 +0000 on 08 Jan (1420728649), Jan Beulich wrote:
>> >> While the REP MOVS acceleration appears to have helped qemu-traditional
>> >> based guests, qemu-upstream (or really the respective video BIOSes)
>> >> doesn't appear to benefit from that. Instead the acceleration added
>> >> here provides a visible performance improvement during very early HVM
>> >> guest boot.
>> >> 
>> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> > 
>> > If I read this right, it's allocating and memset()ing a buffer and
>> > then copying that buffer to the guest?
>> 
>> In the non-MMIO case yes.
>> 
>> > Would it be better to map the guest frame and write directly?  Edge
>> > cases where the STOS crosses a frame boundary could happen the old way.
>> 
>> This matches the REP MOVS handling, which also allocates a
>> temporary buffer. I.e. if you wanted this for REP STOS, we should
>> first make it so for REP MOVS.
> 
> Well, REP MOVS is trickier as it would have to handle page crossings
> in both source and destination.

But I don't think would could blindly map for all sorts of p2mt we
get back - we'd have to special case RAM, and deal with the
other cases the current way anyway (even if for nothing else
than getting a correct error indicator).

Jan

Re: [PATCH 1/3] x86: also allow REP STOS emulation acceleration

[Tim Deegan]

At 17:05 +0000 on 08 Jan (1420733142), Jan Beulich wrote:
> >>> On 08.01.15 at 17:56, <tim@xen.org> wrote:
> > At 16:29 +0000 on 08 Jan (1420730974), Jan Beulich wrote:
> >> >>> On 08.01.15 at 17:16, <tim@xen.org> wrote:
> >> > At 15:50 +0000 on 08 Jan (1420728649), Jan Beulich wrote:
> >> >> While the REP MOVS acceleration appears to have helped qemu-traditional
> >> >> based guests, qemu-upstream (or really the respective video BIOSes)
> >> >> doesn't appear to benefit from that. Instead the acceleration added
> >> >> here provides a visible performance improvement during very early HVM
> >> >> guest boot.
> >> >> 
> >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> > 
> >> > If I read this right, it's allocating and memset()ing a buffer and
> >> > then copying that buffer to the guest?
> >> 
> >> In the non-MMIO case yes.
> >> 
> >> > Would it be better to map the guest frame and write directly?  Edge
> >> > cases where the STOS crosses a frame boundary could happen the old way.
> >> 
> >> This matches the REP MOVS handling, which also allocates a
> >> temporary buffer. I.e. if you wanted this for REP STOS, we should
> >> first make it so for REP MOVS.
> > 
> > Well, REP MOVS is trickier as it would have to handle page crossings
> > in both source and destination.
> 
> But I don't think would could blindly map for all sorts of p2mt we
> get back - we'd have to special case RAM, and deal with the
> other cases the current way anyway (even if for nothing else
> than getting a correct error indicator).

Fair enough - we might as well just have all that once in the hvm_copy
code.

Reviewed-by: Tim Deegan <tim@xen.org>

Re: [PATCH 1/3] x86: also allow REP STOS emulation acceleration

[Andrew Cooper]

On 08/01/15 15:50, Jan Beulich wrote:
> +        if ( !buf )
> +            buf = p_data;
> +        else
> +            switch ( bytes_per_rep )
> +            {
> +#define CASE(bits, suffix)                                      \
> +            case (bits) / 8:                                    \
> +                asm ( "rep stos" #suffix                        \
> +                      :: "a" (*(const uint##bits##_t *)p_data), \
> +                         "D" (buf), "c" (*reps)                 \
> +                      : "memory" );                             \

This looks as if it needs output clobbers for D and c

~Andrew

Re: [PATCH 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

>>> On 08.01.15 at 20:29, <andrew.cooper3@citrix.com> wrote:
> On 08/01/15 15:50, Jan Beulich wrote:
>> +        if ( !buf )
>> +            buf = p_data;
>> +        else
>> +            switch ( bytes_per_rep )
>> +            {
>> +#define CASE(bits, suffix)                                      \
>> +            case (bits) / 8:                                    \
>> +                asm ( "rep stos" #suffix                        \
>> +                      :: "a" (*(const uint##bits##_t *)p_data), \
>> +                         "D" (buf), "c" (*reps)                 \
>> +                      : "memory" );                             \
> 
> This looks as if it needs output clobbers for D and c

Well spotted, will fix.

Jan

Re: [PATCH 3/3] x86/HVM: vMSI simplification

[Andrew Cooper]

On 08/01/15 15:52, Jan Beulich wrote:
> - struct msixtbl_entry's table_len field can be unsigned int, and by
>   moving it down a little the structure size can be reduced slightly
> - a disjoint xmalloc()/memset() pair can be converted to xzalloc()
> - a pointless local variable can be dropped
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

[PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

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

While the REP MOVS acceleration appears to have helped qemu-traditional
based guests, qemu-upstream (or really the respective video BIOSes)
doesn't appear to benefit from that. Instead the acceleration added
here provides a visible performance improvement during very early HVM
guest boot.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: fix asm() constraints in hvmemul_rep_stos(), as pointed out by
    Andrew. Add output operand telling the compiler that "buf" is being
    written.

--- unstable.orig/xen/arch/x86/hvm/emulate.c	2015-01-09 11:19:01.000000000 +0100
+++ unstable/xen/arch/x86/hvm/emulate.c	2015-01-09 11:19:36.000000000 +0100
@@ -731,6 +731,17 @@ static int hvmemul_rep_movs_discard(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_stos_discard(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    return X86EMUL_OKAY;
+}
+
 static int hvmemul_rep_outs_discard(
     enum x86_segment src_seg,
     unsigned long src_offset,
@@ -982,6 +993,114 @@ static int hvmemul_rep_movs(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_stos(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+    unsigned long addr;
+    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 )
+    {
+        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);
+    }
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    /* Check for MMIO op */
+    (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT, &p2mt);
+
+    switch ( p2mt )
+    {
+        unsigned long bytes;
+        void *buf;
+
+    default:
+        /* Allocate temporary buffer. */
+        for ( ; ; )
+        {
+            bytes = *reps * bytes_per_rep;
+            buf = xmalloc_bytes(bytes);
+            if ( buf || *reps <= 1 )
+                break;
+            *reps >>= 1;
+        }
+
+        if ( !buf )
+            buf = p_data;
+        else
+            switch ( bytes_per_rep )
+            {
+                unsigned long dummy;
+
+#define CASE(bits, suffix)                                     \
+            case (bits) / 8:                                   \
+                asm ( "rep stos" #suffix                       \
+                      : "=m" (*(char (*)[bytes])buf),          \
+                        "=D" (dummy), "=c" (dummy)             \
+                      : "a" (*(const uint##bits##_t *)p_data), \
+                         "1" (buf), "2" (*reps)                \
+                      : "memory" );                            \
+                break
+            CASE(8, b);
+            CASE(16, w);
+            CASE(32, l);
+            CASE(64, q);
+#undef CASE
+
+            default:
+                xfree(buf);
+                ASSERT(!buf);
+                return X86EMUL_UNHANDLEABLE;
+            }
+
+        /* Adjust address for reverse store. */
+        if ( df )
+            gpa -= bytes - bytes_per_rep;
+
+        rc = hvm_copy_to_guest_phys(gpa, buf, bytes);
+
+        if ( buf != p_data )
+            xfree(buf);
+
+        switch ( rc )
+        {
+        case HVMCOPY_gfn_paged_out:
+        case HVMCOPY_gfn_shared:
+            return X86EMUL_RETRY;
+        case HVMCOPY_okay:
+            return X86EMUL_OKAY;
+        }
+
+        gdprintk(XENLOG_WARNING,
+                 "Failed REP STOS: gpa=%"PRIpaddr" reps=%lu bytes_per_rep=%u\n",
+                 gpa, *reps, bytes_per_rep);
+        /* fall through */
+    case p2m_mmio_direct:
+        return X86EMUL_UNHANDLEABLE;
+
+    case p2m_mmio_dm:
+        return hvmemul_do_mmio(gpa, reps, bytes_per_rep, 0, IOREQ_WRITE, df,
+                               p_data);
+    }
+}
+
 static int hvmemul_read_segment(
     enum x86_segment seg,
     struct segment_register *reg,
@@ -1239,6 +1358,7 @@ static const struct x86_emulate_ops hvm_
     .rep_ins       = hvmemul_rep_ins,
     .rep_outs      = hvmemul_rep_outs,
     .rep_movs      = hvmemul_rep_movs,
+    .rep_stos      = hvmemul_rep_stos,
     .read_segment  = hvmemul_read_segment,
     .write_segment = hvmemul_write_segment,
     .read_io       = hvmemul_read_io,
@@ -1264,6 +1384,7 @@ static const struct x86_emulate_ops hvm_
     .rep_ins       = hvmemul_rep_ins_discard,
     .rep_outs      = hvmemul_rep_outs_discard,
     .rep_movs      = hvmemul_rep_movs_discard,
+    .rep_stos      = hvmemul_rep_stos_discard,
     .read_segment  = hvmemul_read_segment,
     .write_segment = hvmemul_write_segment,
     .read_io       = hvmemul_read_io_discard,
--- unstable.orig/xen/arch/x86/hvm/stdvga.c	2015-01-09 11:19:01.000000000 +0100
+++ unstable/xen/arch/x86/hvm/stdvga.c	2014-11-04 17:42:12.000000000 +0100
@@ -470,11 +470,11 @@ static int mmio_move(struct hvm_hw_stdvg
     uint64_t addr = p->addr;
     p2m_type_t p2mt;
     struct domain *d = current->domain;
+    int step = p->df ? -p->size : p->size;
 
     if ( p->data_is_ptr )
     {
         uint64_t data = p->data, tmp;
-        int step = p->df ? -p->size : p->size;
 
         if ( p->dir == IOREQ_READ )
         {
@@ -529,13 +529,18 @@ static int mmio_move(struct hvm_hw_stdvg
             }
         }
     }
+    else if ( p->dir == IOREQ_WRITE )
+    {
+        for ( i = 0; i < p->count; i++ )
+        {
+            stdvga_mem_write(addr, p->data, p->size);
+            addr += step;
+        }
+    }
     else
     {
         ASSERT(p->count == 1);
-        if ( p->dir == IOREQ_READ )
-            p->data = stdvga_mem_read(addr, p->size);
-        else
-            stdvga_mem_write(addr, p->data, p->size);
+        p->data = stdvga_mem_read(addr, p->size);
     }
 
     read_data = p->data;
--- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.c	2015-01-09 11:19:01.000000000 +0100
+++ unstable/xen/arch/x86/x86_emulate/x86_emulate.c	2015-01-09 11:08:19.000000000 +0100
@@ -2568,15 +2568,25 @@ x86_emulate(
     }
 
     case 0xaa ... 0xab: /* stos */ {
-        /* unsigned long max_reps = */get_rep_prefix();
-        dst.type  = OP_MEM;
+        unsigned long nr_reps = get_rep_prefix();
         dst.bytes = (d & ByteOp) ? 1 : op_bytes;
         dst.mem.seg = x86_seg_es;
         dst.mem.off = truncate_ea(_regs.edi);
-        dst.val   = _regs.eax;
+        if ( (nr_reps == 1) || !ops->rep_stos ||
+             ((rc = ops->rep_stos(&_regs.eax,
+                                  dst.mem.seg, dst.mem.off, dst.bytes,
+                                  &nr_reps, ctxt)) == X86EMUL_UNHANDLEABLE) )
+        {
+            dst.val = _regs.eax;
+            dst.type = OP_MEM;
+            nr_reps = 1;
+        }
+        else if ( rc != X86EMUL_OKAY )
+            goto done;
         register_address_increment(
-            _regs.edi, (_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes);
-        put_rep_prefix(1);
+            _regs.edi,
+            nr_reps * ((_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes));
+        put_rep_prefix(nr_reps);
         break;
     }
 
--- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.h	2015-01-09 11:19:01.000000000 +0100
+++ unstable/xen/arch/x86/x86_emulate/x86_emulate.h	2014-11-04 17:06:49.000000000 +0100
@@ -241,6 +241,20 @@ struct x86_emulate_ops
         struct x86_emulate_ctxt *ctxt);
 
     /*
+     * rep_stos: Emulate STOS: <*p_data> -> <seg:offset>.
+     *  @bytes_per_rep: [IN ] Bytes transferred per repetition.
+     *  @reps:  [IN ] Maximum repetitions to be emulated.
+     *          [OUT] Number of repetitions actually emulated.
+     */
+    int (*rep_stos)(
+        void *p_data,
+        enum x86_segment seg,
+        unsigned long offset,
+        unsigned int bytes_per_rep,
+        unsigned long *reps,
+        struct x86_emulate_ctxt *ctxt);
+
+    /*
      * read_segment: Emulate a read of full context of a segment register.
      *  @reg:   [OUT] Contents of segment register (visible and hidden state).
      */



[-- Attachment #2: x86emul-rep-stos.patch --]
[-- Type: text/plain, Size: 8889 bytes --]

x86: also allow REP STOS emulation acceleration

While the REP MOVS acceleration appears to have helped qemu-traditional
based guests, qemu-upstream (or really the respective video BIOSes)
doesn't appear to benefit from that. Instead the acceleration added
here provides a visible performance improvement during very early HVM
guest boot.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: fix asm() constraints in hvmemul_rep_stos(), as pointed out by
    Andrew. Add output operand telling the compiler that "buf" is being
    written.

--- unstable.orig/xen/arch/x86/hvm/emulate.c	2015-01-09 11:19:01.000000000 +0100
+++ unstable/xen/arch/x86/hvm/emulate.c	2015-01-09 11:19:36.000000000 +0100
@@ -731,6 +731,17 @@ static int hvmemul_rep_movs_discard(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_stos_discard(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    return X86EMUL_OKAY;
+}
+
 static int hvmemul_rep_outs_discard(
     enum x86_segment src_seg,
     unsigned long src_offset,
@@ -982,6 +993,114 @@ static int hvmemul_rep_movs(
     return X86EMUL_OKAY;
 }
 
+static int hvmemul_rep_stos(
+    void *p_data,
+    enum x86_segment seg,
+    unsigned long offset,
+    unsigned int bytes_per_rep,
+    unsigned long *reps,
+    struct x86_emulate_ctxt *ctxt)
+{
+    struct hvm_emulate_ctxt *hvmemul_ctxt =
+        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
+    unsigned long addr;
+    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 )
+    {
+        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);
+    }
+    if ( rc != X86EMUL_OKAY )
+        return rc;
+
+    /* Check for MMIO op */
+    (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT, &p2mt);
+
+    switch ( p2mt )
+    {
+        unsigned long bytes;
+        void *buf;
+
+    default:
+        /* Allocate temporary buffer. */
+        for ( ; ; )
+        {
+            bytes = *reps * bytes_per_rep;
+            buf = xmalloc_bytes(bytes);
+            if ( buf || *reps <= 1 )
+                break;
+            *reps >>= 1;
+        }
+
+        if ( !buf )
+            buf = p_data;
+        else
+            switch ( bytes_per_rep )
+            {
+                unsigned long dummy;
+
+#define CASE(bits, suffix)                                     \
+            case (bits) / 8:                                   \
+                asm ( "rep stos" #suffix                       \
+                      : "=m" (*(char (*)[bytes])buf),          \
+                        "=D" (dummy), "=c" (dummy)             \
+                      : "a" (*(const uint##bits##_t *)p_data), \
+                         "1" (buf), "2" (*reps)                \
+                      : "memory" );                            \
+                break
+            CASE(8, b);
+            CASE(16, w);
+            CASE(32, l);
+            CASE(64, q);
+#undef CASE
+
+            default:
+                xfree(buf);
+                ASSERT(!buf);
+                return X86EMUL_UNHANDLEABLE;
+            }
+
+        /* Adjust address for reverse store. */
+        if ( df )
+            gpa -= bytes - bytes_per_rep;
+
+        rc = hvm_copy_to_guest_phys(gpa, buf, bytes);
+
+        if ( buf != p_data )
+            xfree(buf);
+
+        switch ( rc )
+        {
+        case HVMCOPY_gfn_paged_out:
+        case HVMCOPY_gfn_shared:
+            return X86EMUL_RETRY;
+        case HVMCOPY_okay:
+            return X86EMUL_OKAY;
+        }
+
+        gdprintk(XENLOG_WARNING,
+                 "Failed REP STOS: gpa=%"PRIpaddr" reps=%lu bytes_per_rep=%u\n",
+                 gpa, *reps, bytes_per_rep);
+        /* fall through */
+    case p2m_mmio_direct:
+        return X86EMUL_UNHANDLEABLE;
+
+    case p2m_mmio_dm:
+        return hvmemul_do_mmio(gpa, reps, bytes_per_rep, 0, IOREQ_WRITE, df,
+                               p_data);
+    }
+}
+
 static int hvmemul_read_segment(
     enum x86_segment seg,
     struct segment_register *reg,
@@ -1239,6 +1358,7 @@ static const struct x86_emulate_ops hvm_
     .rep_ins       = hvmemul_rep_ins,
     .rep_outs      = hvmemul_rep_outs,
     .rep_movs      = hvmemul_rep_movs,
+    .rep_stos      = hvmemul_rep_stos,
     .read_segment  = hvmemul_read_segment,
     .write_segment = hvmemul_write_segment,
     .read_io       = hvmemul_read_io,
@@ -1264,6 +1384,7 @@ static const struct x86_emulate_ops hvm_
     .rep_ins       = hvmemul_rep_ins_discard,
     .rep_outs      = hvmemul_rep_outs_discard,
     .rep_movs      = hvmemul_rep_movs_discard,
+    .rep_stos      = hvmemul_rep_stos_discard,
     .read_segment  = hvmemul_read_segment,
     .write_segment = hvmemul_write_segment,
     .read_io       = hvmemul_read_io_discard,
--- unstable.orig/xen/arch/x86/hvm/stdvga.c	2015-01-09 11:19:01.000000000 +0100
+++ unstable/xen/arch/x86/hvm/stdvga.c	2014-11-04 17:42:12.000000000 +0100
@@ -470,11 +470,11 @@ static int mmio_move(struct hvm_hw_stdvg
     uint64_t addr = p->addr;
     p2m_type_t p2mt;
     struct domain *d = current->domain;
+    int step = p->df ? -p->size : p->size;
 
     if ( p->data_is_ptr )
     {
         uint64_t data = p->data, tmp;
-        int step = p->df ? -p->size : p->size;
 
         if ( p->dir == IOREQ_READ )
         {
@@ -529,13 +529,18 @@ static int mmio_move(struct hvm_hw_stdvg
             }
         }
     }
+    else if ( p->dir == IOREQ_WRITE )
+    {
+        for ( i = 0; i < p->count; i++ )
+        {
+            stdvga_mem_write(addr, p->data, p->size);
+            addr += step;
+        }
+    }
     else
     {
         ASSERT(p->count == 1);
-        if ( p->dir == IOREQ_READ )
-            p->data = stdvga_mem_read(addr, p->size);
-        else
-            stdvga_mem_write(addr, p->data, p->size);
+        p->data = stdvga_mem_read(addr, p->size);
     }
 
     read_data = p->data;
--- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.c	2015-01-09 11:19:01.000000000 +0100
+++ unstable/xen/arch/x86/x86_emulate/x86_emulate.c	2015-01-09 11:08:19.000000000 +0100
@@ -2568,15 +2568,25 @@ x86_emulate(
     }
 
     case 0xaa ... 0xab: /* stos */ {
-        /* unsigned long max_reps = */get_rep_prefix();
-        dst.type  = OP_MEM;
+        unsigned long nr_reps = get_rep_prefix();
         dst.bytes = (d & ByteOp) ? 1 : op_bytes;
         dst.mem.seg = x86_seg_es;
         dst.mem.off = truncate_ea(_regs.edi);
-        dst.val   = _regs.eax;
+        if ( (nr_reps == 1) || !ops->rep_stos ||
+             ((rc = ops->rep_stos(&_regs.eax,
+                                  dst.mem.seg, dst.mem.off, dst.bytes,
+                                  &nr_reps, ctxt)) == X86EMUL_UNHANDLEABLE) )
+        {
+            dst.val = _regs.eax;
+            dst.type = OP_MEM;
+            nr_reps = 1;
+        }
+        else if ( rc != X86EMUL_OKAY )
+            goto done;
         register_address_increment(
-            _regs.edi, (_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes);
-        put_rep_prefix(1);
+            _regs.edi,
+            nr_reps * ((_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes));
+        put_rep_prefix(nr_reps);
         break;
     }
 
--- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.h	2015-01-09 11:19:01.000000000 +0100
+++ unstable/xen/arch/x86/x86_emulate/x86_emulate.h	2014-11-04 17:06:49.000000000 +0100
@@ -241,6 +241,20 @@ struct x86_emulate_ops
         struct x86_emulate_ctxt *ctxt);
 
     /*
+     * rep_stos: Emulate STOS: <*p_data> -> <seg:offset>.
+     *  @bytes_per_rep: [IN ] Bytes transferred per repetition.
+     *  @reps:  [IN ] Maximum repetitions to be emulated.
+     *          [OUT] Number of repetitions actually emulated.
+     */
+    int (*rep_stos)(
+        void *p_data,
+        enum x86_segment seg,
+        unsigned long offset,
+        unsigned int bytes_per_rep,
+        unsigned long *reps,
+        struct x86_emulate_ctxt *ctxt);
+
+    /*
      * read_segment: Emulate a read of full context of a segment register.
      *  @reg:   [OUT] Contents of segment register (visible and hidden state).
      */

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

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

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Andrew Cooper]

On 09/01/15 10:27, Jan Beulich wrote:
> While the REP MOVS acceleration appears to have helped qemu-traditional
> based guests, qemu-upstream (or really the respective video BIOSes)
> doesn't appear to benefit from that. Instead the acceleration added
> here provides a visible performance improvement during very early HVM
> guest boot.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: fix asm() constraints in hvmemul_rep_stos(), as pointed out by
>     Andrew. Add output operand telling the compiler that "buf" is being
>     written.

Is writing buf wise? it looks like you will xfree() a wild pointer, and
appears to interfere the "buf != p_data" logic.

~Andrew

>
> --- unstable.orig/xen/arch/x86/hvm/emulate.c	2015-01-09 11:19:01.000000000 +0100
> +++ unstable/xen/arch/x86/hvm/emulate.c	2015-01-09 11:19:36.000000000 +0100
> @@ -731,6 +731,17 @@ static int hvmemul_rep_movs_discard(
>      return X86EMUL_OKAY;
>  }
>  
> +static int hvmemul_rep_stos_discard(
> +    void *p_data,
> +    enum x86_segment seg,
> +    unsigned long offset,
> +    unsigned int bytes_per_rep,
> +    unsigned long *reps,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    return X86EMUL_OKAY;
> +}
> +
>  static int hvmemul_rep_outs_discard(
>      enum x86_segment src_seg,
>      unsigned long src_offset,
> @@ -982,6 +993,114 @@ static int hvmemul_rep_movs(
>      return X86EMUL_OKAY;
>  }
>  
> +static int hvmemul_rep_stos(
> +    void *p_data,
> +    enum x86_segment seg,
> +    unsigned long offset,
> +    unsigned int bytes_per_rep,
> +    unsigned long *reps,
> +    struct x86_emulate_ctxt *ctxt)
> +{
> +    struct hvm_emulate_ctxt *hvmemul_ctxt =
> +        container_of(ctxt, struct hvm_emulate_ctxt, ctxt);
> +    unsigned long addr;
> +    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 )
> +    {
> +        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);
> +    }
> +    if ( rc != X86EMUL_OKAY )
> +        return rc;
> +
> +    /* Check for MMIO op */
> +    (void)get_gfn_query_unlocked(current->domain, gpa >> PAGE_SHIFT, &p2mt);
> +
> +    switch ( p2mt )
> +    {
> +        unsigned long bytes;
> +        void *buf;
> +
> +    default:
> +        /* Allocate temporary buffer. */
> +        for ( ; ; )
> +        {
> +            bytes = *reps * bytes_per_rep;
> +            buf = xmalloc_bytes(bytes);
> +            if ( buf || *reps <= 1 )
> +                break;
> +            *reps >>= 1;
> +        }
> +
> +        if ( !buf )
> +            buf = p_data;
> +        else
> +            switch ( bytes_per_rep )
> +            {
> +                unsigned long dummy;
> +
> +#define CASE(bits, suffix)                                     \
> +            case (bits) / 8:                                   \
> +                asm ( "rep stos" #suffix                       \
> +                      : "=m" (*(char (*)[bytes])buf),          \
> +                        "=D" (dummy), "=c" (dummy)             \
> +                      : "a" (*(const uint##bits##_t *)p_data), \
> +                         "1" (buf), "2" (*reps)                \
> +                      : "memory" );                            \
> +                break
> +            CASE(8, b);
> +            CASE(16, w);
> +            CASE(32, l);
> +            CASE(64, q);
> +#undef CASE
> +
> +            default:
> +                xfree(buf);
> +                ASSERT(!buf);
> +                return X86EMUL_UNHANDLEABLE;
> +            }
> +
> +        /* Adjust address for reverse store. */
> +        if ( df )
> +            gpa -= bytes - bytes_per_rep;
> +
> +        rc = hvm_copy_to_guest_phys(gpa, buf, bytes);
> +
> +        if ( buf != p_data )
> +            xfree(buf);
> +
> +        switch ( rc )
> +        {
> +        case HVMCOPY_gfn_paged_out:
> +        case HVMCOPY_gfn_shared:
> +            return X86EMUL_RETRY;
> +        case HVMCOPY_okay:
> +            return X86EMUL_OKAY;
> +        }
> +
> +        gdprintk(XENLOG_WARNING,
> +                 "Failed REP STOS: gpa=%"PRIpaddr" reps=%lu bytes_per_rep=%u\n",
> +                 gpa, *reps, bytes_per_rep);
> +        /* fall through */
> +    case p2m_mmio_direct:
> +        return X86EMUL_UNHANDLEABLE;
> +
> +    case p2m_mmio_dm:
> +        return hvmemul_do_mmio(gpa, reps, bytes_per_rep, 0, IOREQ_WRITE, df,
> +                               p_data);
> +    }
> +}
> +
>  static int hvmemul_read_segment(
>      enum x86_segment seg,
>      struct segment_register *reg,
> @@ -1239,6 +1358,7 @@ static const struct x86_emulate_ops hvm_
>      .rep_ins       = hvmemul_rep_ins,
>      .rep_outs      = hvmemul_rep_outs,
>      .rep_movs      = hvmemul_rep_movs,
> +    .rep_stos      = hvmemul_rep_stos,
>      .read_segment  = hvmemul_read_segment,
>      .write_segment = hvmemul_write_segment,
>      .read_io       = hvmemul_read_io,
> @@ -1264,6 +1384,7 @@ static const struct x86_emulate_ops hvm_
>      .rep_ins       = hvmemul_rep_ins_discard,
>      .rep_outs      = hvmemul_rep_outs_discard,
>      .rep_movs      = hvmemul_rep_movs_discard,
> +    .rep_stos      = hvmemul_rep_stos_discard,
>      .read_segment  = hvmemul_read_segment,
>      .write_segment = hvmemul_write_segment,
>      .read_io       = hvmemul_read_io_discard,
> --- unstable.orig/xen/arch/x86/hvm/stdvga.c	2015-01-09 11:19:01.000000000 +0100
> +++ unstable/xen/arch/x86/hvm/stdvga.c	2014-11-04 17:42:12.000000000 +0100
> @@ -470,11 +470,11 @@ static int mmio_move(struct hvm_hw_stdvg
>      uint64_t addr = p->addr;
>      p2m_type_t p2mt;
>      struct domain *d = current->domain;
> +    int step = p->df ? -p->size : p->size;
>  
>      if ( p->data_is_ptr )
>      {
>          uint64_t data = p->data, tmp;
> -        int step = p->df ? -p->size : p->size;
>  
>          if ( p->dir == IOREQ_READ )
>          {
> @@ -529,13 +529,18 @@ static int mmio_move(struct hvm_hw_stdvg
>              }
>          }
>      }
> +    else if ( p->dir == IOREQ_WRITE )
> +    {
> +        for ( i = 0; i < p->count; i++ )
> +        {
> +            stdvga_mem_write(addr, p->data, p->size);
> +            addr += step;
> +        }
> +    }
>      else
>      {
>          ASSERT(p->count == 1);
> -        if ( p->dir == IOREQ_READ )
> -            p->data = stdvga_mem_read(addr, p->size);
> -        else
> -            stdvga_mem_write(addr, p->data, p->size);
> +        p->data = stdvga_mem_read(addr, p->size);
>      }
>  
>      read_data = p->data;
> --- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.c	2015-01-09 11:19:01.000000000 +0100
> +++ unstable/xen/arch/x86/x86_emulate/x86_emulate.c	2015-01-09 11:08:19.000000000 +0100
> @@ -2568,15 +2568,25 @@ x86_emulate(
>      }
>  
>      case 0xaa ... 0xab: /* stos */ {
> -        /* unsigned long max_reps = */get_rep_prefix();
> -        dst.type  = OP_MEM;
> +        unsigned long nr_reps = get_rep_prefix();
>          dst.bytes = (d & ByteOp) ? 1 : op_bytes;
>          dst.mem.seg = x86_seg_es;
>          dst.mem.off = truncate_ea(_regs.edi);
> -        dst.val   = _regs.eax;
> +        if ( (nr_reps == 1) || !ops->rep_stos ||
> +             ((rc = ops->rep_stos(&_regs.eax,
> +                                  dst.mem.seg, dst.mem.off, dst.bytes,
> +                                  &nr_reps, ctxt)) == X86EMUL_UNHANDLEABLE) )
> +        {
> +            dst.val = _regs.eax;
> +            dst.type = OP_MEM;
> +            nr_reps = 1;
> +        }
> +        else if ( rc != X86EMUL_OKAY )
> +            goto done;
>          register_address_increment(
> -            _regs.edi, (_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes);
> -        put_rep_prefix(1);
> +            _regs.edi,
> +            nr_reps * ((_regs.eflags & EFLG_DF) ? -dst.bytes : dst.bytes));
> +        put_rep_prefix(nr_reps);
>          break;
>      }
>  
> --- unstable.orig/xen/arch/x86/x86_emulate/x86_emulate.h	2015-01-09 11:19:01.000000000 +0100
> +++ unstable/xen/arch/x86/x86_emulate/x86_emulate.h	2014-11-04 17:06:49.000000000 +0100
> @@ -241,6 +241,20 @@ struct x86_emulate_ops
>          struct x86_emulate_ctxt *ctxt);
>  
>      /*
> +     * rep_stos: Emulate STOS: <*p_data> -> <seg:offset>.
> +     *  @bytes_per_rep: [IN ] Bytes transferred per repetition.
> +     *  @reps:  [IN ] Maximum repetitions to be emulated.
> +     *          [OUT] Number of repetitions actually emulated.
> +     */
> +    int (*rep_stos)(
> +        void *p_data,
> +        enum x86_segment seg,
> +        unsigned long offset,
> +        unsigned int bytes_per_rep,
> +        unsigned long *reps,
> +        struct x86_emulate_ctxt *ctxt);
> +
> +    /*
>       * read_segment: Emulate a read of full context of a segment register.
>       *  @reg:   [OUT] Contents of segment register (visible and hidden state).
>       */
>
>

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

>>> On 09.01.15 at 11:56, <andrew.cooper3@citrix.com> wrote:
> On 09/01/15 10:27, Jan Beulich wrote:
>> While the REP MOVS acceleration appears to have helped qemu-traditional
>> based guests, qemu-upstream (or really the respective video BIOSes)
>> doesn't appear to benefit from that. Instead the acceleration added
>> here provides a visible performance improvement during very early HVM
>> guest boot.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> v2: fix asm() constraints in hvmemul_rep_stos(), as pointed out by
>>     Andrew. Add output operand telling the compiler that "buf" is being
>>     written.
> 
> Is writing buf wise? it looks like you will xfree() a wild pointer, and
> appears to interfere the "buf != p_data" logic.

Perhaps the textual description was a little ambiguous, but the code
is not: It's not the variable "buf" that gets written, but buf[] in its
entirety.

Jan

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Tim Deegan]

At 10:56 +0000 on 09 Jan (1420797418), Andrew Cooper wrote:
> On 09/01/15 10:27, Jan Beulich wrote:
> > While the REP MOVS acceleration appears to have helped qemu-traditional
> > based guests, qemu-upstream (or really the respective video BIOSes)
> > doesn't appear to benefit from that. Instead the acceleration added
> > here provides a visible performance improvement during very early HVM
> > guest boot.
> >
> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > ---
> > v2: fix asm() constraints in hvmemul_rep_stos(), as pointed out by
> >     Andrew. Add output operand telling the compiler that "buf" is being
> >     written.
> 
> Is writing buf wise? it looks like you will xfree() a wild pointer, and
> appears to interfere the "buf != p_data" logic.

I think the constraints are correct, though the 'memory' clobber makes
the rest of it moot.  But this:

> > +            default:
> > +                xfree(buf);
> > +                ASSERT(!buf);

looks dodgy...

Tim.

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

>>> On 09.01.15 at 12:18, <tim@xen.org> wrote:
> At 10:56 +0000 on 09 Jan (1420797418), Andrew Cooper wrote:
>> On 09/01/15 10:27, Jan Beulich wrote:
>> > While the REP MOVS acceleration appears to have helped qemu-traditional
>> > based guests, qemu-upstream (or really the respective video BIOSes)
>> > doesn't appear to benefit from that. Instead the acceleration added
>> > here provides a visible performance improvement during very early HVM
>> > guest boot.
>> >
>> > Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> > ---
>> > v2: fix asm() constraints in hvmemul_rep_stos(), as pointed out by
>> >     Andrew. Add output operand telling the compiler that "buf" is being
>> >     written.
>> 
>> Is writing buf wise? it looks like you will xfree() a wild pointer, and
>> appears to interfere the "buf != p_data" logic.
> 
> I think the constraints are correct, though the 'memory' clobber makes
> the rest of it moot.  But this:

Ah, yes, the memory clobber could be dropped now that the "=m"
is there.

>> > +            default:
>> > +                xfree(buf);
>> > +                ASSERT(!buf);
> 
> looks dodgy...

In which way? The "default" is supposed to be unreachable, and sits
in the else branch to an if(!buf), i.e. in a release build we'll correctly
free the buffer, while in a debug build the ASSERT() will trigger.

Jan

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Tim Deegan]

At 11:24 +0000 on 09 Jan (1420799087), Jan Beulich wrote:
> >>> On 09.01.15 at 12:18, <tim@xen.org> wrote:
> >> > +            default:
> >> > +                xfree(buf);
> >> > +                ASSERT(!buf);
> > 
> > looks dodgy...
> 
> In which way? The "default" is supposed to be unreachable, and sits
> in the else branch to an if(!buf), i.e. in a release build we'll correctly
> free the buffer, while in a debug build the ASSERT() will trigger.

Oh I see.  Can you please use ASSERT(0) for that?

Tim.

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

>>> On 09.01.15 at 12:45, <tim@xen.org> wrote:
> At 11:24 +0000 on 09 Jan (1420799087), Jan Beulich wrote:
>> >>> On 09.01.15 at 12:18, <tim@xen.org> wrote:
>> >> > +            default:
>> >> > +                xfree(buf);
>> >> > +                ASSERT(!buf);
>> > 
>> > looks dodgy...
>> 
>> In which way? The "default" is supposed to be unreachable, and sits
>> in the else branch to an if(!buf), i.e. in a release build we'll correctly
>> free the buffer, while in a debug build the ASSERT() will trigger.
> 
> Oh I see.  Can you please use ASSERT(0) for that?

I sincerely dislike ASSERT(0), but if that's the only way to get
the patch accepted...

Jan

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Andrew Cooper]

On 09/01/15 12:10, Jan Beulich wrote:
>>>> On 09.01.15 at 12:45, <tim@xen.org> wrote:
>> At 11:24 +0000 on 09 Jan (1420799087), Jan Beulich wrote:
>>>>>> On 09.01.15 at 12:18, <tim@xen.org> wrote:
>>>>>> +            default:
>>>>>> +                xfree(buf);
>>>>>> +                ASSERT(!buf);
>>>> looks dodgy...
>>> In which way? The "default" is supposed to be unreachable, and sits
>>> in the else branch to an if(!buf), i.e. in a release build we'll correctly
>>> free the buffer, while in a debug build the ASSERT() will trigger.
>> Oh I see.  Can you please use ASSERT(0) for that?
> I sincerely dislike ASSERT(0), but if that's the only way to get
> the patch accepted...
>
> Jan
>

Perhaps introducing ASSERT_UNREACHABLE() as an alternative which is more
obvious in nature than both ASSERT(!buf) and ASSERT(0) ?

~Andrew

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Tim Deegan]

At 12:46 +0000 on 09 Jan (1420804005), Andrew Cooper wrote:
> On 09/01/15 12:10, Jan Beulich wrote:
> >>>> On 09.01.15 at 12:45, <tim@xen.org> wrote:
> >> At 11:24 +0000 on 09 Jan (1420799087), Jan Beulich wrote:
> >>>>>> On 09.01.15 at 12:18, <tim@xen.org> wrote:
> >>>>>> +            default:
> >>>>>> +                xfree(buf);
> >>>>>> +                ASSERT(!buf);
> >>>> looks dodgy...
> >>> In which way? The "default" is supposed to be unreachable, and sits
> >>> in the else branch to an if(!buf), i.e. in a release build we'll correctly
> >>> free the buffer, while in a debug build the ASSERT() will trigger.
> >> Oh I see.  Can you please use ASSERT(0) for that?
> > I sincerely dislike ASSERT(0), but if that's the only way to get
> > the patch accepted...
> >
> > Jan
> >
> 
> Perhaps introducing ASSERT_UNREACHABLE() as an alternative which is more
> obvious in nature than both ASSERT(!buf) and ASSERT(0) ?

Fine by me!

Tim.

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[George Dunlap]

On Fri, Jan 9, 2015 at 12:10 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 09.01.15 at 12:45, <tim@xen.org> wrote:
>> At 11:24 +0000 on 09 Jan (1420799087), Jan Beulich wrote:
>>> >>> On 09.01.15 at 12:18, <tim@xen.org> wrote:
>>> >> > +            default:
>>> >> > +                xfree(buf);
>>> >> > +                ASSERT(!buf);
>>> >
>>> > looks dodgy...
>>>
>>> In which way? The "default" is supposed to be unreachable, and sits
>>> in the else branch to an if(!buf), i.e. in a release build we'll correctly
>>> free the buffer, while in a debug build the ASSERT() will trigger.
>>
>> Oh I see.  Can you please use ASSERT(0) for that?
>
> I sincerely dislike ASSERT(0), but if that's the only way to get
> the patch accepted...

Um, by what set of criteria is ASSERT(!buf) better than ASSERT(0)?  At
least the second tells the reader that you're not using ASSERT() the
normal way.

I agree that ASSERT_UNREACHABLE() is probably the best option.

 -George

Re: [PATCH v2 1/3] x86: also allow REP STOS emulation acceleration

[Jan Beulich]

>>> On 12.01.15 at 15:54, <George.Dunlap@eu.citrix.com> wrote:
> On Fri, Jan 9, 2015 at 12:10 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 09.01.15 at 12:45, <tim@xen.org> wrote:
>>> At 11:24 +0000 on 09 Jan (1420799087), Jan Beulich wrote:
>>>> >>> On 09.01.15 at 12:18, <tim@xen.org> wrote:
>>>> >> > +            default:
>>>> >> > +                xfree(buf);
>>>> >> > +                ASSERT(!buf);
>>>> >
>>>> > looks dodgy...
>>>>
>>>> In which way? The "default" is supposed to be unreachable, and sits
>>>> in the else branch to an if(!buf), i.e. in a release build we'll correctly
>>>> free the buffer, while in a debug build the ASSERT() will trigger.
>>>
>>> Oh I see.  Can you please use ASSERT(0) for that?
>>
>> I sincerely dislike ASSERT(0), but if that's the only way to get
>> the patch accepted...
> 
> Um, by what set of criteria is ASSERT(!buf) better than ASSERT(0)?  At
> least the second tells the reader that you're not using ASSERT() the
> normal way.

Because the resulting message ("0") is completely meaningless,
while for "!buf" you at least have a chance of finding the original
ASSERT() without line numbers matching up 100%.

Jan