From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Durrant Subject: Re: [PATCH v4 07/17] x86/hvm: add length to mmio check op Date: Thu, 25 Jun 2015 13:48:17 +0000 Message-ID: <9AAE0902D5BC7E449B7C8E4E778ABCD0259712EB@AMSPEX01CL02.citrite.net> References: <1435145089-21999-1-git-send-email-paul.durrant@citrix.com> <1435145089-21999-8-git-send-email-paul.durrant@citrix.com> <558BF247.9000208@citrix.com> <558C14370200007800089A1F@mail.emea.novell.com> <558C0349.6030808@citrix.com> <9AAE0902D5BC7E449B7C8E4E778ABCD025971274@AMSPEX01CL02.citrite.net> <558C0428.3080706@citrix.com> <9AAE0902D5BC7E449B7C8E4E778ABCD0259712AC@AMSPEX01CL02.citrite.net> <558C0649.5080807@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z87Vu-0004XI-5p for xen-devel@lists.xenproject.org; Thu, 25 Jun 2015 13:48:22 +0000 In-Reply-To: <558C0649.5080807@citrix.com> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Andrew Cooper , Jan Beulich Cc: "xen-devel@lists.xenproject.org" , "Keir (Xen.org)" List-Id: xen-devel@lists.xenproject.org > -----Original Message----- > From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > Sent: 25 June 2015 14:47 > To: Paul Durrant; Jan Beulich > Cc: xen-devel@lists.xenproject.org; Keir (Xen.org) > Subject: Re: [PATCH v4 07/17] x86/hvm: add length to mmio check op > > On 25/06/15 14:38, Paul Durrant wrote: > >> -----Original Message----- > >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > >> Sent: 25 June 2015 14:38 > >> To: Paul Durrant; Jan Beulich > >> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org) > >> Subject: Re: [PATCH v4 07/17] x86/hvm: add length to mmio check op > >> > >> On 25/06/15 14:36, Paul Durrant wrote: > >>>> -----Original Message----- > >>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com] > >>>> Sent: 25 June 2015 14:34 > >>>> To: Jan Beulich > >>>> Cc: Paul Durrant; xen-devel@lists.xenproject.org; Keir (Xen.org) > >>>> Subject: Re: [PATCH v4 07/17] x86/hvm: add length to mmio check op > >>>> > >>>> On 25/06/15 13:46, Jan Beulich wrote: > >>>>>>>> On 25.06.15 at 14:21, wrote: > >>>>>> On 24/06/15 12:24, Paul Durrant wrote: > >>>>>>> When memory mapped I/O is range checked by internal handlers, > the > >>>> length > >>>>>>> of the access should be taken into account. > >>>>>>> > >>>>>>> Signed-off-by: Paul Durrant > >>>>>>> Cc: Keir Fraser > >>>>>>> Cc: Jan Beulich > >>>>>>> Cc: Andrew Cooper > >>>>>>> > >>>>>> For what purpose? The length of the access doesn't affect which > >> handler > >>>>>> should accept the IO. > >>>>>> > >>>>>> This length check now causes an MMIO handler to not claim an > access > >>>>>> which straddles the upper boundary. > >>>>>> > >>>>>> It is probably fine to terminate such an access early, but it isn't fine > >>>>>> to pass such a straddled access to the default ioreq server. > >>>>> No, without involving the length in the check we can end up with > >>>>> check() saying "Yes, mine" but read() or write() saying "Not me". > >>>>> What I would agree with is for the generic handler to split the > >>>>> access if the first byte fits, but the final byte doesn't. > >>>> I discussed this with Paul over lunch. I had not considered how IO gets > >>>> forwarded to the device model for shared implementations. > >>>> > >>>> Is it reasonable to split a straddled access and direct the halves at > >>>> different handlers? This is not in line with how other hardware behaves > >>>> (PCIe will reject any straddled access). Furthermore, given small MMIO > >>>> regions and larger registers, there is no guarantee that a single split > >>>> will suffice. > >>>> > >>>> I see in the other thread going on that a domain_crash() is deemed ok > >>>> for now, which is fine my me. > >>>> > >>> I think that also allows me to simplfy the patch since I don't have to > modify > >> the mmio_check op any more. I simply call it once for the first byte of the > >> access and, if it accepts, verify that it also accepts the last byte of the > access. > >> > >> At that point, I would say it would be easier to modify the claim check > >> to return "yes/straddled/no" rather than calling it twice. > > That's excessive code churn, I think. The check functions are generally > cheap and the second call is only made if the first accepts. > > You are already churning everything anyway by inserting an extra > parameter. I do think it would make the logic cleaner and easier to > follow (which IMO takes precedent over churn). > No, my point was that by making the second call I don't need to add the extra parameter. Wait for the revised patch... it's about 6 lines long now ;-) Paul > ~Andrew