uq/master: TPR access optimization for Windows guests

hw/kvmvapic.c new file +16 -0

+static void vapic_map_rom_writable(VAPICROMState *s)
+{
+    target_phys_addr_t rom_paddr = s->rom_state_paddr & ROM_BLOCK_MASK;
+    MemoryRegionSection section;
+    MemoryRegion *as;
+    size_t rom_size;
+    uint8_t *ram;
+
+    as = sysbus_address_space(&s->busdev);
+
+    if (s->rom_mapped_writable) {
+        memory_region_del_subregion(as, &s->rom);
+        memory_region_destroy(&s->rom);
+    }
+
+    /* grab RAM memory region (region @rom_paddr may still be pc.rom) */

Avi,

Before I forget:

Quoted 21 lines · Jan Kiszka

hw/kvmvapic.c new file +21 -0

+static void vapic_map_rom_writable(VAPICROMState *s)
+{
+    target_phys_addr_t rom_paddr = s->rom_state_paddr & ROM_BLOCK_MASK;
+    MemoryRegionSection section;
+    MemoryRegion *as;
+    size_t rom_size;
+    uint8_t *ram;
+
+    as = sysbus_address_space(&s->busdev);
+
+    if (s->rom_mapped_writable) {
+        memory_region_del_subregion(as, &s->rom);
+        memory_region_destroy(&s->rom);
+    }
+
+    /* grab RAM memory region (region @rom_paddr may still be pc.rom) */
+    section = memory_region_find(as, 0, 1);
+
+    /* read ROM size from RAM region */
+    ram = memory_region_get_ram_ptr(section.mr);
+    rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;

hw/kvmvapic.c line 500

Quoted 5 lines

hw/kvmvapic.c new file +5 -0

+    s->rom_size = rom_size;
+
+    /* FIXME: round up as everything underneath would fall apart otherwise
+     * (subpages are broken) */
+    rom_size = TARGET_PAGE_ALIGN(rom_size);

Removing this alignment triggers an interesting bug in the memory layer. Haven't understood the details yet. Is subpage support supposed to work?

Quoted 26 lines · Jan Kiszka

Avi,

Before I forget:

Quoted 21 lines · Jan Kiszka

hw/kvmvapic.c new file +21 -0

+static void vapic_map_rom_writable(VAPICROMState *s)
+{
+    target_phys_addr_t rom_paddr = s->rom_state_paddr & ROM_BLOCK_MASK;
+    MemoryRegionSection section;
+    MemoryRegion *as;
+    size_t rom_size;
+    uint8_t *ram;
+
+    as = sysbus_address_space(&s->busdev);
+
+    if (s->rom_mapped_writable) {
+        memory_region_del_subregion(as, &s->rom);
+        memory_region_destroy(&s->rom);
+    }
+
+    /* grab RAM memory region (region @rom_paddr may still be pc.rom) */
+    section = memory_region_find(as, 0, 1);
+
+    /* read ROM size from RAM region */
+    ram = memory_region_get_ram_ptr(section.mr);
+    rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;

Quoted 10 lines

hw/kvmvapic.c line 500

Quoted 5 lines

hw/kvmvapic.c new file +5 -0

+    s->rom_size = rom_size;
+
+    /* FIXME: round up as everything underneath would fall apart otherwise
+     * (subpages are broken) */
+    rom_size = TARGET_PAGE_ALIGN(rom_size);

Removing this alignment triggers an interesting bug in the memory layer. Haven't understood the details yet. Is subpage support supposed to work?

There are plenty of restrictions wrt page size in the memory API. Some will be removed, some are enforced by hardware (for example the low bits of the guest address and host address must match).

Subpage of course works, but it can't be direct mapped.

Quoted 42 lines · Avi Kivity

Quoted 26 lines · Jan Kiszka

Avi,

Before I forget:

Quoted 21 lines · Jan Kiszka

hw/kvmvapic.c new file +21 -0

+static void vapic_map_rom_writable(VAPICROMState *s)
+{
+    target_phys_addr_t rom_paddr = s->rom_state_paddr & ROM_BLOCK_MASK;
+    MemoryRegionSection section;
+    MemoryRegion *as;
+    size_t rom_size;
+    uint8_t *ram;
+
+    as = sysbus_address_space(&s->busdev);
+
+    if (s->rom_mapped_writable) {
+        memory_region_del_subregion(as, &s->rom);
+        memory_region_destroy(&s->rom);
+    }
+
+    /* grab RAM memory region (region @rom_paddr may still be pc.rom) */
+    section = memory_region_find(as, 0, 1);
+
+    /* read ROM size from RAM region */
+    ram = memory_region_get_ram_ptr(section.mr);
+    rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;

Quoted 10 lines

hw/kvmvapic.c line 500

Quoted 5 lines

hw/kvmvapic.c new file +5 -0

+    s->rom_size = rom_size;
+
+    /* FIXME: round up as everything underneath would fall apart otherwise
+     * (subpages are broken) */
+    rom_size = TARGET_PAGE_ALIGN(rom_size);

Removing this alignment triggers an interesting bug in the memory layer. Haven't understood the details yet. Is subpage support supposed to work?

There are plenty of restrictions wrt page size in the memory API. Some will be removed, some are enforced by hardware (for example the low bits of the guest address and host address must match).

Quoted 2 lines

Subpage of course works, but it can't be direct mapped.

Does this mean you can't use them with RAM backing? Should probably be documented and caught gracefully in that case.

From: Jan Kiszka <[email protected]>

This enables acceleration for MMIO-based TPR registers accesses of 32-bit Windows guest systems. It is mostly useful with KVM enabled, either on older Intel CPUs (without flexpriority feature, can also be manually disabled for testing) or any current AMD processor.

The approach introduced here is derived from the original version of qemu-kvm. It was refactored, documented, and extended by support for user space APIC emulation, both with and without KVM acceleration.

The
VMState format was kept compatible, so was the ABI to the option ROM that implements the guest-side para-virtualized driver service. This enables seamless migration from qemu-kvm to upstream or, one day, between KVM and TCG mode.

The basic concept goes like this:
- VAPIC PV interface consisting of I/O port 0x7e and (for KVM in-kernel irqchip) a vmcall hypercall is registered
- VAPIC option ROM is loaded into guest
- option ROM activates TPR MMIO access reporting via port 0x7e
- TPR accesses are trapped and patched in the guest to call into option ROM instead, VAPIC support is enabled
- option ROM TPR helpers track state in memory and invoke hypercall to poll for pending IRQs if required

hw/kvmvapic.c new file +23 -0

+
+static int evaluate_tpr_instruction(VAPICROMState *s, CPUState *env,
+                                    target_ulong *pip, int access)
+{
+    target_ulong addr_offset;
+    target_ulong ip = *pip;
+    uint8_t opcode[2];
+    uint32_t real_tpr_addr;
+
+    if ((ip & 0xf0000000) != 0x80000000 && (ip & 0xf0000000) != 0xe0000000) {
+        return -1;
+    }
+
+    /*
+     * Early Windows 2003 SMP initialization contains a
+     *
+     *   mov imm32, r/m32
+     *
+     * instruction that is patched by TPR optimization. The problem is that
+     * RSP, used by the patched instruction, is zero, so the guest gets a
+     * double fault and dies.
+     */
+    if (env->regs[R_ESP] == 0) {

hw/kvmvapic.c new file +48 -0

+        ip -= 5;
+        if (cpu_memory_rw_debug(env, ip, opcode, 1, 0) < 0) {
+            return -1;
+        }
+        if (opcode[0] == 0xa3) {
+            addr_offset = 1;
+            goto instruction_ok;
+        }
+
+        ip--;
+        if (cpu_memory_rw_debug(env, ip, opcode, 2, 0) < 0) {
+            return -1;
+        }
+        if (opcode[0] == 0x89 && is_abs_modrm(opcode[1])) {
+            addr_offset = 2;
+            goto instruction_ok;
+        }
+
+        ip -= 4;
+        if (cpu_memory_rw_debug(env, ip, opcode, 2, 0) < 0) {
+            return -1;
+        }
+        if (opcode[0] != 0xc7 || modrm_reg(opcode[1]) != 0 ||
+            !is_abs_modrm(opcode[1])) {
+            return -1;
+        }
+        addr_offset = 2;
+    } else {
+        if (cpu_memory_rw_debug(env, ip, opcode, sizeof(opcode), 0) < 0) {
+            return -1;
+        }
+
+        switch (opcode[0]) {
+        case 0xc7: /* mov imm32, r/m32 (c7/0) */
+            if (modrm_reg(opcode[1]) != 0) {
+                return -1;
+            }
+            /* fall through */
+        case 0x89: /* mov r32 to r/m32 */
+        case 0x8b: /* mov r/m32 to r32 */
+            if (!is_abs_modrm(opcode[1])) {
+                return -1;
+            }
+            addr_offset = 2;
+            break;
+        case 0xa1: /* mov abs to eax */
+        case 0xa3: /* mov eax to abs */
+            addr_offset = 1;

hw/kvmvapic.c new file +33 -0

+    }
+
+instruction_ok:
+    /*
+     * Grab the virtual TPR address from the instruction
+     * and update the cached values.
+     */
+    if (cpu_memory_rw_debug(env, ip + addr_offset, (void *)&real_tpr_addr,
+                            sizeof(real_tpr_addr), 0) < 0) {
+        return -1;
+    }
+    real_tpr_addr = le32_to_cpu(real_tpr_addr);
+    if ((real_tpr_addr & 0xfff) != 0x80) {
+        return -1;
+    }
+    s->real_tpr_addr = real_tpr_addr;
+    update_guest_rom_state(s);
+
+    *pip = ip;
+    return 0;
+}
+
+
+/*
+ * Tries to read the unique processor number from the Kernel Processor Control
+ * Region (KPCR) of 32-bit Windows. Returns -1 if the KPCR cannot be accessed
+ * or is considered invalid.
+ */
+static int get_kpcr_number(CPUState *env)
+{
+    struct kpcr {
+        uint8_t  fill1[0x1c];
+        uint32_t self;

hw/kvmvapic.c new file +34 -0

+        return -1;
+    }
+    return kpcr.number;
+}
 
+static void patch_instruction(VAPICROMState *s, CPUState *env, target_ulong ip)
+{
+    target_phys_addr_t paddr;
+    VAPICHandlers *handlers;
+    uint8_t opcode[2];
+    uint32_t imm32;
+
+    if (smp_cpus == 1) {
+        handlers = &s->rom_state.up;
+    } else {
+        handlers = &s->rom_state.mp;
+    }
+
+    cpu_memory_rw_debug(env, ip, opcode, sizeof(opcode), 0);
+
+    switch (opcode[0]) {
+    case 0x89: /* mov r32 to r/m32 */
+        patch_byte(env, ip, 0x50 + modrm_reg(opcode[1]));  /* push reg */
+        patch_call(s, env, ip + 1, handlers->set_tpr);
+        break;
+    case 0x8b: /* mov r/m32 to r32 */
+        patch_byte(env, ip, 0x90);
+        patch_call(s, env, ip + 1, handlers->get_tpr[modrm_reg(opcode[1])]);
+        break;
+    case 0xa1: /* mov abs to eax */
+        patch_call(s, env, ip, handlers->get_tpr[0]);
+        break;
+    case 0xa3: /* mov eax to abs */
+        patch_call(s, env, ip, handlers->set_tpr_eax);
+        break;

hw/kvmvapic.c new file +26 -0

+    default:
+        abort();
+    }
+
+    paddr = cpu_get_phys_page_debug(env, ip);
+    paddr += ip & ~TARGET_PAGE_MASK;
+    tb_invalidate_phys_page_range(paddr, paddr + 1, 1);
+}
+
+
+static void vapic_map_rom_writable(VAPICROMState *s)
+{
+    target_phys_addr_t rom_paddr = s->rom_state_paddr & ROM_BLOCK_MASK;
+    MemoryRegionSection section;
+    MemoryRegion *as;
+    size_t rom_size;
+    uint8_t *ram;
+
+    as = sysbus_address_space(&s->busdev);
+
+    if (s->rom_mapped_writable) {
+        memory_region_del_subregion(as, &s->rom);
+        memory_region_destroy(&s->rom);
+    }
+
+    /* grab RAM memory region (region @rom_paddr may still be pc.rom) */

Quoted 7 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

This enables acceleration for MMIO-based TPR registers accesses of 32-bit Windows guest systems. It is mostly useful with KVM enabled, either on older Intel CPUs (without flexpriority feature, can also be manually disabled for testing) or any current AMD processor.

Quoted 3 lines

The approach introduced here is derived from the original version of qemu-kvm. It was refactored, documented, and extended by support for user space APIC emulation, both with and without KVM acceleration.

However, it's presented as a rewrite instead of a series of changes, so we can't see what the changes are.

Having said that, the end result is way, way better than the original.

Quoted 44 lines

The
VMState format was kept compatible, so was the ABI to the option ROM that implements the guest-side para-virtualized driver service. This enables seamless migration from qemu-kvm to upstream or, one day, between KVM and TCG mode.

The basic concept goes like this:
- VAPIC PV interface consisting of I/O port 0x7e and (for KVM in-kernel irqchip) a vmcall hypercall is registered
- VAPIC option ROM is loaded into guest
- option ROM activates TPR MMIO access reporting via port 0x7e
- TPR accesses are trapped and patched in the guest to call into option ROM instead, VAPIC support is enabled
- option ROM TPR helpers track state in memory and invoke hypercall to poll for pending IRQs if required

hw/kvmvapic.c new file +27 -0

+
+static int evaluate_tpr_instruction(VAPICROMState *s, CPUState *env,
+                                    target_ulong *pip, int access)
+{
+    target_ulong addr_offset;
+    target_ulong ip = *pip;
+    uint8_t opcode[2];
+    uint32_t real_tpr_addr;
+
+    if ((ip & 0xf0000000) != 0x80000000 && (ip & 0xf0000000) != 0xe0000000) {
+        return -1;
+    }
+
+    /*
+     * Early Windows 2003 SMP initialization contains a
+     *
+     *   mov imm32, r/m32
+     *
+     * instruction that is patched by TPR optimization. The problem is that
+     * RSP, used by the patched instruction, is zero, so the guest gets a
+     * double fault and dies.
+     */
+    if (env->regs[R_ESP] == 0) {
+        return -1;
+    }
+
+    if (access == TPR_ACCESS_WRITE && kvm_enabled() &&

hw/kvmvapic.c line 145

Quoted 6 lines

hw/kvmvapic.c new file +6 -0

+        !kvm_irqchip_in_kernel()) {
+        /*
+         * KVM without TPR access reporting calls into the user space APIC on
+         * write with IP pointing after the accessing instruction. So we need
+         * to look backward to find the reason.
+         */

Why do we need to do anything at all?

I'm not sure if the ABI guarantees anything about %rip.

Quoted 55 lines

hw/kvmvapic.c new file +54 -0

+        ip -= 5;
+        if (cpu_memory_rw_debug(env, ip, opcode, 1, 0) < 0) {
+            return -1;
+        }
+        if (opcode[0] == 0xa3) {
+            addr_offset = 1;
+            goto instruction_ok;
+        }
+
+        ip--;
+        if (cpu_memory_rw_debug(env, ip, opcode, 2, 0) < 0) {
+            return -1;
+        }
+        if (opcode[0] == 0x89 && is_abs_modrm(opcode[1])) {
+            addr_offset = 2;
+            goto instruction_ok;
+        }
+
+        ip -= 4;
+        if (cpu_memory_rw_debug(env, ip, opcode, 2, 0) < 0) {
+            return -1;
+        }
+        if (opcode[0] != 0xc7 || modrm_reg(opcode[1]) != 0 ||
+            !is_abs_modrm(opcode[1])) {
+            return -1;
+        }
+        addr_offset = 2;
+    } else {
+        if (cpu_memory_rw_debug(env, ip, opcode, sizeof(opcode), 0) < 0) {
+            return -1;
+        }
+
+        switch (opcode[0]) {
+        case 0xc7: /* mov imm32, r/m32 (c7/0) */
+            if (modrm_reg(opcode[1]) != 0) {
+                return -1;
+            }
+            /* fall through */
+        case 0x89: /* mov r32 to r/m32 */
+        case 0x8b: /* mov r/m32 to r32 */
+            if (!is_abs_modrm(opcode[1])) {
+                return -1;
+            }
+            addr_offset = 2;
+            break;
+        case 0xa1: /* mov abs to eax */
+        case 0xa3: /* mov eax to abs */
+            addr_offset = 1;
+            break;
+        case 0xff: /* push r/m32 */
+            if (modrm_reg(opcode[1]) != 6 || !is_abs_modrm(opcode[1])) {
+                return -1;
+            }
+            addr_offset = 2;

hw/kvmvapic.c line 203

Quoted 4 lines

hw/kvmvapic.c new file +4 -0

+            break;
+        default:
+            return -1;
+        }

You could code this as a table of predicate functions and insn sizes. When working backward you use the sizes to offset %rip. This eliminates duplication.

Quoted 39 lines

hw/kvmvapic.c new file +38 -0

+    }
+
+instruction_ok:
+    /*
+     * Grab the virtual TPR address from the instruction
+     * and update the cached values.
+     */
+    if (cpu_memory_rw_debug(env, ip + addr_offset, (void *)&real_tpr_addr,
+                            sizeof(real_tpr_addr), 0) < 0) {
+        return -1;
+    }
+    real_tpr_addr = le32_to_cpu(real_tpr_addr);
+    if ((real_tpr_addr & 0xfff) != 0x80) {
+        return -1;
+    }
+    s->real_tpr_addr = real_tpr_addr;
+    update_guest_rom_state(s);
+
+    *pip = ip;
+    return 0;
+}
+
+
+/*
+ * Tries to read the unique processor number from the Kernel Processor Control
+ * Region (KPCR) of 32-bit Windows. Returns -1 if the KPCR cannot be accessed
+ * or is considered invalid.
+ */
+static int get_kpcr_number(CPUState *env)
+{
+    struct kpcr {
+        uint8_t  fill1[0x1c];
+        uint32_t self;
+        uint8_t  fill2[0x31];
+        uint8_t  number;
+    } QEMU_PACKED kpcr;
+
+    if (smp_cpus == 1) {

hw/kvmvapic.c line 302

Quoted 5 lines

hw/kvmvapic.c new file +5 -0

+        return 0;
+    }
+    if (cpu_memory_rw_debug(env, env->segs[R_FS].base,
+                            (void *)&kpcr, sizeof(kpcr), 0) < 0 ||
+        kpcr.self != env->segs[R_FS].base) {

Ah, so it works. We may want to do it for UP as well, as a way of verifying that the guest is compatible with these hacks.

Quoted 40 lines

hw/kvmvapic.c new file +38 -0

+        return -1;
+    }
+    return kpcr.number;
+}
 
+static void patch_instruction(VAPICROMState *s, CPUState *env, target_ulong ip)
+{
+    target_phys_addr_t paddr;
+    VAPICHandlers *handlers;
+    uint8_t opcode[2];
+    uint32_t imm32;
+
+    if (smp_cpus == 1) {
+        handlers = &s->rom_state.up;
+    } else {
+        handlers = &s->rom_state.mp;
+    }
+
+    cpu_memory_rw_debug(env, ip, opcode, sizeof(opcode), 0);
+
+    switch (opcode[0]) {
+    case 0x89: /* mov r32 to r/m32 */
+        patch_byte(env, ip, 0x50 + modrm_reg(opcode[1]));  /* push reg */
+        patch_call(s, env, ip + 1, handlers->set_tpr);
+        break;
+    case 0x8b: /* mov r/m32 to r32 */
+        patch_byte(env, ip, 0x90);
+        patch_call(s, env, ip + 1, handlers->get_tpr[modrm_reg(opcode[1])]);
+        break;
+    case 0xa1: /* mov abs to eax */
+        patch_call(s, env, ip, handlers->get_tpr[0]);
+        break;
+    case 0xa3: /* mov eax to abs */
+        patch_call(s, env, ip, handlers->set_tpr_eax);
+        break;
+    case 0xc7: /* mov imm32, r/m32 (c7/0) */
+        patch_byte(env, ip, 0x68);  /* push imm32 */
+        cpu_memory_rw_debug(env, ip + 6, (void *)&imm32, sizeof(imm32), 0);
+        cpu_memory_rw_debug(env, ip + 1, (void *)&imm32, sizeof(imm32), 1);

hw/kvmvapic.c line 382

Quoted 6 lines

hw/kvmvapic.c new file +6 -0

+        patch_call(s, env, ip + 5, handlers->set_tpr);
+        break;
+    case 0xff: /* push r/m32 */
+        patch_byte(env, ip, 0x50); /* push eax */
+        patch_call(s, env, ip + 1, handlers->get_tpr_stack);
+        break;

With a table, we could put the patch sequences there as well.

Quoted 27 lines

hw/kvmvapic.c new file +26 -0

+    default:
+        abort();
+    }
+
+    paddr = cpu_get_phys_page_debug(env, ip);
+    paddr += ip & ~TARGET_PAGE_MASK;
+    tb_invalidate_phys_page_range(paddr, paddr + 1, 1);
+}
+
+
+static void vapic_map_rom_writable(VAPICROMState *s)
+{
+    target_phys_addr_t rom_paddr = s->rom_state_paddr & ROM_BLOCK_MASK;
+    MemoryRegionSection section;
+    MemoryRegion *as;
+    size_t rom_size;
+    uint8_t *ram;
+
+    as = sysbus_address_space(&s->busdev);
+
+    if (s->rom_mapped_writable) {
+        memory_region_del_subregion(as, &s->rom);
+        memory_region_destroy(&s->rom);
+    }
+
+    /* grab RAM memory region (region @rom_paddr may still be pc.rom) */

hw/kvmvapic.c line 500

Quoted 10 lines

hw/kvmvapic.c new file +10 -0

+    section = memory_region_find(as, 0, 1);
+
+    /* read ROM size from RAM region */
+    ram = memory_region_get_ram_ptr(section.mr);
+    rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;
+    s->rom_size = rom_size;
+
+    /* FIXME: round up as everything underneath would fall apart otherwise
+     * (subpages are broken) */
+    rom_size = TARGET_PAGE_ALIGN(rom_size);

Ok. Subpages aren't broken (at least, they can't be expected to work here). Without a page aligned region you can't expect kvm to map this area.

Wrt tcg, it should work, but I don't think anyone ever tested tcg out of subpage.

hw/kvmvapic.c line 504

Quoted 5 lines

hw/kvmvapic.c new file +4 -0

+
+    memory_region_init_alias(&s->rom, "kvmvapic-rom", section.mr, rom_paddr,
+                             rom_size);
+    memory_region_add_subregion_overlap(as, rom_paddr, &s->rom, 1000);

This is incredibly hacky, so at least the spirit of the original code is preserved.

hw/kvmvapic.c line 507

Quoted 5 lines

hw/kvmvapic.c new file +3 -0

+    s->rom_mapped_writable = true;
+}
+

Impressive refactoring overall; I thought that code was a basket case.

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

This enables acceleration for MMIO-based TPR registers accesses of 32-bit Windows guest systems. It is mostly useful with KVM enabled, either on older Intel CPUs (without flexpriority feature, can also be manually disabled for testing) or any current AMD processor.

The approach introduced here is derived from the original version of qemu-kvm. It was refactored, documented, and extended by support for user space APIC emulation, both with and without KVM acceleration.

Quoted 2 lines

However, it's presented as a rewrite instead of a series of changes, so we can't see what the changes are.

Yes, but the original code also depends on interfaces we don't have in upstream.

Quoted 9 lines

Quoted 8 lines

hw/kvmvapic.c new file +8 -0

+
+    if (access == TPR_ACCESS_WRITE && kvm_enabled() &&
+        !kvm_irqchip_in_kernel()) {
+        /*
+         * KVM without TPR access reporting calls into the user space APIC on
+         * write with IP pointing after the accessing instruction. So we need
+         * to look backward to find the reason.
+         */

Quoted 1 line

Why do we need to do anything at all?

We need to patch the causing instruction, so we have to know where it starts. Or what do you mean?

Quoted 2 lines

I'm not sure if the ABI guarantees anything about %rip.

That's indeed a point. It's likely coupled to the emulator's internals and when it calls out to user space for MMIO write. How to deal with it?

Quoted 10 lines

Quoted 10 lines

hw/kvmvapic.c new file +10 -0

+static int get_kpcr_number(CPUState *env)
+{
+    struct kpcr {
+        uint8_t  fill1[0x1c];
+        uint32_t self;
+        uint8_t  fill2[0x31];
+        uint8_t  number;
+    } QEMU_PACKED kpcr;
+
+    if (smp_cpus == 1) {

Quoted 8 lines

hw/kvmvapic.c line 302

Quoted 5 lines

hw/kvmvapic.c new file +5 -0

+        return 0;
+    }
+    if (cpu_memory_rw_debug(env, env->segs[R_FS].base,
+                            (void *)&kpcr, sizeof(kpcr), 0) < 0 ||
+        kpcr.self != env->segs[R_FS].base) {

Ah, so it works. We may want to do it for UP as well, as a way of verifying that the guest is compatible with these hacks.

I'm not sure if Windows has this properly set up for the UP HAL. I rather think this was a bug in the original implementation. The ROM uses 0 as CPU index in UP mode unconditionally, so should we in QEMU.

Quoted 6 lines

Quoted 5 lines

hw/kvmvapic.c new file +4 -0

+
+    memory_region_init_alias(&s->rom, "kvmvapic-rom", section.mr, rom_paddr,
+                             rom_size);
+    memory_region_add_subregion_overlap(as, rom_paddr, &s->rom, 1000);

Quoted 2 lines

This is incredibly hacky, so at least the spirit of the original code is preserved.

I know, and it caused some pain to write it (not only to find out how to solve it technically). We would need to pass the RAM memory region down to this freaky device, like we do to the i440fx for PAM purposes. But, well, that is not straightforward right now. Better ideas welcome.

Quoted 13 lines · Jan Kiszka

Quoted 12 lines · Avi Kivity

Quoted 10 lines · Jan Kiszka

From: Jan Kiszka <[email protected]>

This enables acceleration for MMIO-based TPR registers accesses of 32-bit Windows guest systems. It is mostly useful with KVM enabled, either on older Intel CPUs (without flexpriority feature, can also be manually disabled for testing) or any current AMD processor.

The approach introduced here is derived from the original version of qemu-kvm. It was refactored, documented, and extended by support for user space APIC emulation, both with and without KVM acceleration.

Quoted 5 lines

Quoted 2 lines

However, it's presented as a rewrite instead of a series of changes, so we can't see what the changes are.

Yes, but the original code also depends on interfaces we don't have in upstream.

The usual rant: patch qemu-kvm until it's suitable for upsteam, then submit the end result for upstream. But you don't deserve this today.

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

hw/kvmvapic.c new file +8 -0

+
+    if (access == TPR_ACCESS_WRITE && kvm_enabled() &&
+        !kvm_irqchip_in_kernel()) {
+        /*
+         * KVM without TPR access reporting calls into the user space APIC on
+         * write with IP pointing after the accessing instruction. So we need
+         * to look backward to find the reason.
+         */

Quoted 4 lines

Quoted 1 line

Why do we need to do anything at all?

We need to patch the causing instruction, so we have to know where it starts. Or what do you mean?

Just don't deal with this at all, no one runs on kernels without kernel irqchip.

Quoted 6 lines

Quoted 3 lines

I'm not sure if the ABI guarantees anything about %rip.

That's indeed a point. It's likely coupled to the emulator's internals and when it calls out to user space for MMIO write. How to deal with it?

One way is to verify that it worked this way at least N versions back, and then retro-doc it. The downside is that it reduces our flexibility in the future, but I think that's a small downside.

Another way is not to do it at all.

Quoted 19 lines

Quoted 18 lines

Quoted 5 lines

hw/kvmvapic.c new file +5 -0

+static int get_kpcr_number(CPUState *env)
+{
+    struct kpcr {
+        uint8_t  fill1[0x1c];
+        uint32_t self;

hw/kvmvapic.c line 302

Quoted 10 lines

hw/kvmvapic.c new file +10 -0

+        uint8_t  fill2[0x31];
+        uint8_t  number;
+    } QEMU_PACKED kpcr;
+
+    if (smp_cpus == 1) {
+        return 0;
+    }
+    if (cpu_memory_rw_debug(env, env->segs[R_FS].base,
+                            (void *)&kpcr, sizeof(kpcr), 0) < 0 ||
+        kpcr.self != env->segs[R_FS].base) {

Ah, so it works. We may want to do it for UP as well, as a way of verifying that the guest is compatible with these hacks.

Quoted 3 lines

I'm not sure if Windows has this properly set up for the UP HAL. I rather think this was a bug in the original implementation. The ROM uses 0 as CPU index in UP mode unconditionally, so should we in QEMU.

I mean just check kpcr.self.

The reason up and smp are so different is that for a long while smp didn't work at all, and for some time it used even uglier hacks than we have today (like stashing the cpu id in TR.sel), so I didn't want to pollute th up code with the smp ugliness. It's also marginally faster (less locked ops), though I doubt it matters on today's processors.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

hw/kvmvapic.c new file +4 -0

+
+    memory_region_init_alias(&s->rom, "kvmvapic-rom", section.mr, rom_paddr,
+                             rom_size);
+    memory_region_add_subregion_overlap(as, rom_paddr, &s->rom, 1000);

Quoted 7 lines

Quoted 2 lines

This is incredibly hacky, so at least the spirit of the original code is preserved.

I know, and it caused some pain to write it (not only to find out how to solve it technically). We would need to pass the RAM memory region down to this freaky device, like we do to the i440fx for PAM purposes. But, well, that is not straightforward right now. Better ideas welcome.

Could we make it a child<> of i440FX, and have i440FX pass it the MemoryRegion directly?

It means we'll need to redo the glue for new chipsets, but it should be just a few lines.

Quoted 6 lines

Quoted 3 lines

We need to patch the causing instruction, so we have to know where it starts. Or what do you mean?

Just don't deal with this at all, no one runs on kernels without kernel irqchip.

Not true for upstream,

and not a design goal of this approach,
specifically when considering that it also works with TCG. Would be a pity to lose this generality.

Quoted 8 lines

Quoted 7 lines

Quoted 4 lines

I'm not sure if the ABI guarantees anything about %rip.

That's indeed a point. It's likely coupled to the emulator's internals and when it calls out to user space for MMIO write. How to deal with it?

Quoted 3 lines

One way is to verify that it worked this way at least N versions back, and then retro-doc it. The downside is that it reduces our flexibility in the future, but I think that's a small downside.

It need not reduce our flexibility, we just need to signal to user space when our behaviour changes again.

Quoted 6 lines

Quoted 4 lines

I'm not sure if Windows has this properly set up for the UP HAL. I rather think this was a bug in the original implementation. The ROM uses 0 as CPU index in UP mode unconditionally, so should we in QEMU.

I mean just check kpcr.self.

Yes, clear, but that means that Windows must have initialized FS.base to point to the KPCR also in UP mode. Is that really the case? E.g. when ACPI is off?!

I wonder if that explains the reported bug of qemu-kvm with -no-acpi and in-kernel irqchip...

Quoted 9 lines

Quoted 5 lines

I know, and it caused some pain to write it (not only to find out how to solve it technically). We would need to pass the RAM memory region down to this freaky device, like we do to the i440fx for PAM purposes. But, well, that is not straightforward right now. Better ideas welcome.

Could we make it a child<> of i440FX, and have i440FX pass it the MemoryRegion directly?

Quoted 2 lines

It means we'll need to redo the glue for new chipsets, but it should be just a few lines.

Hmm... not really nice. It is rather a child of the APIC than of the chipset IMHO. Moving it over would also mean establishing logical link to the APIC from there.

It is really an ugly beast...

Quoted 4 lines · Jan Kiszka

Quoted 4 lines

Quoted 3 lines

We need to patch the causing instruction, so we have to know where it starts. Or what do you mean?

Quoted 4 lines

Quoted 2 lines

Just don't deal with this at all, no one runs on kernels without kernel irqchip.

Not true for upstream,

It was introduced in 2.6.24? Is anyone running anything earlier?

I hope that qemu-1.1 will default to kvm irqchip.

In fact we should
start thinking about deprecating apic-less kvm (in the kernel).

Quoted 4 lines

and not a design goal of this approach,
specifically when considering that it also works with TCG. Would be a pity to lose this generality.

It doesn't really speed up tcg, does it?

Quoted 13 lines

Quoted 12 lines

Quoted 8 lines

Quoted 5 lines

I'm not sure if the ABI guarantees anything about %rip.

That's indeed a point. It's likely coupled to the emulator's internals and when it calls out to user space for MMIO write. How to deal with it?

One way is to verify that it worked this way at least N versions back, and then retro-doc it. The downside is that it reduces our flexibility in the future, but I think that's a small downside.

Quoted 2 lines

It need not reduce our flexibility, we just need to signal to user space when our behaviour changes again.

This means that if this code detects that rip is no longer accurate using this signal, it has to disable itself. That's not something we want, I think.

Quoted 11 lines

Quoted 7 lines

Quoted 5 lines

I'm not sure if Windows has this properly set up for the UP HAL. I rather think this was a bug in the original implementation. The ROM uses 0 as CPU index in UP mode unconditionally, so should we in QEMU.

I mean just check kpcr.self.

Yes, clear, but that means that Windows must have initialized FS.base to point to the KPCR also in UP mode. Is that really the case? E.g. when ACPI is off?!

No idea.

Quoted 2 lines

I wonder if that explains the reported bug of qemu-kvm with -no-acpi and in-kernel irqchip...

acpi-less smp? it exists but rarely used. It could explain the problem, yes.

Quoted 13 lines

Quoted 12 lines

Quoted 6 lines

I know, and it caused some pain to write it (not only to find out how to solve it technically). We would need to pass the RAM memory region down to this freaky device, like we do to the i440fx for PAM purposes. But, well, that is not straightforward right now. Better ideas welcome.

Could we make it a child<> of i440FX, and have i440FX pass it the MemoryRegion directly?

It means we'll need to redo the glue for new chipsets, but it should be just a few lines.

Quoted 3 lines

Hmm... not really nice. It is rather a child of the APIC than of the chipset IMHO. Moving it over would also mean establishing logical link to the APIC from there.

Clearly it's involved with the 440fx as well, as it has the magical ability to turn ROM into RAM.

Quoted 1 line

I mean just check kpcr.self.

Yes, clear, but that means that Windows must have initialized FS.base to point to the KPCR also in UP mode. Is that really the case? E.g. when ACPI is off?! I wonder if that explains the reported bug of qemu-kvm with -no-acpi and in-kernel irqchip...

Quoted 6 lines · Jan Kiszka

Quoted 1 line

I mean just check kpcr.self.

Yes, clear, but that means that Windows must have initialized FS.base to point to the KPCR also in UP mode. Is that really the case? E.g. when ACPI is off?! I wonder if that explains the reported bug of qemu-kvm with -no-acpi and in-kernel irqchip...

Yes, it does. It's used by some fast-path kernel APIs, and indeed the canonical way to find the KPCR base from ring 0 is to look at FS:[1Ch].