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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan