Re: [PATCH v7 7/8] xen/riscv: page table handling

[oleksii.kurochko@xxxxxxxxx]

On Wed, 2024-09-25 at 16:22 +0200, Jan Beulich wrote:
> On 25.09.2024 12:07, oleksii.kurochko@xxxxxxxxx wrote:
> > On Tue, 2024-09-24 at 15:31 +0200, Jan Beulich wrote:
> > > On 24.09.2024 13:30, oleksii.kurochko@xxxxxxxxx wrote:
> > > > On Tue, 2024-09-24 at 12:49 +0200, Jan Beulich wrote:
> > > > > On 13.09.2024 17:57, Oleksii Kurochko wrote:
> > > > > > +static int pt_next_level(bool alloc_tbl, pte_t **table,
> > > > > > unsigned
> > > > > > int offset)
> > > > > > +{
> > > > > > +    pte_t *entry;
> > > > > > +    mfn_t mfn;
> > > > > > +
> > > > > > +    entry = *table + offset;
> > > > > > +
> > > > > > +    if ( !pte_is_valid(*entry) )
> > > > > > +    {
> > > > > > +        if ( !alloc_tbl )
> > > > > > +            return XEN_TABLE_MAP_FAILED;
> > > > > > +
> > > > > > +        if ( create_table(entry) )
> > > > > > +            return XEN_TABLE_MAP_FAILED;
> > > > > 
> > > > > You're still losing the -ENOMEM here.
> > > > Agree, I will save the return value of create_table and return
> > > > it.
> > > 
> > > That won't work very well, will it? 
> > I think it will work, just will be needed another one check in
> > pt_update_entry() where pt_next_level() is called:
> >         if ( (rc == XEN_TABLE_MAP_FAILED) || (rc == -ENOMEM) )
> >         ...
> 
> Yet that's precisely why I said "won't work very well": You're now
> having
> rc in two entirely distinct number spaces (XEN_TABLE_MAP_* and -E*).
> That's imo just calling for trouble down the road. Unless you
> emphasized
> this aspect pretty well in a comment.
> 
> > > Imo you need a new XEN_TABLE_MAP_NOMEM.
> > > (And then XEN_TABLE_MAP_FAILED may want renaming to e.g.
> > > XEN_TABLE_MAP_NONE).
> > I am still curious if we really need this separation. If to in this
> > way
> > then it should be updated the check in pt_update_entry():
> >    --- a/xen/arch/riscv/pt.c
> >    +++ b/xen/arch/riscv/pt.c
> >    @@ -165,10 +165,10 @@ static int pt_next_level(bool alloc_tbl,
> > pte_t
> >    **table, unsigned int offset)
> >         if ( !pte_is_valid(*entry) )
> >         {
> >             if ( !alloc_tbl )
> >    -            return XEN_TABLE_MAP_FAILED;
> >    +            return XEN_TABLE_MAP_NONE;
> >     
> >             if ( create_table(entry) )
> >    -            return XEN_TABLE_MAP_FAILED;
> >    +            return XEN_TABLE_MAP_NOMEM;
> >         }
> >     
> >         if ( pte_is_mapping(*entry) )
> >    @@ -209,7 +209,7 @@ static int pt_update_entry(mfn_t root,
> > unsigned
> >    long virt,
> >         for ( ; level > target; level-- )
> >         {
> >             rc = pt_next_level(alloc_tbl, &table, offsets[level]);
> >    -        if ( rc == XEN_TABLE_MAP_FAILED )
> >    +        if ( (rc == XEN_TABLE_MAP_NONE) && (rc ==
> > XEN_TABLE_MAP_NOMEM)
> >    )
> >             {
> >                 rc = 0;
> > But the handling of XEN_TABLE_MAP_NONE and XEN_TABLE_MAP_NOMEM
> > seems to
> > me should be left the same as this one part of the code actually
> > catching the case when create_table() returns -ENOMEM:
> >    pt_next_level()
> >    {
> >    ...
> >                if ( flags & (PTE_VALID | PTE_POPULATE) )
> >                {
> >                    dprintk(XENLOG_ERR,
> >                            "%s: Unable to map level %u\n",
> > __func__,
> >    level);
> >                    rc = -ENOMEM;
> >                }
> 
> Except that you want to avoid "inventing" an error code when you were
> handed one. Just consider what happens to this code if another -E...
> could also come back from the helper.
I think we can drop the usage of -ENOMEM in the helper create_table()
by returning XEN_TABLE_MAP_FAILED in case of failure, with a
redefinition of XEN_TABLE_MAP_FAILED = 1, XEN_TABLE_SUPER_PAGE = 2, and
XEN_TABLE_NORMAL = 3, as value 0 is used to indicate that everything is
okay.

We can leave the pt_update() code as it is now:
...
            if ( flags & (PTE_VALID | PTE_POPULATE) )
            {
                dprintk(XENLOG_ERR,
                      "%s: Unable to map level %u\n", __func__, level);
                rc = -ENOMEM;
            }
...

Because for the end user, it's better to receive the error code from
xen/errno.h rather than a custom error code introduced nearby the
helper.

Does it make sense?

~ Oleksii