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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 13 Jun 2019, Julien Grall wrote:
> 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.

I would drop it, but I don't care much about it.

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

Yes, either way

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