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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.07.16 at 11:02, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> @@ -178,8 +179,34 @@ 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;
> +
> +        if ( is_mmio )
> +        {
> +            unsigned long gmfn = paddr_to_pfn(addr);
> +            p2m_type_t p2mt;
> +
> +            (void) get_gfn_query_unlocked(currd, gmfn, &p2mt);
> +
> +            if ( p2mt == p2m_ioreq_server )
> +            {
> +                unsigned int flags;
> +
> +                if ( dir != IOREQ_WRITE )
> +                    s = NULL;
> +                else
> +                {
> +                    s = p2m_get_ioreq_server(currd, &flags);
> +
> +                    if ( !(flags & P2M_IOREQ_HANDLE_WRITE_ACCESS) )
> +                        s = NULL;
> +                }
> +            }
> +            else
> +                s = hvm_select_ioreq_server(currd, &p);
> +        }
> +        else
> +            s = hvm_select_ioreq_server(currd, &p);

Wouldn't it both be more natural and make the logic even easier
to follow if s got set to NULL up front, all the "else"-s dropped,
and a simple

        if ( !s )
            s = hvm_select_ioreq_server(currd, &p);

be done in the end?

> @@ -5447,6 +5452,21 @@ static int hvmop_set_mem_type(
>      if ( !is_hvm_domain(d) )
>          goto out;
>  
> +    if ( a.hvmmem_type == HVMMEM_ioreq_server )
> +    {
> +        unsigned int flags;
> +        struct hvm_ioreq_server *s;
> +
> +        /* HVMMEM_ioreq_server is only supported for HAP enabled hvm. */
> +        if ( !hap_enabled(d) )
> +            goto out;
> +
> +        /* Do not change to HVMMEM_ioreq_server if no ioreq server mapped. */
> +        s = p2m_get_ioreq_server(d, &flags);
> +        if ( s == NULL )
> +            goto out;

Either drop s as an intermediate variable altogether (preferred), or
constify it properly.

> +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;
> +
> +    /* For now, only HVMMEM_ioreq_server is supported. */
> +    if ( type != HVMMEM_ioreq_server )
> +        return -EINVAL;
> +
> +    /* For now, only write emulation is supported. */
> +    if ( flags & ~(XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE) )
> +        return -EINVAL;
> +
> +    spin_lock(&d->arch.hvm_domain.ioreq_server.lock);

This lock did get converted to a recursive one a little while back.

> +    rc = -ENOENT;
> +    list_for_each_entry ( s,
> +                          &d->arch.hvm_domain.ioreq_server.list,
> +                          list_entry )
> +    {
> +        if ( s == d->arch.hvm_domain.default_ioreq_server )
> +            continue;
> +
> +        if ( s->id == id )
> +        {
> +            rc = p2m_set_ioreq_server(d, flags, s);
> +            if ( rc == 0 )
> +                dprintk(XENLOG_DEBUG, "%u %s type HVMMEM_ioreq_server.\n",
> +                         s->id, (flags != 0) ? "mapped to" : "unmapped 
> from");

Is this really useful?

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -132,6 +132,13 @@ 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 = 1;
> +            entry->w = !(p2m->ioreq.flags & P2M_IOREQ_HANDLE_WRITE_ACCESS);
> +            entry->x = 0;
> +            entry->a = !!cpu_has_vmx_ept_ad;
> +            entry->d = entry->w && cpu_has_vmx_ept_ad;

For self-consistency, could this become

            entry->d = entry->w && entry->a;

?

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -3225,8 +3225,7 @@ static int sh_page_fault(struct vcpu *v,
>      }
>  
>      /* Need to hand off device-model MMIO to the device model */
> -    if ( p2mt == p2m_mmio_dm
> -         || (p2mt == p2m_ioreq_server && ft == ft_demand_write) )
> +    if ( p2mt == p2m_mmio_dm )

Could you remind me again what the code being removed here gets
replaced by, or why it doesn't need any replacement?

> @@ -336,6 +336,23 @@ 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 XEN_HVMOP_IOREQ_MEM_ACCESS_WRITE
> +#define P2M_IOREQ_HANDLE_READ_ACCESS  XEN_HVMOP_IOREQ_MEM_ACCESS_READ

I think I did say so on a previous iteration already: I can't see the
value of these two defines, or in fact I can see these being actively
dangerous: The rest of your patch assume that each pair shares
their values (as there's no translation between them, but also no
BUILD_BUG_ON() ensuring they're identical).

> --- 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. 
> */

Wouldn't it be worth also noting in the comment that only changes
to/from rw are permitted?

Jan

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