Re: [PATCH v4 07/17] x86/hvm: add length to mmio check op

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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, <andrew.cooper3@citrix.com> 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 <paul.durrant@citrix.com>
> >>> Cc: Keir Fraser <keir@xen.org>
> >>> Cc: Jan Beulich <jbeulich@suse.com>
> >>> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> >>>
> >> 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.

  Paul

> ~Andrew