Re: [Xen-devel] [PATCH MM-PART3 v2 10/12] xen/arm: mm: Rework Xen page-tables walk during update

[Julien Grall <Julien.Grall@xxxxxxx>]

Hi Stefano,

On 13/06/2019 18:59, Stefano Stabellini wrote:
> On Thu, 13 Jun 2019, Julien Grall wrote: >>>> + * Return values:
>>>> + *  XEN_TABLE_MAP_FAILED: Either read_only was set and the entry
>>>> + *  was empty, or allocating a new page failed.
>>>> + *  XEN_TABLE_NORMAL_PAGE: next level mapped normally
>>>> + *  XEN_TABLE_SUPER_PAGE: The next entry points to a superpage.
>>>> + */
>>>> +static int xen_pt_next_level(bool read_only, unsigned int level,
>>>> +                             lpae_t **table, unsigned int offset)
>>>> +{
>>>> +    lpae_t *entry;
>>>> +    int ret;
>>>> +
>>>> +    entry = *table + offset;
>>>> +
>>>> +    if ( !lpae_is_valid(*entry) )
>>>> +    {
>>>> +        if ( read_only )
>>>> +            return XEN_TABLE_MAP_FAILED;
>>>> +
>>>> +        ret = create_xen_table(entry);
>>>> +        if ( ret )
>>>> +            return XEN_TABLE_MAP_FAILED;
>>>> +    }
>>>> +
>>>> +    ASSERT(lpae_is_valid(*entry));
>>>
>>> Why the ASSERT just after the lpae_is_valid check above?
>>
>> When the entry is invalid, the new page table will be allocated and the entry
>> will be generated. The rest of the function will then be executed. The
>> ASSERT() here confirms the entry we have in hand is valid in all the cases.
> 
> So it's to double-check that after getting into the `if' statement, the
> entry becomes valid, which is kind of redundant due to the two errors
> check above but it is still valid. OK.

While I agree that we have 2 ifs above, we only check "rc" and not "entry".

I ought to think I wrote perfect code, sadly this is not always the case ;).

Here, it would catch any mistake if "rc" is zero but "entry" is still 
invalid. The risk here is the "entry" would be invalid but the mistake 
may be spotted a long time after (i.e any access to the mapping will 
fault). This would potentially cost a lot of debug.

I agree this is probably over cautious, I can't remember if I hit the 
problem before. Anyway, I am happy to drop the ASSERT() if you think it 
is too redundant.

Regardless that, are you happy with the rest of the patch? If so, can I 
get your acked-by/reviewed-by?

Cheers,

-- 
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel