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

[oleksii.kurochko@gmail.com]

On Fri, 2024-09-27 at 11:15 +0200, Jan Beulich wrote:
> On 27.09.2024 10:49, oleksii.kurochko@gmail.com wrote:
> > On Wed, 2024-09-25 at 16:58 +0200, Jan Beulich wrote:
> > > On 25.09.2024 16:50, oleksii.kurochko@gmail.com wrote:
> > > > On Wed, 2024-09-25 at 16:22 +0200, Jan Beulich wrote:
> > > > > On 25.09.2024 12:07, oleksii.kurochko@gmail.com wrote:
> > > > > > On Tue, 2024-09-24 at 15:31 +0200, Jan Beulich wrote:
> > > > > > > On 24.09.2024 13:30, oleksii.kurochko@gmail.com 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?
> > > 
> > > While I think I see where you're coming from, I still don't
> > > agree.
> > > And
> > > I never suggested to bubble up some custom error indication. Up
> > > the
> > > call
> > > chain it wants to be -ENOMEM, sure. Yet keying its generation to
> > > flags & (PTE_VALID | PTE_POPULATE) is both less obvious and more
> > > fragile
> > > (towards future code changes) than keying it to rc ==
> > > XEN_TABLE_MAP_NOMEM.
> > I am not sure that (rc == XEN_TABLE_MAP_NOMEM) is equal to (flags &
> > (PTE_VALID | PTE_POPULATE) as XEN_TABLE_MAP_NOMEM miss the case
> > (flags
> > & PTE_VALID) == 0 ( removing a mapping case ) and for which should
> > be
> > returned 0 but not -ENOMEM.
> 
> The intention is quite clear: Return -ENOMEM if and only if an
> allocation
> failed. Hence why I think the XEN_TABLE_MAP_NOMEM approach is
> preferable.
Just to be sure. Do you mean the following:

diff --git a/xen/arch/riscv/pt.c b/xen/arch/riscv/pt.c
index d7eb207ddc..6cd2e595b6 100644
--- a/xen/arch/riscv/pt.c
+++ b/xen/arch/riscv/pt.c
@@ -209,24 +209,15 @@ 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_NOMEM )
         {
-            rc = 0;
-
-            /*
-             * We are here because pt_next_level has failed to map
-             * the intermediate page table (e.g the table does not
exist
-             * and the pt couldn't be allocated). It is a valid case
when
-             * removing a mapping as it may not exist in the page
table.
-             * In this case, just ignore it.
-             */
-            if ( flags & (PTE_VALID | PTE_POPULATE) )
-            {
-                dprintk(XENLOG_ERR,
-                        "%s: Unable to map level %u\n", __func__,
level);
-                rc = -ENOMEM;
-            }
+            rc = -ENOMEM;
+            goto out;
+        }
 
+        if ( rc == XEN_TABLE_MAP_NONE )
+        {
+            rc = 0;
             goto out;
         }

~ Oleksii