Re: [PATCH v14 12/17] pvh: Use PV handlers for cpuid, and IO

[George Dunlap <george.dunlap@eu.citrix.com>]

On 04/11/13 12:15, George Dunlap wrote:
> For cpuid, this means putting hooks into the vmexit handler to call it instead
> of the hvm one.
>
> For IO, this now means putting a hook into the emulation code to call the PV
> guest_io_{read,write} functions.
>
> NB at this point this won't do the full "copy and execute on the stack
> with full GPRs" work-around; this may need to be sorted out for dom0 to allow
> these instructions to happen in guest context.
>
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> Signed-off-by: Mukesh Rathor <mukesh.rathor@oracle.com>
> ---
> v14-hvmio:
>   - Use HVM path to emulate IO, calling into PV just for the final IO
>   - Don't support forced invalid ops any more (so we can avoid PV emulation altogether)
> v13:
>   - Remove unnecessary privilege check in PIO path, update related comment
>   - Move ? and : to end of line rather than beginning, as requested
>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Keir Fraser <keir@xen.org>
> ---
>   xen/arch/x86/hvm/emulate.c      |   75 ++++++++++++++++++++++++++++++++++-----
>   xen/arch/x86/hvm/vmx/vmx.c      |    3 +-
>   xen/arch/x86/traps.c            |    6 ++--
>   xen/include/asm-x86/processor.h |    2 ++
>   xen/include/asm-x86/traps.h     |    8 +++++
>   5 files changed, 81 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
> index a41eaa1..0d767c2 100644
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -16,14 +16,14 @@
>   #include <xen/paging.h>
>   #include <xen/trace.h>
>   #include <asm/event.h>
> +#include <asm/traps.h>
>   #include <asm/xstate.h>
>   #include <asm/hvm/emulate.h>
>   #include <asm/hvm/hvm.h>
>   #include <asm/hvm/trace.h>
>   #include <asm/hvm/support.h>
>   
> -static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
> -{
> +static void trace_io_assist(int is_mmio, int dir, int data_valid, paddr_t addr, unsigned int data) {
>       unsigned int size, event;
>       unsigned char buffer[12];
>   
> @@ -31,22 +31,28 @@ static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
>           return;
>   
>       if ( is_mmio )
> -        event = p->dir ? TRC_HVM_IOMEM_READ : TRC_HVM_IOMEM_WRITE;
> +        event = dir ? TRC_HVM_IOMEM_READ : TRC_HVM_IOMEM_WRITE;
>       else
> -        event = p->dir ? TRC_HVM_IOPORT_READ : TRC_HVM_IOPORT_WRITE;
> +        event = dir ? TRC_HVM_IOPORT_READ : TRC_HVM_IOPORT_WRITE;
>   
> -    *(uint64_t *)buffer = p->addr;
> -    size = (p->addr != (u32)p->addr) ? 8 : 4;
> +    *(uint64_t *)buffer = addr;
> +    size = (addr != (u32)addr) ? 8 : 4;
>       if ( size == 8 )
>           event |= TRC_64_FLAG;
>   
> -    if ( !p->data_is_ptr )
> +    if ( data_valid )
>       {
> -        *(uint32_t *)&buffer[size] = p->data;
> +        *(uint32_t *)&buffer[size] = data;
>           size += 4;
>       }
>   
>       trace_var(event, 0/*!cycles*/, size, buffer);
> +
> +}
> +
> +static void hvmtrace_io_assist(int is_mmio, ioreq_t *p)
> +{
> +    trace_io_assist(is_mmio, p->dir, !p->data_is_ptr, p->addr, p->data);
>   }
>   
>   static int hvmemul_do_io(
> @@ -140,6 +146,9 @@ static int hvmemul_do_io(
>           }
>       }
>   
> +    if ( is_pvh_vcpu(curr) )
> +        ASSERT(vio->io_state == HVMIO_none);
> +
>       switch ( vio->io_state )
>       {
>       case HVMIO_none:
> @@ -284,11 +293,59 @@ static int hvmemul_do_io(
>       return X86EMUL_OKAY;
>   }
>   
> +static int pvhemul_do_pio(
> +    unsigned long port, int size, paddr_t ram_gpa, int dir, void *p_data)
> +{
> +    paddr_t value = ram_gpa;
> +    struct vcpu *curr = current;
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +
> +    /*
> +     * Weird-sized accesses have undefined behaviour: we discard writes
> +     * and read all-ones.
> +     */
> +    if ( unlikely((size > sizeof(long)) || (size & (size - 1))) )
> +    {
> +        gdprintk(XENLOG_WARNING, "bad mmio size %d\n", size);
> +        ASSERT(p_data != NULL); /* cannot happen with a REP prefix */
> +        if ( dir == IOREQ_READ )
> +            memset(p_data, ~0, size);
> +        return X86EMUL_UNHANDLEABLE;
> +    }
> +
> +    if ( dir == IOREQ_WRITE ) {
> +        if ( (p_data != NULL) )
> +        {
> +            memcpy(&value, p_data, size);
> +            p_data = NULL;
> +        }
> +
> +    if ( dir == IOREQ_WRITE )
> +        trace_io_assist(0, dir, 1, port, value);
> +
> +        guest_io_write(port, size, value, curr, regs);
> +    }
> +    else
> +    {
> +        value = guest_io_read(port, size, curr, regs);
> +        trace_io_assist(0, dir, 1, port, value);
> +        if ( (p_data != NULL) )
> +            memcpy(p_data, &value, size);
> +        memcpy(&regs->eax, &value, size);
> +    }
> +
> +    return X86EMUL_OKAY;
> +}
> +
> +
>   int hvmemul_do_pio(
>       unsigned long port, unsigned long *reps, int size,
>       paddr_t ram_gpa, int dir, int df, void *p_data)
>   {
> -    return hvmemul_do_io(0, port, reps, size, ram_gpa, dir, df, p_data);
> +    return is_hvm_vcpu(current) ?
> +        hvmemul_do_io(0, port, reps, size, ram_gpa, dir, df, p_data) :
> +        pvhemul_do_pio(port, size, ram_gpa, dir, p_data);
> +
>   }
>   
>   static int hvmemul_do_mmio(
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 94e9e21..5d1e367 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -56,6 +56,7 @@
>   #include <asm/apic.h>
>   #include <asm/hvm/nestedhvm.h>
>   #include <asm/event.h>
> +#include <asm/traps.h>
>   
>   enum handler_return { HNDL_done, HNDL_unhandled, HNDL_exception_raised };
>   
> @@ -2694,8 +2695,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
>           break;
>       }
>       case EXIT_REASON_CPUID:
> +        is_pvh_vcpu(v) ? pv_cpuid(regs) : vmx_do_cpuid(regs);
>           update_guest_eip(); /* Safe: CPUID */
> -        vmx_do_cpuid(regs);
>           break;
>       case EXIT_REASON_HLT:
>           update_guest_eip(); /* Safe: HLT */
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index edb7a6a..6c278bc 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -729,7 +729,7 @@ int cpuid_hypervisor_leaves( uint32_t idx, uint32_t sub_idx,
>       return 1;
>   }
>   
> -static void pv_cpuid(struct cpu_user_regs *regs)
> +void pv_cpuid(struct cpu_user_regs *regs)
>   {
>       uint32_t a, b, c, d;
>   
> @@ -1681,7 +1681,7 @@ static int pci_cfg_ok(struct domain *d, int write, int size)
>       return 1;
>   }
>   
> -static uint32_t guest_io_read(
> +uint32_t guest_io_read(
>       unsigned int port, unsigned int bytes,
>       struct vcpu *v, struct cpu_user_regs *regs)
>   {
> @@ -1748,7 +1748,7 @@ static uint32_t guest_io_read(
>       return data;
>   }
>   
> -static void guest_io_write(
> +void guest_io_write(
>       unsigned int port, unsigned int bytes, uint32_t data,
>       struct vcpu *v, struct cpu_user_regs *regs)
>   {
> diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
> index 893afa3..551036d 100644
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -567,6 +567,8 @@ void microcode_set_module(unsigned int);
>   int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
>   int microcode_resume_cpu(int cpu);
>   
> +void pv_cpuid(struct cpu_user_regs *regs);
> +
>   #endif /* !__ASSEMBLY__ */
>   
>   #endif /* __ASM_X86_PROCESSOR_H */
> diff --git a/xen/include/asm-x86/traps.h b/xen/include/asm-x86/traps.h
> index 82cbcee..a26b318 100644
> --- a/xen/include/asm-x86/traps.h
> +++ b/xen/include/asm-x86/traps.h
> @@ -49,4 +49,12 @@ extern int guest_has_trap_callback(struct domain *d, uint16_t vcpuid,
>   extern int send_guest_trap(struct domain *d, uint16_t vcpuid,
>   				unsigned int trap_nr);
>   
> +uint32_t guest_io_read(
> +    unsigned int port, unsigned int bytes,
> +    struct vcpu *v, struct cpu_user_regs *regs);
> +void guest_io_write(
> +    unsigned int port, unsigned int bytes, uint32_t data,
> +    struct vcpu *v, struct cpu_user_regs *regs);
> +
> +
>   #endif /* ASM_TRAP_H */