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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 11.03.17 at 09:42, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> On 3/10/2017 11:29 PM, Jan Beulich wrote:
>>>>> On 08.03.17 at 16:33, <yu.c.zhang@xxxxxxxxxxxxxxx> 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.

>>> @@ -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.

>>> @@ -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"?

>>> +            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.

>>> +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.

Jan

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