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 14.06.16 at 15:13, <george.dunlap@xxxxxxxxxx> wrote:
> On 14/06/16 11:45, Jan Beulich wrote:
>>> +                         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?
> 
> You make the map_memtype_... hypercall with "flags" set to 0.  I'm not
> sure what's unclear about the sentence; how would you put it differently?

I'd use "flushing", or indeed anything that doesn't resemble wording
used to describe how arguments get handed to functions.

>> 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.)
> 
> How is that different than calling set_mem_type() first, and then
> de-registering without first unmapping all the types?

Didn't we all agree this is something that should be disallowed
anyway (not that I've seen this implemented, i.e. just being
reminded of it by your reply)?

>>> +    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?
> 
> I assume he's just going along with what he sees in params.h.
> "Violating standard C name space rules" by having #defines which start
> with a single _ seems to be a well-established policy for Xen. :-)

Sadly, and I'm trying to prevent matters becoming worse.
Speaking of which - there are XEN_ prefixes missing here too.

Jan


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