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

[Michael S. Tsirkin]

On Wed, Mar 15, 2017 at 12:04:27PM +0800, Peter Xu wrote:
> On Wed, Mar 15, 2017 at 05:30:56AM +0200, Michael S. Tsirkin wrote:
> > 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).
> 
> Yes. To be more specific, could I ask why do we need 128 bits here
> when doing "info mtree"?

Because when you add two 64 bit addresses you sometimes get a 67
bit one. info mtree shows some fictitious data: base/end addresses
that region would have had if it was fully visible in a flatview.
67 bit addresses are never visible there, that is true,
but that is not the only kind of address that is not visible
in flatview yet shown by info mtree.

> > 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.
> 
> Not sure I fully understand the case mentioned above... I believe for
> above example, current patch (either with, or without) will print:
> 
>     0x2000000000000000
> 
> And even with the patch "memory: use 128 bit in info mtree", it should
> print the same. IIUC this is what we want, no? Did I miss anything?

What are you trying to achieve though? The issue that started
it all is an openbios bug which did not init 64 bit bars
correctly. As a result the bar was not visible to guest
in the flatview and device did not work.


> > 
> > > 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.
> 
> Hmm... frankly speaking I don't know whether current memory API would
> allow this happen. I just see no danger if that happens, as long as we
> will make sure those outranged regions will never be used during
> rendering.
> 
> Anyway, IMHO that's another topic. This patch should be solely solving
> the issue that was reported. Thanks,
> 
> -- peterx


I think we need to address the root issue which is 64 bit math
which is the wrong thing to do within memory core.

-- 
MST