From mboxrd@z Thu Jan 1 00:00:00 1970 From: Don Slutz Subject: Re: [PATCH v13 7/8] Add IOREQ_TYPE_VMWARE_PORT Date: Mon, 14 Dec 2015 07:39:13 -0500 Message-ID: <566EB871.4040503@Gmail.com> References: <1448747105-19966-1-git-send-email-Don.Slutz@Gmail.com> <1448747105-19966-8-git-send-email-Don.Slutz@Gmail.com> <9AAE0902D5BC7E449B7C8E4E778ABCD02F69F705@AMSPEX01CL01.citrite.net> <5660DCFF.5050301@Gmail.com> <9AAE0902D5BC7E449B7C8E4E778ABCD02F6A6099@AMSPEX01CL01.citrite.net> <56620645.3090707@Gmail.com> <9AAE0902D5BC7E449B7C8E4E778ABCD02F6B43E6@AMSPEX01CL03.citrite.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <9AAE0902D5BC7E449B7C8E4E778ABCD02F6B43E6@AMSPEX01CL03.citrite.net> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Paul Durrant , "xen-devel@lists.xen.org" Cc: Kevin Tian , "Keir (Xen.org)" , Ian Campbell , Andrew Cooper , Eddie Dong , George Dunlap , Don Slutz , "Tim (Xen.org)" , Jan Beulich , Stefano Stabellini , Aravind Gopalakrishnan , Jun Nakajima , Ian Jackson , Wei Liu , Boris Ostrovsky , Suravee Suthikulpanit List-Id: xen-devel@lists.xenproject.org On 12/07/15 08:36, Paul Durrant wrote: >> -----Original Message----- > [snip] >>>>>> >>>>>> if ( rc ) >>>>>> - hvm_unmap_ioreq_page(s, 0); >>>>>> + { >>>>>> + hvm_unmap_ioreq_page(s, IOREQ_PAGE_TYPE_IOREQ); >>>>>> + return rc; >>>>>> + } >>>>>> + >>>>>> + rc = hvm_map_ioreq_page(s, IOREQ_PAGE_TYPE_VMPORT, >>>>>> vmport_ioreq_pfn); >>>>> >>>>> Is every ioreq server going to have one of these? It doesn't look >>>>> like it, so should you not have validity check on the pfn? >>>>> >>>> >>>> >>>> Currently the default is that all ioreq servers get the mapping: >>>> >>> >>> That's probably a bit wasteful. It should probably be >>> selectable in the create HVM op. >> >> Since the most common case is QEMU and it can use it since upstream >> version 2.2.0, the waste is real small. If a non-QEMU ioreq server does >> not want it, it add 0 overhead. > > It's not 0 overhead if an extra magic page is used for each ioreq server is it? > My understanding is that the Xen overhead is 1 entry in p2m for each ioreq server. >> The only case I know of (which is PVH >> related to PVH) is if QEMU is not running and you add a non-QEMU ioreq >> server that does not use it, you get 1 page + page table entries. >> >> While the following exists: >> >> #define HVM_IOREQSRV_BUFIOREQ_OFF 0 >> #define HVM_IOREQSRV_BUFIOREQ_LEGACY 1 >> /* >> * Use this when read_pointer gets updated atomically and >> * the pointer pair gets read atomically: >> */ >> #define HVM_IOREQSRV_BUFIOREQ_ATOMIC 2 >> uint8_t handle_bufioreq; /* IN - should server handle buffered ioreqs */ >> >> I see no tests that use these. Also adding vmport enable/disable to >> handle_bufioreq is much more of a hack then I expect to pass a code >> review. >> >> Does not look simple to add a new additional argument, but that could >> just be my lack of understanding in the area. > > It doesn't look that bad. The bufioreq flag has now expanded > from 1 bit to 2 bits, but you could rename 'handle_bufioreq' to > 'flags' or some such and then use bit 3 to indicate whether or > not the vmport ioreq page should be allocated. > Ok, I will code this up and plan to go with it. Since old QEMU need to be supported, bit 3 will be a negative flag, when set no page will be mapped. >> >>> I don't know whether you'd >>> even need it there in the default server since I guess the QEMU >>> end of things post-dates the use of the HVM op (rather than the >>> old param). >>> >> >> Not sure how to parse "post-dates". Here is some tables about this that >> I know about: >> >> >> xen Supports handle_bufioreq >> create_ioreq_server >> 4.5 yes 0 or !0 >> 4.6 yes 0 or !0 >> 4.7 yes 0 or !0 >> >> Upstream vmport is_default atomic >> QEMU support >> >> 2.2.x yes yes n/a >> 2.3.x yes no no >> 2.4.x yes no no >> 2.5.x yes no yes >> >> Xen vmport is_default atomic >> QEMU support >> >> 4.5.x no yes n/a >> 4.6.x yes no yes >> 4.7.x yes no yes >> >> With just a "xen only" build, you will not get a QEMU that is a default >> ioreq server. However it looks possible to mix Xen and QEMU and get >> this case. >> > > What I meant was that I believe that the vmport ioreq page will > only be used by a qemu that also make use of the > create_ioreq_server hmvop, so you don't have to care about > making it work with older QEMU. It looks like 2.2.x may prove > me wrong though in which case it should be present in the > default ioreq server but still optional for all others. > Yes, default ioreq server will get the mapping when enabled, optional for all others. -Don Slutz > Paul > >> So unless I hear otherwise, I will not be making a change here. >> >>>> + /* VMware port */ >>>> + if ( i == HVMOP_IO_RANGE_VMWARE_PORT && >>>> + s->domain->arch.hvm_domain.is_vmware_port_enabled ) >>>> + rc = rangeset_add_range(s->range[i], 1, 1); >>>> >>>> >>>> >>>> but you are right that a check on is_vmware_port_enabled should be >>>> added. Will do. >>> >>> Cool. Thanks, >>> >>> Paul >>> >>>> >>>> -Don Slutz >>>> >>>>> Paul >>>>>