[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/16/2016 6:02 PM, Jan Beulich wrote: On 16.06.16 at 11:32, <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:@@ -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.OK. I can keep the interface, and using uint32_t type in the internal routine would means a implicit type conversion from uint16_6, which I do not think is a problem.Just to reiterate: Unless there is a specific need, please avoid fixed width integer types for any internal use.@@ -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?Please wait for Andrew to confirm this (or correct me) - there are some differences between AMD and Intel, and iirc the bit gets ignored by AMD when NX is off. Hi Andrew, sorry to bother you. Any comments on this? Thanks! :) Yu _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |