Re: [Qemu-devel] [PATCH 2/6] exec.c: remove an unnecessary assert on PHYS_MAP_NODE_NIL in phys_map_node_alloc()

[Wei Yang]

On Thu, Sep 12, 2019 at 02:42:26PM +0200, Paolo Bonzini wrote:
>On 12/09/19 04:51, Wei Yang wrote:
>> On Fri, Aug 23, 2019 at 09:07:50AM +0800, Wei Yang wrote:
>>> On Thu, Aug 22, 2019 at 12:24:32PM +0200, Paolo Bonzini wrote:
>>>> On 21/03/19 09:25, Wei Yang wrote:
>>>>> PHYS_MAP_NODE_NIL is assigned to PhysPageEntry.ptr in case this is not a
>>>>> leaf entry, while map->nodes_nb range in [0, nodes_nb_alloc).
>>>>>
>>>>> Seems we are asserting on two different things, just remove it.
>>>>
>>>> The assertion checks that this "if" is not entered incorrectly:
>>>>
>>>>    if (lp->skip && lp->ptr == PHYS_MAP_NODE_NIL) {
>>>>        lp->ptr = phys_map_node_alloc(map, level == 0);
>>>>    }
>>>>
>>>
>>> Hmm... I may not get your point.
>>>
>>> phys_map_node_alloc() will get an available PhysPageEntry and return its
>>> index, which will be assigned to its parent's ptr.
>>>
>>> The "if" checks on the parent's ptr, while the assertion asserts the index 
>>> for
>>> the new child. I may miss something?
>>>
>> 
>> Hi, Paolo,
>> 
>> Do I miss something?
>
>Sorry, I was on vacation.  phys_page_set_level can be called multiple
>times, with the same lp.  The assertion checks that only the first call
>will reach phys_map_node_alloc.
>

Ah, I am just back from vacation too. The mountain makes me painful. :-) 

So I guess you are talking about the situation of wrap around.
PHYS_MAP_NODE_NIL is an indicator that this lp is not used yet. And we are not
expecting any valid lp has its ptr with value equal or bigger than
PHYS_MAP_NODE_NIL.

If this is the case, I am thinking this won't happen in current
implementation. Because if my understanding is correct, the total number of
PhysPageEntry would be less than PHYS_MAP_NODE_NIL.

First let's look at the PHYS_MAP_NODE_NIL's value

    PHYS_MAP_NODE_NIL = 2^26 = 6710 8864.
    
So we could represent 6710 8864 PhysPageEntry at most.

Then let's look how many PhysPageEntry would be. The PhysPageEntry structure
forms a tree with 9 nodes on each level with P_L2_LEVELS levels. This means
the tree could have 9^P_L2_LEVELS nodes at most. 

Since

#define ADDR_SPACE_BITS 64
#define P_L2_BITS 9
#define P_L2_LEVELS (((ADDR_SPACE_BITS - TARGET_PAGE_BITS - 1) / P_L2_BITS) + 1)

And TARGET_PAGE_BITS >= 12, so P_L2_LEVELS is smaller than 

    7 = (64 - 12 - 1) / 9 + 1.

This leads to 

    9^P_L2_LEVELS <= 9^7 = 478 2969

It shows PHYS_MAP_NODE_NIL may represents more node the tree could hold.

The assert here is not harmful, while maybe we can have a better way to handle
it?

>Paolo

-- 
Wei Yang
Help you, Help me