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 15.06.16 at 12:52, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
> On 6/14/2016 6:45 PM, Jan Beulich wrote:
>>>>> On 19.05.16 at 11:05, <yu.c.zhang@xxxxxxxxxxxxxxx> wrote:
>>> A new HVMOP - HVMOP_map_mem_type_to_ioreq_server, is added to
>>> let one ioreq server claim/disclaim its responsibility for the
>>> handling of guest pages with p2m type p2m_ioreq_server. Users
>>> of this HVMOP can specify which kind of operation is supposed
>>> to be emulated in a parameter named flags. Currently, this HVMOP
>>> only support the emulation of write operations. And it can be
>>> easily extended to support the emulation of read ones if an
>>> ioreq server has such requirement in the future.
>> Didn't we determine that this isn't as easy as everyone first thought?
> 
> My understanding is that to emulate read, we need to change the definition
> of is_epte_present(), and I do not think this change will cause much 
> trouble.
> But since no one is using the read emulation, I am convinced the more 
> cautious
> way is to only support emulations for write operations for now.

Well, okay. I'd personally drop the "easily", but you know what
to tell people if later they come ask how this "easily" was meant.

>>> @@ -914,6 +916,45 @@ 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,
>>> +                                     uint16_t type, uint32_t flags)
>> I see no reason why both can't be unsigned int.
> 
> Parameter type is passed in from the type field inside struct 
> xen_hvm_map_mem_type_to_ioreq_server,
> which is a uint16_t, followed with a uint16_t pad. Now I am wondering, 
> may be we can just remove the pad
> field in this structure and just define type as uint32_t.

I think keeping the interface structure unchanged is the desirable
route here. What I dislike is the passing around of non-natural
width types, which is more expensive in terms of processing. I.e.
as long as a fixed width type (which is necessary to be used in
the public interface) fits in "unsigned int", that should be the
respective internal type. Otherwise "unsigned long" etc.

There are cases where even internally we indeed want to use
fixed width types, and admittedly there are likely far more cases
where internally fixed width types get used without good reason,
but just like everywhere else - let's please not make this worse.
IOW please use fixed width types only when you really need them.

>>> --- a/xen/arch/x86/mm/p2m-ept.c
>>> +++ b/xen/arch/x86/mm/p2m-ept.c
>>> @@ -132,6 +132,12 @@ 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 = entry->x = 1;
>> Why x?
> 
> Setting entry->x to 1 is not a must. I can remove it. :)

Please do. We shouldn't grant permissions without reason.

>>> @@ -94,8 +96,16 @@ static unsigned long p2m_type_to_flags(p2m_type_t t, 
>>> mfn_t mfn,
>>>       default:
>>>           return flags | _PAGE_NX_BIT;
>>>       case p2m_grant_map_ro:
>>> -    case p2m_ioreq_server:
>>>           return flags | P2M_BASE_FLAGS | _PAGE_NX_BIT;
>>> +    case p2m_ioreq_server:
>>> +    {
>>> +        flags |= P2M_BASE_FLAGS | _PAGE_RW;
>>> +
>>> +        if ( p2m->ioreq.flags & P2M_IOREQ_HANDLE_WRITE_ACCESS )
>>> +            return flags & ~_PAGE_RW;
>>> +        else
>>> +            return flags;
>>> +    }
>> Same here (for the missing _PAGE_NX) plus no need for braces.
> 
> I'll remove the brace. And we do not need to set the _PAGE_NX_BIT, like 
> the p2m_ram_ro case I guess.

I hope you mean the inverse: You should set _PAGE_NX_BIT here.

>>> +                         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?
> 
> It means if this routine is called with flags=0, it will unmap the ioreq 
> server.

Oh, that's a completely different reading of the comment than I had
implied: With what you say, "flags" here really refers to the function
parameter, whereas I implied it to refer to the structure field. I think
if that's what you want to say, then the comment should be put next
to the surrounding if() to clarify what "flags" refers to.

>>> +{
>>> +    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;
>>> +}
>> 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.)
> 
> Yes. The case you mentioned might happen. But it's not a big deal I 
> guess. If such
> case happens, the  backend driver will receive an io request and can 
> then discard it
> if it has just de-registered the mem type.

Could you clarify in a comment then what the lock is (and is not)
meant to guard against?

>>> +struct xen_hvm_map_mem_type_to_ioreq_server {
>>> +    domid_t domid;      /* IN - domain to be serviced */
>>> +    ioservid_t id;      /* IN - ioreq server id */
>>> +    uint16_t type;      /* IN - memory type */
>>> +    uint16_t pad;
>> This field does not appear to get checked in the handler.
> 
> I am now wondering, how about we remove this pad field and define type 
> as uint32_t?

As above - I think the current layout is fine. But I'm also not heavily
opposed to using uint32_t here. It's not a stable interface anyway
(and I already have a series mostly ready to split off all control
operations from the HVMOP_* ones, into a new HVMCTL_* set,
which will make all of them interface-versioned).

Jan

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