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

[oleksii.kurochko@gmail.com]

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) )
        ...

> 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;
               }
   ...
   
~ Oleksii