Re: [PATCH v4 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.

[George Dunlap <george.dunlap@citrix.com>]

On 14/06/16 14:31, Jan Beulich wrote:
>>>> On 14.06.16 at 15:13, <george.dunlap@citrix.com> 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)?

I think I suggested it as a good idea, but Paul and Yang both thought it
wasn't necessary.  Do you think it should be a requirement?

We could have the de-registering operation fail in those circumstances;
but probably a more robust thing to do would be to have Xen go change
all the ioreq_server entires back to ram_rw (since if the caller just
ignores the failure, things are in an even worse state).

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

Right, so in that case I think I would have said, "I realize that lots
of other places in the Xen interface use this sort of template for
flags, but I think it's a bad idea and I'm trying to stop it expanding.
 Is there any actual need to have the bit numbers defined separately?
If not, please just define each flag as (1u << 0), &c."

I think you've tripped over "changing coding styles" in unfamiliar code
before too, so you know how frustrating it is to try to follow the
existing coding style only to be told that you did it wrong. :-)

 -George



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