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

[Yu Zhang <yu.c.zhang@linux.intel.com>]

On 3/13/2017 7:20 PM, Jan Beulich wrote:
>>>> On 11.03.17 at 09:42, <yu.c.zhang@linux.intel.com> wrote:
>> On 3/10/2017 11:29 PM, Jan Beulich wrote:
>>>>>> On 08.03.17 at 16:33, <yu.c.zhang@linux.intel.com> wrote:
>>>> changes in v7:
>>>>     - Use new ioreq server interface - XEN_DMOP_map_mem_type_to_ioreq_server.
>>>>     - According to comments from George: removed domain_pause/unpause() in
>>>>       hvm_map_mem_type_to_ioreq_server(), because it's too expensive,
>>>>       and we can avoid the:
>>>>       a> deadlock between p2m lock and ioreq server lock by using these locks
>>>>          in the same order - solved in patch 4;
>>> That is, until patch 4 there is deadlock potential? I think you want to
>>> re-order the patches if so. Or was it that the type can't really be used
>>> until the last patch of the series? (I'm sorry, it's been quite a while
>>> since the previous version.)
>> Oh. There's no deadlock potential in this version patch set. But in v6, there was, and I used
>> domain_pause/unpause() to avoid this. Later on, I realized that if I use different locks in the
>> same order, the deadlock potential can be avoid and we do not need domain_pause/unpause
>> in this version.
> Well, okay, but in the future please don't add misleading change
> info then.

Got it. Thanks. :-)
>>>> @@ -365,6 +383,24 @@ static int dm_op(domid_t domid,
>>>>            break;
>>>>        }
>>>>    
>>>> +    case XEN_DMOP_map_mem_type_to_ioreq_server:
>>>> +    {
>>>> +        const struct xen_dm_op_map_mem_type_to_ioreq_server *data =
>>>> +            &op.u.map_mem_type_to_ioreq_server;
>>>> +
>>>> +        rc = -EINVAL;
>>>> +        if ( data->pad )
>>>> +            break;
>>>> +
>>>> +        /* Only support for HAP enabled hvm. */
>>>> +        if ( !hap_enabled(d) )
>>>> +            break;
>>> Perhaps better to give an error other than -EINVAL in this case?
>>> If so, then the same error should likely also be used in your
>>> set_mem_type() addition.
>> How about -ENOTSUP?
> If you mean -EOPNOTSUPP, then yes.

Yes, I mean -EOPNOTSUPP
>>>> @@ -177,8 +178,65 @@ 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 current
>>>> +         * ioreq_server for the domain, but there are some corner
>>>> +         * cases:
>>> Who or what is "the current ioreq_server for the domain"?
>> It means "the current ioreq_server which maps the p2m_ioreq_server type
>> for this domain"...
>> I'd like to use a succinct phrase, but now seems not accurate enough.
>> Any preference?
> Just add "designated" after "current", or replacing "current"?

Replacing "current" with "designated" sounds good to me. :-)
>>>> +            if ( p2mt == p2m_ioreq_server )
>>>> +            {
>>>> +                unsigned int flags;
>>>> +
>>>> +                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.
>>>> +                 */
>>>> +                if ( s == NULL )
>>>> +                {
>>>> +                    rc = X86EMUL_RETRY;
>>>> +                    vio->io_req.state = STATE_IOREQ_NONE;
>>>> +                    break;
>>>> +                }
>>>> +
>>>> +                /*
>>>> +                 * If the IOREQ_MEM_ACCESS_WRITE flag is not set,
>>>> +                 * we should set s to NULL, and just ignore such
>>>> +                 * access.
>>>> +                 */
>>>> +                if ( !(flags & XEN_DMOP_IOREQ_MEM_ACCESS_WRITE) )
>>>> +                    s = NULL;
>>> What is this about? You only allow WRITE registrations, so this looks
>>> to be dead code. Yet if it is meant to guard against future enabling
>>> of READ, then this clearly should not be done for reads.
>> It's to guard against future emulation of READ. We can remove it for now.
> I guess first of all you need to settle on what you want the code to
> look like generally wrt reads: Do you want it to support the option
> as much as possible, reducing code changes to a minimum when
> someone wants to actually add support, or do you want to reject
> such attempts? Whichever variant you choose, you should carry it
> out consistently rather than mixing both.
>
>>>> --- 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);
>>> Along the lines of the previous comment - if you mean to have the
>>> code cope with READ, please also set ->r accordingly, or add a
>>> comment why this won't have the intended effect (yielding a not
>>> present EPTE).
>> How about we keep this code and do not support READ? I'll remove above
>> dead code in hvmemul_do_io().
> Sure, as said above: All I'd like to push for is that the result is
> consistent across the code base.

Thank you, Jan. I should have keep a consistent principle in this code.
My preference is to remove the possible read emulation logic. But could 
we still keep the
definition of XEN_DMOP_IOREQ_MEM_ACCESS_READ, so that people can still 
know this
interface can be extended in the future?

>>>> +struct hvm_ioreq_server *p2m_get_ioreq_server(struct domain *d,
>>>> +                                              unsigned int *flags)
>>>> +{
>>>> +    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;
>>>> +}
>>> I'm afraid this question was asked before, but since there's no
>>> comment here or anywhere, I can't recall if there was a reason why
>>> s potentially being stale by the time the caller looks at it is not a
>>> problem.
>> Well, it is possibe that s is stale. I did not take it as a problem
>> because the device model
>> will later discard such io request. And I believe current
>> hvm_select_ioreq_server() also
>> has the same issue - the returned s should be considered to be stale, if
>> the MMIO/PIO
>> address is removed from the ioreq server's rangeset.
>>
>> Another thought is, if you think it is inappropriate for device model to
>> do the check,
>> we can use spin_lock_recursive on ioreq_server.lock to protect all the
>> ioreq server select
>> and release the lock after the ioreq server is sent out.
> Well, let's first ask Paul as to what his perspective here is, both
> specifically for this change and more generally regarding what
> you say above.

Paul, any suggestions on this and the above one? :)

Thanks
Yu
> Jan
>


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