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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.09.2024 16:50, oleksii.kurochko@xxxxxxxxx wrote:
> 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?

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.

If I can't convince you, then so be it. Yet then I'm also not very likely
to ack this patch of yours (which of course won't mean I wouldn't further
look at other aspects of the change, hopefully at least making it easier
for someone else to ack it).

Jan