Re: [Xen-devel] [PATCH v4 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 19.05.16 at 11:05, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> A new HVMOP - HVMOP_map_mem_type_to_ioreq_server, is added to
> let one ioreq server claim/disclaim its responsibility for the
> handling of guest pages with p2m type p2m_ioreq_server. Users
> of this HVMOP can specify which kind of operation is supposed
> to be emulated in a parameter named flags. Currently, this HVMOP
> only support the emulation of write operations. And it can be
> easily extended to support the emulation of read ones if an
> ioreq server has such requirement in the future.

Didn't we determine that this isn't as easy as everyone first thought?

> @@ -178,8 +179,33 @@ static int hvmemul_do_io(
>          break;
>      case X86EMUL_UNHANDLEABLE:
>      {
> -        struct hvm_ioreq_server *s =
> -            hvm_select_ioreq_server(curr->domain, &p);
> +        struct hvm_ioreq_server *s;
> +        p2m_type_t p2mt;
> +
> +        if ( is_mmio )
> +        {
> +            unsigned long gmfn = paddr_to_pfn(addr);
> +
> +            (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
> +
> +            if ( p2mt == p2m_ioreq_server )
> +            {
> +                unsigned long flags;
> +
> +                s = p2m_get_ioreq_server(currd, &flags);
> +
> +                if ( dir == IOREQ_WRITE &&
> +                     !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS) )

Shouldn't this be 

                if ( dir != IOREQ_WRITE ||
                     !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS) )
                    s = NULL;

in which case the question is whether you wouldn't better avoid
calling p2m_get_ioreq_server() in the first place when
dir != IOREQ_WRITE.

> +                    s = NULL;
> +            }
> +            else
> +                s = hvm_select_ioreq_server(currd, &p);
> +        }
> +        else
> +        {
> +            p2mt = p2m_invalid;

What is this needed for? In fact it looks like the variable decaration
could move into the next inner scope (alongside gmfn, which is
questionable to be a local variable anyway, considering that is gets
used just once).

> @@ -914,6 +916,45 @@ int hvm_unmap_io_range_from_ioreq_server(struct domain 
> *d, ioservid_t id,
>      return rc;
>  }
>  
> +int hvm_map_mem_type_to_ioreq_server(struct domain *d, ioservid_t id,
> +                                     uint16_t type, uint32_t flags)

I see no reason why both can't be unsigned int.

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -132,6 +132,12 @@ static void ept_p2m_type_to_flags(struct p2m_domain 
> *p2m, ept_entry_t *entry,
>              entry->r = entry->w = entry->x = 1;
>              entry->a = entry->d = !!cpu_has_vmx_ept_ad;
>              break;
> +        case p2m_ioreq_server:
> +            entry->r = entry->x = 1;

Why x?

> @@ -94,8 +96,16 @@ static unsigned long p2m_type_to_flags(p2m_type_t t, mfn_t 
> mfn,
>      default:
>          return flags | _PAGE_NX_BIT;
>      case p2m_grant_map_ro:
> -    case p2m_ioreq_server:
>          return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
> +    case p2m_ioreq_server:
> +    {
> +        flags |= P2M_BASE_FLAGS | _PAGE_RW;
> +
> +        if ( p2m->ioreq.flags & P2M_IOREQ_HANDLE_WRITE_ACCESS )
> +            return flags & ~_PAGE_RW;
> +        else
> +            return flags;
> +    }

Same here (for the missing _PAGE_NX) plus no need for braces.

> @@ -289,6 +291,74 @@ void p2m_memory_type_changed(struct domain *d)
>      }
>  }
>  
> +int p2m_set_ioreq_server(struct domain *d,
> +                         unsigned long flags,

Why "long" and not just "int"?

> +                         struct hvm_ioreq_server *s)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    int rc;
> +
> +    spin_lock(&p2m->ioreq.lock);
> +
> +    if ( flags == 0 )
> +    {
> +        rc = -EINVAL;
> +        if ( p2m->ioreq.server != s )
> +            goto out;
> +
> +        /* Unmap ioreq server from p2m type by passing flags with 0. */
> +        p2m->ioreq.server = NULL;
> +        p2m->ioreq.flags = 0;
> +    }

What does "passing" refer to in the comment?

> +    else
> +    {
> +        rc = -EBUSY;
> +        if ( p2m->ioreq.server != NULL )
> +            goto out;
> +
> +        p2m->ioreq.server = s;
> +        p2m->ioreq.flags = flags;
> +    }
> +
> +    rc = 0;
> +
> + out:
> +    spin_unlock(&p2m->ioreq.lock);
> +
> +    return rc;
> +}
> +
> +struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
> +                                              unsigned long *flags)

Again  why "long" and not just "int"?

> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct hvm_ioreq_server *s;
> +
> +    spin_lock(&p2m->ioreq.lock);
> +
> +    s = p2m->ioreq.server;
> +    *flags = p2m->ioreq.flags;
> +
> +    spin_unlock(&p2m->ioreq.lock);
> +    return s;
> +}

Locking is somewhat strange here: You protect against the "set"
counterpart altering state while you retrieve it, but you don't
protect against the returned data becoming stale by the time
the caller can consume it. Is that not a problem? (The most
concerning case would seem to be a race of hvmop_set_mem_type()
with de-registration of the type.)

> +void p2m_destroy_ioreq_server(struct domain *d,
> +                              struct hvm_ioreq_server *s)

const

> @@ -336,6 +336,24 @@ struct p2m_domain {
>          struct ept_data ept;
>          /* NPT-equivalent structure could be added here. */
>      };
> +
> +    struct {
> +        spinlock_t lock;
> +        /*
> +         * ioreq server who's responsible for the emulation of
> +         * gfns with specific p2m type(for now, p2m_ioreq_server).
> +         */
> +        struct hvm_ioreq_server *server;
> +        /*
> +         * flags specifies whether read, write or both operations
> +         * are to be emulated by an ioreq server.
> +         */
> +        unsigned int flags;
> +
> +#define P2M_IOREQ_HANDLE_WRITE_ACCESS HVMOP_IOREQ_MEM_ACCESS_WRITE
> +#define P2M_IOREQ_HANDLE_READ_ACCESS  HVMOP_IOREQ_MEM_ACCESS_READ

Is there anything wrong with using the HVMOP_* values directly?

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -89,7 +89,9 @@ typedef enum {
>      HVMMEM_unused,             /* Placeholder; setting memory to this type
>                                    will fail for code after 4.7.0 */
>  #endif
> -    HVMMEM_ioreq_server
> +    HVMMEM_ioreq_server        /* Memory type claimed by an ioreq server; 
> type
> +                                  changes to this value are only allowed 
> after
> +                                  an ioreq server has claimed its ownership 
> */

Missing trailing full stop.

> @@ -383,6 +385,37 @@ struct xen_hvm_set_ioreq_server_state {
>  typedef struct xen_hvm_set_ioreq_server_state 
> xen_hvm_set_ioreq_server_state_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_set_ioreq_server_state_t);
>  
> +/*
> + * HVMOP_map_mem_type_to_ioreq_server : map or unmap the IOREQ Server <id>
> + *                                      to specific memroy type <type>
> + *                                      for specific accesses <flags>
> + *
> + * For now, flags only accept the value of HVMOP_IOREQ_MEM_ACCESS_WRITE,
> + * which means 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 future.
> + */
> +#define HVMOP_map_mem_type_to_ioreq_server 26
> +struct xen_hvm_map_mem_type_to_ioreq_server {
> +    domid_t domid;      /* IN - domain to be serviced */
> +    ioservid_t id;      /* IN - ioreq server id */
> +    uint16_t type;      /* IN - memory type */
> +    uint16_t pad;

This field does not appear to get checked in the handler.

> +    uint32_t flags;     /* IN - types of accesses to be forwarded to the
> +                           ioreq server. flags with 0 means to unmap the
> +                           ioreq server */
> +#define _HVMOP_IOREQ_MEM_ACCESS_READ 0
> +#define HVMOP_IOREQ_MEM_ACCESS_READ \
> +    (1u << _HVMOP_IOREQ_MEM_ACCESS_READ)
> +
> +#define _HVMOP_IOREQ_MEM_ACCESS_WRITE 1
> +#define HVMOP_IOREQ_MEM_ACCESS_WRITE \
> +    (1u << _HVMOP_IOREQ_MEM_ACCESS_WRITE)

Is there any use for these _HVMOP_* values? The more that they
violate standard C name space rules?

Jan

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