Re: [Qemu-devel] [PATCH] Memory API bugfix - abolish addrrrange_end()

[Avi Kivity <avi@redhat.com>]

On 10/18/2011 03:38 AM, David Gibson wrote:
> On Mon, Oct 17, 2011 at 12:34:19PM +0200, Avi Kivity wrote:
> > On 10/17/2011 07:31 AM, David Gibson wrote:
> > > > 
> > > > In terms of how the code looks, it's seriously more ugly (see the
> > > > patches I sent out).  Conceptually it's cleaner, since we're not dodging
> > > > the issue that we need to deal with a full 64-bit domain.
> > >
> > > We don't have to dodge that issue.  I know how to remove the
> > > requirement for intermediate negative values, I just haven't made up a
> > > patch yet.  With that we can change to uint64 and cover the full 64
> > > bit range.  In fact I think I can make it so that size==0 represents
> > > size=2^64 and even handle the full 64-bit, inclusive range properly.
> > 
> > That means you can't do a real size == 0.
>
> Yeah... a memory range with size 0 has no effect by definition, I
> think we can do without it.

How do we make sure all callers know this?

> > > > But my main concern is maintainability.  The 64-bit blanket is to short,
> > > > if we keep pulling it in various directions we'll just expose ourselves
> > > > in new ways.
> > >
> > > Nonsense, dealing with full X-bit range calculations in X-bit types is
> > > a fairly standard problem.  The kernel does it in VMA handling for
> > > one.  It just requires thinking about overflow cases.
> > 
> > We discovered three bugs already (you found two, and I had one during
> > development).  Even if it can probably be done with extreme care, but is
> > it worth spending all that development time on?
> > 
> > I'm not sure there is a parallel with vmas, since we're offsetting in
> > both the positive and negative directions.
>
> I think the so-called "negative offsetting" is just an artifact of our
> implementation.  I don't see that it's any different from having a VMA
> whose file offset is larger than its memory address.
>

Consider the vga window at 0xa0000 pointing into the framebuffer at
alias_offset 0x1a0000.  To the system, it looks like a copy of the
framebuffer starts at -0x1000000, becomes visible 0x1a0000 bytes later
(at 0xa0000), then becomes invisible again at 0xa8000.

Yes, it's an artifact, but I don't want to spend too much time worrying
about it, if I can throw a few more bits at the problem.  The API is too
central to make a case by case analysis of where things can go wrong, it
needs to be robust.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.