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 <mark.cave-ayl...@ilande.co.uk> > > > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > > > > > > > > > > > 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