Re: [Qemu-devel] [PATCH] memory: info mtree check mr range overflow

[Michael S. Tsirkin]

On Wed, Mar 15, 2017 at 11:15:50AM +0800, Peter Xu wrote:
> On Wed, Mar 15, 2017 at 03:24:04AM +0200, Michael S. Tsirkin wrote:
> > On Tue, Mar 14, 2017 at 08:56:27PM +0800, Peter Xu wrote:
> > > The address of memory regions might overflow when something wrong
> > > happened, like reported in:
> > > 
> > > https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg02043.html
> > > 
> > > For easier debugging, let's try to detect it.
> > > 
> > > Reported-by: Mark Cave-Ayland <address@hidden>
> > > Signed-off-by: Peter Xu <address@hidden>
> > 
> > 
> > After a chat with Paolo, I think the following is a more general fix
> > 
> > - fix info mtree to do 128 bit math and display more than
> >   16 digits if necessary
> 
> Could you help elaborate in what case will we really need that 128 bit
> address?

This is how memory API works. It uses 128 bit addresses (in reality
it typically only needs 64 bit addresses but 128 means it can do
math without worrying about it too much).
Thus a region at offset 0xf << 60 in parent with address 0x1 << 60
and size 0x1 << 20 is not "overflowing" it is simply at and address
0x1 << 64 which is outside the range of parent so not visible
in the flat view.
But same can be said for region at offset 0x1 << 60 in same parent
and your patch does nothing to help detect it.

> Btw, thanks for pointing out in the other thread that your patch
> wasn't printing 128 bits but 64 bits, actually I didn't notice that
> before... but even with that, I would still slightly prefer this one
> though considering readability and simplicity.

Right but it's just trying to address the specific problem with
the given device. Which is unlikely to trigger again exactly
in the same way. The general issue is that the child region
address is outside the range of the parent.

> > - add info about region visibility
> >   how much info is appropriate is arguable - after all we already have info 
> > mtree -f
> >   we probably should report if region is not visible at all,
> >   how about partially occluded ones? listing all windows is probably not
> >   needed - we have the -f flag for that.
> 
> For me, "info mtree" and its "-f" form work good enough. So I'll leave
> the discussion on this one to people who know better than me...
> 
> Thanks,
> 
> -- peterx