On 25.09.2024 12:07, oleksii.kuroc...@gmail.com wrote:
> On Tue, 2024-09-24 at 15:31 +0200, Jan Beulich wrote:
>> On 24.09.2024 13:30, oleksii.kuroc...@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.
Jan
