Re: [Xen-devel] [PATCH v9 2/5] x86/ioreq server: Add DMOP to map guest ram with p2m_ioreq_server to an ioreq server.

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.03.17 at 03:52, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> ---
>  xen/arch/x86/hvm/dm.c            | 37 ++++++++++++++++++--
>  xen/arch/x86/hvm/emulate.c       | 65 ++++++++++++++++++++++++++++++++---
>  xen/arch/x86/hvm/ioreq.c         | 38 +++++++++++++++++++++
>  xen/arch/x86/mm/hap/nested_hap.c |  2 +-
>  xen/arch/x86/mm/p2m-ept.c        |  8 ++++-
>  xen/arch/x86/mm/p2m-pt.c         | 19 +++++++----
>  xen/arch/x86/mm/p2m.c            | 74 
> ++++++++++++++++++++++++++++++++++++++++
>  xen/arch/x86/mm/shadow/multi.c   |  3 +-
>  xen/include/asm-x86/hvm/ioreq.h  |  2 ++
>  xen/include/asm-x86/p2m.h        | 26 ++++++++++++--
>  xen/include/public/hvm/dm_op.h   | 28 +++++++++++++++
>  xen/include/public/hvm/hvm_op.h  |  8 ++++-
>  12 files changed, 290 insertions(+), 20 deletions(-)

Btw., isn't there a libdevicemodel wrapper missing here for this new
sub-op?

> @@ -177,8 +178,64 @@ static int hvmemul_do_io(
>          break;
>      case X86EMUL_UNHANDLEABLE:
>      {
> -        struct hvm_ioreq_server *s =
> -            hvm_select_ioreq_server(curr->domain, &p);
> +        /*
> +         * Xen isn't emulating the instruction internally, so see if
> +         * there's an ioreq server that can handle it. Rules:
> +         *
> +         * - PIO and "normal" MMIO run through hvm_select_ioreq_server()
> +         * to choose the ioreq server by range. If no server is found,
> +         * the access is ignored.
> +         *
> +         * - p2m_ioreq_server accesses are handled by the designated
> +         * ioreq_server for the domain, but there are some corner
> +         * cases:
> +         *
> +         *   - If the domain ioreq_server is NULL, assume there is a
> +         *   race between the unbinding of ioreq server and guest fault
> +         *   so re-try the instruction.

And that retry won't come back here because of? (The answer
should not include any behavior added by subsequent patches.)

> +         */
> +        struct hvm_ioreq_server *s = NULL;
> +        p2m_type_t p2mt = p2m_invalid;
> +
> +        if ( is_mmio )
> +        {
> +            unsigned long gmfn = paddr_to_pfn(addr);
> +
> +            get_gfn_query_unlocked(currd, gmfn, &p2mt);
> +
> +            if ( p2mt == p2m_ioreq_server )
> +            {
> +                unsigned int flags;
> +
> +                /*
> +                 * Value of s could be stale, when we lost a race
> +                 * with dm_op which unmaps p2m_ioreq_server from the
> +                 * ioreq server. Yet there's no cheap way to avoid
> +                 * this, so device model need to do the check.
> +                 */
> +                s = p2m_get_ioreq_server(currd, &flags);
> +
> +                /*
> +                 * If p2mt is ioreq_server but ioreq_server is NULL,
> +                 * we probably lost a race with unbinding of ioreq
> +                 * server, just retry the access.
> +                 */

This repeats the earlier comment - please settle on where to state
this, but don't say the exact same thing twice within a few lines of
code.

> +                if ( s == NULL )
> +                {
> +                    rc = X86EMUL_RETRY;
> +                    vio->io_req.state = STATE_IOREQ_NONE;
> +                    break;
> +                }
> +            }
> +        }
> +
> +        /*
> +         * Value of s could be stale, when we lost a race with dm_op
> +         * which unmaps this PIO/MMIO address from the ioreq server.
> +         * The device model side need to do the check.

I think "will do" would be more natural here, or add "anyway" to
the end of the sentence.

> @@ -914,6 +916,42 @@ 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,
> +                                     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_DMOP_IOREQ_MEM_ACCESS_WRITE) )

Stray parentheses.

> --- a/xen/arch/x86/mm/hap/nested_hap.c
> +++ b/xen/arch/x86/mm/hap/nested_hap.c
> @@ -172,7 +172,7 @@ nestedhap_walk_L0_p2m(struct p2m_domain *p2m, paddr_t 
> L1_gpa, paddr_t *L0_gpa,
>      if ( *p2mt == p2m_mmio_direct )
>          goto direct_mmio_out;
>      rc = NESTEDHVM_PAGEFAULT_MMIO;
> -    if ( *p2mt == p2m_mmio_dm )
> +    if ( *p2mt == p2m_mmio_dm || *p2mt == p2m_ioreq_server )

Btw., how does this addition match up with the rc value being
assigned right before the if()?

> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -131,6 +131,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 & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE);

Is this effectively open coded p2m_get_ioreq_server() actually
okay? If so, why does the function need to be used elsewhere,
instead of doing direct, lock-free accesses?

> +void p2m_destroy_ioreq_server(const struct domain *d,
> +                              const struct hvm_ioreq_server *s)
> +{
> +    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +
> +    spin_lock(&p2m->ioreq.lock);
> +
> +    if ( p2m->ioreq.server == s )
> +    {
> +        p2m->ioreq.server = NULL;
> +        p2m->ioreq.flags = 0;
> +    }
> +
> +    spin_unlock(&p2m->ioreq.lock);
> +}

Is this function really needed? I.e. can't the caller simply call
p2m_set_ioreq_server(d, 0, s) instead?

Jan

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