[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v4 3/3] x86/ioreq server: Add HVMOP to map guest ram with p2m_ioreq_server to an ioreq server.
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. OK. Let's drop the word "easily". :) OK. I can keep the interface, and using uint32_t type in the internal routine@@ -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. would means a implicit type conversion from uint16_6, which I do not think is a problem. --- 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. Got it. Thanks. @@ -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. Oh, right. I meant the reverse. Thanks for the remind. :)And I have a question, here in p2m_type_to_flags(), I saw current code uses _PAGE_NX_BIT to disable the executable permission, and I wonder, why don't we choose the _PAGE_NX, which is defined as: #define _PAGE_NX (cpu_has_nx ? _PAGE_NX_BIT : 0)How do we know for sure that bit 63 from pte is not a reserved one without checking the cpu capability(the cpu_has_nx)? Is there any other reasons, i.e. the page tables might be shared with IOMMU? + 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. Agreed. I'll move this comment above the surrounding if(). +{ + 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? For now, only one ioreq server is allowed to be bounded with HVMMEM_ioreq_server, one usage of this lock is that in p2m_set_ioreq_server(), it can prevent concurrent setting requirements from multiple ioreq servers. And although it can not protect the return value from p2m_get_ioreq_server(), it can still provide some kind protection inside the routine. I'll add the comments to illustrate this. :) +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). I'd like to keep this interface. BTW, you mentioned "this field does not appear to get checked in the handler", do you mean we need to check the pad in the handler? And why? Thanks Yu _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |