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
