Re: [Qemu-devel] [PATCH v2 2/5] target-i386: Add Intel HAX files

[Paolo Bonzini]

On 11/11/2016 12:28, Vincent Palatin wrote:
> +
> +    memcpy(env->xmm_regs, fpu.mmx_1, sizeof(fpu.mmx_1));
> +    memcpy((ZMMReg *) (env->xmm_regs) + 8, fpu.mmx_2, sizeof(fpu.mmx_2));

HAX will only support SSE (128-bit) registers, while env->xmm_regs
supports AVX512 (512-bit) so you have to copy registers one by one.

Is there documentation for HAX?  In particular I'm curious as to what
the CPUID information looks like in the guest, and whether there are
ioctls to change it.

> +
> +static int hax_handle_fastmmio(CPUArchState *env, struct hax_fastmmio *hft)
> +{
> +    uint64_t buf = 0;
> +    /*
> +     * With fast MMIO, QEMU need not sync vCPU state with HAXM
> +     * driver because it will only invoke MMIO handler
> +     * However, some MMIO operations utilize virtual address like qemu_pipe
> +     * Thus we need to sync the CR0, CR3 and CR4 so that QEMU
> +     * can translate the guest virtual address to guest physical
> +     * address
> +     */
> +    env->cr[0] = hft->_cr0;
> +    env->cr[2] = hft->_cr2;
> +    env->cr[3] = hft->_cr3;
> +    env->cr[4] = hft->_cr4;

These seem to apply only to some parts of the Android emulator that are
not upstream, so you can remove them.

> +    buf = hft->value;
> +
> +    cpu_physical_memory_rw(hft->gpa, (uint8_t *) &buf, hft->size,
> +                           hft->direction);
> +    if (hft->direction == 0) {
> +        hft->value = buf;
> +    }

No need to use "buf", you can use &hft->value directly.

> +    return 0;
> +}
> +
> +static int hax_handle_io(CPUArchState *env, uint32_t df, uint16_t port,
> +                         int direction, int size, int count, void *buffer)
> +{
> +    uint8_t *ptr;
> +    int i;
> +
> +    if (!df) {
> +        ptr = (uint8_t *) buffer;
> +    } else {
> +        ptr = buffer + size * count - size;
> +    }
> +    for (i = 0; i < count; i++) {
> +        if (direction == HAX_EXIT_IO_IN) {
> +            switch (size) {
> +            case 1:
> +                stb_p(ptr, cpu_inb(port));
> +                break;
> +            case 2:
> +                stw_p(ptr, cpu_inw(port));
> +                break;
> +            case 4:
> +                stl_p(ptr, cpu_inl(port));
> +                break;
> +            }
> +        } else {
> +            switch (size) {
> +            case 1:
> +                cpu_outb(port, ldub_p(ptr));
> +                break;
> +            case 2:
> +                cpu_outw(port, lduw_p(ptr));
> +                break;
> +            case 4:
> +                cpu_outl(port, ldl_p(ptr));
> +                break;
> +            }
> +        }

The whole "if" can be replaced by

    MemTxAttrs = { 0 };
    ...

        address_space_rw(&address_space_io, port, attrs,
                         ptr, size, direction == HAX_EXIT_IO_OUT);

Thanks,

Paolo

> +        if (!df) {
> +            ptr += size;
> +        } else {
> +            ptr -= size;
> +        }
> +    }
> +
> +    return 0;
> +}
> +