[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

RE: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it common



> -----Original Message-----
> From: Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>
> Sent: 15 October 2020 17:44
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>; Paul Durrant 
> <paul@xxxxxxx>; Jan Beulich
> <jbeulich@xxxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Roger Pau 
> Monné
> <roger.pau@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini 
> <sstabellini@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>; Julien Grall <julien.grall@xxxxxxx>
> Subject: [PATCH V2 01/23] x86/ioreq: Prepare IOREQ feature for making it 
> common
> 
> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> 
> As a lot of x86 code can be re-used on Arm later on, this
> patch makes some preparation to x86/hvm/ioreq.c before moving
> to the common code. This way we will get a verbatim copy for
> a code movement in subsequent patch (arch/x86/hvm/ioreq.c
> will be *just* renamed to common/ioreq).
> 
> This patch does the following:
> 1. Introduce *inline* arch_hvm_ioreq_init(), arch_hvm_ioreq_destroy(),
>    arch_hvm_io_completion(), arch_hvm_destroy_ioreq_server() and
>    hvm_ioreq_server_get_type_addr() to abstract arch specific materials.
> 2  Make hvm_map_mem_type_to_ioreq_server() *inline*. It is not going
>    to be called from the common code.
> 3. Make get_ioreq_server() global. It is going to be called from
>    a few places.
> 4. Add IOREQ_STATUS_* #define-s and update candidates for moving.
> 5. Re-order #include-s alphabetically.
> 
> This support is going to be used on Arm to be able run device
> emulator outside of Xen hypervisor.
> 
> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
> CC: Julien Grall <julien.grall@xxxxxxx>
> 
> ---
> Please note, this is a split/cleanup/hardening of Julien's PoC:
> "Add support for Guest IO forwarding to a device emulator"
> 
> Changes RFC -> V1:
>    - new patch, was split from:
>      "[RFC PATCH V1 01/12] hvm/ioreq: Make x86's IOREQ feature common"
>    - fold the check of p->type into hvm_get_ioreq_server_range_type()
>      and make it return success/failure
>    - remove relocate_portio_handler() call from arch_hvm_ioreq_destroy()
>      in arch/x86/hvm/ioreq.c
>    - introduce arch_hvm_destroy_ioreq_server()/arch_handle_hvm_io_completion()
> 
> Changes V1 -> V2:
>    - update patch description
>    - make arch functions inline and put them into arch header
>      to achieve a truly rename by the subsequent patch
>    - return void in arch_hvm_destroy_ioreq_server()
>    - return bool in arch_hvm_ioreq_destroy()
>    - bring relocate_portio_handler() back to arch_hvm_ioreq_destroy()
>    - rename IOREQ_IO* to IOREQ_STATUS*
>    - remove *handle* from arch_handle_hvm_io_completion()
>    - re-order #include-s alphabetically
>    - rename hvm_get_ioreq_server_range_type() to 
> hvm_ioreq_server_get_type_addr()
>      and add "const" to several arguments
> ---
>  xen/arch/x86/hvm/ioreq.c        | 153 +++++--------------------------------
>  xen/include/asm-x86/hvm/ioreq.h | 165 
> +++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 184 insertions(+), 134 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/ioreq.c b/xen/arch/x86/hvm/ioreq.c
> index 1cc27df..d3433d7 100644
> --- a/xen/arch/x86/hvm/ioreq.c
> +++ b/xen/arch/x86/hvm/ioreq.c
> @@ -1,5 +1,5 @@
>  /*
> - * hvm/io.c: hardware virtual machine I/O emulation
> + * ioreq.c: hardware virtual machine I/O emulation
>   *
>   * Copyright (c) 2016 Citrix Systems Inc.
>   *
> @@ -17,21 +17,18 @@
>   */
> 
>  #include <xen/ctype.h>
> +#include <xen/domain.h>
> +#include <xen/event.h>
>  #include <xen/init.h>
> +#include <xen/irq.h>
>  #include <xen/lib.h>
> -#include <xen/trace.h>
> +#include <xen/paging.h>
>  #include <xen/sched.h>
> -#include <xen/irq.h>
>  #include <xen/softirq.h>
> -#include <xen/domain.h>
> -#include <xen/event.h>
> -#include <xen/paging.h>
> +#include <xen/trace.h>
>  #include <xen/vpci.h>
> 
> -#include <asm/hvm/emulate.h>
> -#include <asm/hvm/hvm.h>
>  #include <asm/hvm/ioreq.h>
> -#include <asm/hvm/vmx/vmx.h>
> 
>  #include <public/hvm/ioreq.h>
>  #include <public/hvm/params.h>
> @@ -48,8 +45,8 @@ static void set_ioreq_server(struct domain *d, unsigned int 
> id,
>  #define GET_IOREQ_SERVER(d, id) \
>      (d)->arch.hvm.ioreq_server.server[id]
> 
> -static struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
> -                                                 unsigned int id)
> +struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
> +                                          unsigned int id)
>  {
>      if ( id >= MAX_NR_IOREQ_SERVERS )
>          return NULL;
> @@ -209,19 +206,8 @@ bool handle_hvm_io_completion(struct vcpu *v)
>          return handle_pio(vio->io_req.addr, vio->io_req.size,
>                            vio->io_req.dir);
> 
> -    case HVMIO_realmode_completion:
> -    {
> -        struct hvm_emulate_ctxt ctxt;
> -
> -        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> -        vmx_realmode_emulate_one(&ctxt);
> -        hvm_emulate_writeback(&ctxt);
> -
> -        break;
> -    }
>      default:
> -        ASSERT_UNREACHABLE();
> -        break;
> +        return arch_hvm_io_completion(io_completion);
>      }
> 
>      return true;
> @@ -855,7 +841,7 @@ int hvm_destroy_ioreq_server(struct domain *d, ioservid_t 
> id)
> 
>      domain_pause(d);
> 
> -    p2m_set_ioreq_server(d, 0, s);
> +    arch_hvm_destroy_ioreq_server(s);
> 
>      hvm_ioreq_server_disable(s);
> 
> @@ -1080,54 +1066,6 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain 
> *d, ioservid_t id,
>      return rc;
>  }
> 
> -/*
> - * Map or unmap an ioreq server to specific memory type. For now, only
> - * HVMMEM_ioreq_server is supported, and in the future new types can be
> - * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
> - * currently, only write operations are to be forwarded to an ioreq server.
> - * Support for the emulation of read operations can be added when an ioreq
> - * server has such requirement in the future.
> - */
> -int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
> -                                     uint32_t type, uint32_t flags)
> -{
> -    struct hvm_ioreq_server *s;
> -    int rc;
> -
> -    if ( type != HVMMEM_ioreq_server )
> -        return -EINVAL;
> -
> -    if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> -        return -EINVAL;
> -
> -    spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> -
> -    s = get_ioreq_server(d, id);
> -
> -    rc = -ENOENT;
> -    if ( !s )
> -        goto out;
> -
> -    rc = -EPERM;
> -    if ( s->emulator != current->domain )
> -        goto out;
> -
> -    rc = p2m_set_ioreq_server(d, flags, s);
> -
> - out:
> -    spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> -
> -    if ( rc == 0 && flags == 0 )
> -    {
> -        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> -
> -        if ( read_atomic(&p2m->ioreq.entry_count) )
> -            p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> -    }
> -
> -    return rc;
> -}
> -
>  int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
>                                 bool enabled)
>  {
> @@ -1215,7 +1153,7 @@ void hvm_destroy_all_ioreq_servers(struct domain *d)
>      struct hvm_ioreq_server *s;
>      unsigned int id;
> 
> -    if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> +    if ( !arch_hvm_ioreq_destroy(d) )
>          return;
> 
>      spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> @@ -1243,50 +1181,13 @@ struct hvm_ioreq_server 
> *hvm_select_ioreq_server(struct domain *d,
>                                                   ioreq_t *p)
>  {
>      struct hvm_ioreq_server *s;
> -    uint32_t cf8;
>      uint8_t type;
>      uint64_t addr;
>      unsigned int id;
> 
> -    if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
> +    if ( hvm_ioreq_server_get_type_addr(d, p, &type, &addr) )
>          return NULL;
> 
> -    cf8 = d->arch.hvm.pci_cf8;
> -
> -    if ( p->type == IOREQ_TYPE_PIO &&
> -         (p->addr & ~3) == 0xcfc &&
> -         CF8_ENABLED(cf8) )
> -    {
> -        uint32_t x86_fam;
> -        pci_sbdf_t sbdf;
> -        unsigned int reg;
> -
> -        reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
> -
> -        /* PCI config data cycle */
> -        type = XEN_DMOP_IO_RANGE_PCI;
> -        addr = ((uint64_t)sbdf.sbdf << 32) | reg;
> -        /* AMD extended configuration space access? */
> -        if ( CF8_ADDR_HI(cf8) &&
> -             d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
> -             (x86_fam = get_cpu_family(
> -                 d->arch.cpuid->basic.raw_fms, NULL, NULL)) >= 0x10 &&
> -             x86_fam < 0x17 )
> -        {
> -            uint64_t msr_val;
> -
> -            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
> -                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
> -                addr |= CF8_ADDR_HI(cf8);
> -        }
> -    }
> -    else
> -    {
> -        type = (p->type == IOREQ_TYPE_PIO) ?
> -                XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
> -        addr = p->addr;
> -    }
> -
>      FOR_EACH_IOREQ_SERVER(d, id, s)
>      {
>          struct rangeset *r;
> @@ -1351,7 +1252,7 @@ static int hvm_send_buffered_ioreq(struct 
> hvm_ioreq_server *s, ioreq_t *p)
>      pg = iorp->va;
> 
>      if ( !pg )
> -        return X86EMUL_UNHANDLEABLE;
> +        return IOREQ_STATUS_UNHANDLED;
> 
>      /*
>       * Return 0 for the cases we can't deal with:
> @@ -1381,7 +1282,7 @@ static int hvm_send_buffered_ioreq(struct 
> hvm_ioreq_server *s, ioreq_t *p)
>          break;
>      default:
>          gdprintk(XENLOG_WARNING, "unexpected ioreq size: %u\n", p->size);
> -        return X86EMUL_UNHANDLEABLE;
> +        return IOREQ_STATUS_UNHANDLED;
>      }
> 
>      spin_lock(&s->bufioreq_lock);
> @@ -1391,7 +1292,7 @@ static int hvm_send_buffered_ioreq(struct 
> hvm_ioreq_server *s, ioreq_t *p)
>      {
>          /* The queue is full: send the iopacket through the normal path. */
>          spin_unlock(&s->bufioreq_lock);
> -        return X86EMUL_UNHANDLEABLE;
> +        return IOREQ_STATUS_UNHANDLED;
>      }
> 
>      pg->buf_ioreq[pg->ptrs.write_pointer % IOREQ_BUFFER_SLOT_NUM] = bp;
> @@ -1422,7 +1323,7 @@ static int hvm_send_buffered_ioreq(struct 
> hvm_ioreq_server *s, ioreq_t *p)
>      notify_via_xen_event_channel(d, s->bufioreq_evtchn);
>      spin_unlock(&s->bufioreq_lock);
> 
> -    return X86EMUL_OKAY;
> +    return IOREQ_STATUS_HANDLED;
>  }
> 
>  int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t *proto_p,
> @@ -1438,7 +1339,7 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, ioreq_t 
> *proto_p,
>          return hvm_send_buffered_ioreq(s, proto_p);
> 
>      if ( unlikely(!vcpu_start_shutdown_deferral(curr)) )
> -        return X86EMUL_RETRY;
> +        return IOREQ_STATUS_RETRY;
> 
>      list_for_each_entry ( sv,
>                            &s->ioreq_vcpu_list,
> @@ -1478,11 +1379,11 @@ int hvm_send_ioreq(struct hvm_ioreq_server *s, 
> ioreq_t *proto_p,
>              notify_via_xen_event_channel(d, port);
> 
>              sv->pending = true;
> -            return X86EMUL_RETRY;
> +            return IOREQ_STATUS_RETRY;
>          }
>      }
> 
> -    return X86EMUL_UNHANDLEABLE;
> +    return IOREQ_STATUS_UNHANDLED;
>  }
> 
>  unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool buffered)
> @@ -1496,30 +1397,18 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool 
> buffered)
>          if ( !s->enabled )
>              continue;
> 
> -        if ( hvm_send_ioreq(s, p, buffered) == X86EMUL_UNHANDLEABLE )
> +        if ( hvm_send_ioreq(s, p, buffered) == IOREQ_STATUS_UNHANDLED )
>              failed++;
>      }
> 
>      return failed;
>  }
> 
> -static int hvm_access_cf8(
> -    int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> -{
> -    struct domain *d = current->domain;
> -
> -    if ( dir == IOREQ_WRITE && bytes == 4 )
> -        d->arch.hvm.pci_cf8 = *val;
> -
> -    /* We always need to fall through to the catch all emulator */
> -    return X86EMUL_UNHANDLEABLE;
> -}
> -
>  void hvm_ioreq_init(struct domain *d)
>  {
>      spin_lock_init(&d->arch.hvm.ioreq_server.lock);
> 
> -    register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
> +    arch_hvm_ioreq_init(d);
>  }
> 
>  /*
> diff --git a/xen/include/asm-x86/hvm/ioreq.h b/xen/include/asm-x86/hvm/ioreq.h
> index e2588e9..376e2ef 100644
> --- a/xen/include/asm-x86/hvm/ioreq.h
> +++ b/xen/include/asm-x86/hvm/ioreq.h
> @@ -19,6 +19,165 @@
>  #ifndef __ASM_X86_HVM_IOREQ_H__
>  #define __ASM_X86_HVM_IOREQ_H__
> 
> +#include <asm/hvm/emulate.h>
> +#include <asm/hvm/vmx/vmx.h>
> +
> +#include <public/hvm/params.h>
> +
> +struct hvm_ioreq_server *get_ioreq_server(const struct domain *d,
> +                                          unsigned int id);
> +
> +static inline bool arch_hvm_io_completion(enum hvm_io_completion 
> io_completion)
> +{
> +    switch ( io_completion )
> +    {
> +    case HVMIO_realmode_completion:
> +    {
> +        struct hvm_emulate_ctxt ctxt;
> +
> +        hvm_emulate_init_once(&ctxt, NULL, guest_cpu_user_regs());
> +        vmx_realmode_emulate_one(&ctxt);
> +        hvm_emulate_writeback(&ctxt);
> +
> +        break;
> +    }
> +
> +    default:
> +        ASSERT_UNREACHABLE();
> +        break;
> +    }
> +
> +    return true;
> +}
> +
> +/* Called when target domain is paused */
> +static inline void arch_hvm_destroy_ioreq_server(struct hvm_ioreq_server *s)
> +{
> +    p2m_set_ioreq_server(s->target, 0, s);
> +}
> +
> +/*
> + * Map or unmap an ioreq server to specific memory type. For now, only
> + * HVMMEM_ioreq_server is supported, and in the future new types can be
> + * introduced, e.g. HVMMEM_ioreq_serverX mapped to ioreq server X. And
> + * currently, only write operations are to be forwarded to an ioreq server.
> + * Support for the emulation of read operations can be added when an ioreq
> + * server has such requirement in the future.
> + */
> +static inline int hvm_map_mem_type_to_ioreq_server(struct domain *d,
> +                                                   ioservid_t id,
> +                                                   uint32_t type,
> +                                                   uint32_t flags)
> +{
> +    struct hvm_ioreq_server *s;
> +    int rc;
> +
> +    if ( type != HVMMEM_ioreq_server )
> +        return -EINVAL;
> +
> +    if ( flags & ~XEN_DMOP_IOREQ_MEM_ACCESS_WRITE )
> +        return -EINVAL;
> +
> +    spin_lock_recursive(&d->arch.hvm.ioreq_server.lock);
> +
> +    s = get_ioreq_server(d, id);
> +
> +    rc = -ENOENT;
> +    if ( !s )
> +        goto out;
> +
> +    rc = -EPERM;
> +    if ( s->emulator != current->domain )
> +        goto out;
> +
> +    rc = p2m_set_ioreq_server(d, flags, s);
> +
> + out:
> +    spin_unlock_recursive(&d->arch.hvm.ioreq_server.lock);
> +
> +    if ( rc == 0 && flags == 0 )
> +    {
> +        struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +        if ( read_atomic(&p2m->ioreq.entry_count) )
> +            p2m_change_entry_type_global(d, p2m_ioreq_server, p2m_ram_rw);
> +    }
> +
> +    return rc;
> +}
> +

The above doesn't really feel right to me. It's really an entry point into the 
ioreq server code and as such I think it ought to be left in the common code. I 
suggest replacing the p2m_set_ioreq_server() function with an arch specific 
function (also taking the type) which you can then implement here.

The rest of the patch looks ok.

  Paul

> +static inline int hvm_ioreq_server_get_type_addr(const struct domain *d,
> +                                                 const ioreq_t *p,
> +                                                 uint8_t *type,
> +                                                 uint64_t *addr)
> +{
> +    uint32_t cf8 = d->arch.hvm.pci_cf8;
> +
> +    if ( p->type != IOREQ_TYPE_COPY && p->type != IOREQ_TYPE_PIO )
> +        return -EINVAL;
> +
> +    if ( p->type == IOREQ_TYPE_PIO &&
> +         (p->addr & ~3) == 0xcfc &&
> +         CF8_ENABLED(cf8) )
> +    {
> +        uint32_t x86_fam;
> +        pci_sbdf_t sbdf;
> +        unsigned int reg;
> +
> +        reg = hvm_pci_decode_addr(cf8, p->addr, &sbdf);
> +
> +        /* PCI config data cycle */
> +        *type = XEN_DMOP_IO_RANGE_PCI;
> +        *addr = ((uint64_t)sbdf.sbdf << 32) | reg;
> +        /* AMD extended configuration space access? */
> +        if ( CF8_ADDR_HI(cf8) &&
> +             d->arch.cpuid->x86_vendor == X86_VENDOR_AMD &&
> +             (x86_fam = get_cpu_family(
> +                 d->arch.cpuid->basic.raw_fms, NULL, NULL)) >= 0x10 &&
> +             x86_fam < 0x17 )
> +        {
> +            uint64_t msr_val;
> +
> +            if ( !rdmsr_safe(MSR_AMD64_NB_CFG, msr_val) &&
> +                 (msr_val & (1ULL << AMD64_NB_CFG_CF8_EXT_ENABLE_BIT)) )
> +                *addr |= CF8_ADDR_HI(cf8);
> +        }
> +    }
> +    else
> +    {
> +        *type = (p->type == IOREQ_TYPE_PIO) ?
> +                 XEN_DMOP_IO_RANGE_PORT : XEN_DMOP_IO_RANGE_MEMORY;
> +        *addr = p->addr;
> +    }
> +
> +    return 0;
> +}
> +
> +static inline int hvm_access_cf8(
> +    int dir, unsigned int port, unsigned int bytes, uint32_t *val)
> +{
> +    struct domain *d = current->domain;
> +
> +    if ( dir == IOREQ_WRITE && bytes == 4 )
> +        d->arch.hvm.pci_cf8 = *val;
> +
> +    /* We always need to fall through to the catch all emulator */
> +    return X86EMUL_UNHANDLEABLE;
> +}
> +
> +static inline void arch_hvm_ioreq_init(struct domain *d)
> +{
> +    register_portio_handler(d, 0xcf8, 4, hvm_access_cf8);
> +}
> +
> +static inline bool arch_hvm_ioreq_destroy(struct domain *d)
> +{
> +    if ( !relocate_portio_handler(d, 0xcf8, 0xcf8, 4) )
> +        return false;
> +
> +    return true;
> +}
> +
>  bool hvm_io_pending(struct vcpu *v);
>  bool handle_hvm_io_completion(struct vcpu *v);
>  bool is_ioreq_server_page(struct domain *d, const struct page_info *page);
> @@ -38,8 +197,6 @@ int hvm_map_io_range_to_ioreq_server(struct domain *d, 
> ioservid_t id,
>  int hvm_unmap_io_range_from_ioreq_server(struct domain *d, ioservid_t id,
>                                           uint32_t type, uint64_t start,
>                                           uint64_t end);
> -int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
> -                                     uint32_t type, uint32_t flags);
>  int hvm_set_ioreq_server_state(struct domain *d, ioservid_t id,
>                                 bool enabled);
> 
> @@ -55,6 +212,10 @@ unsigned int hvm_broadcast_ioreq(ioreq_t *p, bool 
> buffered);
> 
>  void hvm_ioreq_init(struct domain *d);
> 
> +#define IOREQ_STATUS_HANDLED     X86EMUL_OKAY
> +#define IOREQ_STATUS_UNHANDLED   X86EMUL_UNHANDLEABLE
> +#define IOREQ_STATUS_RETRY       X86EMUL_RETRY
> +
>  #endif /* __ASM_X86_HVM_IOREQ_H__ */
> 
>  /*
> --
> 2.7.4





 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.