On 29.07.2025 15:47, Oleksii Kurochko wrote:
>
> On 7/22/25 2:05 PM, Jan Beulich wrote:
>> On 22.07.2025 14:03, Oleksii Kurochko wrote:
>>> On 7/21/25 3:39 PM, Jan Beulich wrote:
>>>> On 18.07.2025 16:37, Oleksii Kurochko wrote:
>>>>> On 7/2/25 12:28 PM, Jan Beulich wrote:
>>>>>> On 02.07.2025 12:09, Jan Beulich wrote:
>>>>>>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>>>>>>> @@ -613,3 +612,91 @@ void __iomem *ioremap(paddr_t pa, size_t len)
>>>>>>>> {
>>>>>>>> return ioremap_attr(pa, len, PAGE_HYPERVISOR_NOCACHE);
>>>>>>>> }
>>>>>>>> +
>>>>>>>> +int page_is_ram_type(unsigned long mfn, unsigned long mem_type)
>>>>>>>> +{
>>>>>>>> + ASSERT_UNREACHABLE();
>>>>>>>> +
>>>>>>>> + return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static struct domain *page_get_owner_and_nr_reference(struct
>>>>>>>> page_info *page,
>>>>>>>> + unsigned long
>>>>>>>> nr)
>>>>>>>> +{
>>>>>>>> + unsigned long x, y = page->count_info;
>>>>>>>> + struct domain *owner;
>>>>>>>> +
>>>>>>>> + /* Restrict nr to avoid "double" overflow */
>>>>>>>> + if ( nr >= PGC_count_mask )
>>>>>>>> + {
>>>>>>>> + ASSERT_UNREACHABLE();
>>>>>>>> + return NULL;
>>>>>>>> + }
>>>>>>> I question the validity of this, already in the Arm original: I can't
>>>>>>> spot
>>>>>>> how the caller guarantees to stay below that limit. Without such an
>>>>>>> (attempted) guarantee, ASSERT_UNREACHABLE() is wrong to use. All I can
>>>>>>> see
>>>>>>> is process_shm_node() incrementing shmem_extra[].nr_shm_borrowers,
>>>>>>> without
>>>>>>> any limit check.
>>>>>>>
>>>>>>>> + do {
>>>>>>>> + x = y;
>>>>>>>> + /*
>>>>>>>> + * Count == 0: Page is not allocated, so we cannot take a
>>>>>>>> reference.
>>>>>>>> + * Count == -1: Reference count would wrap, which is invalid.
>>>>>>>> + */
>>>>>>> May I once again ask that you look carefully at comments (as much as at
>>>>>>> code)
>>>>>>> you copy. Clearly this comment wasn't properly updated when the bumping
>>>>>>> by 1
>>>>>>> was changed to bumping by nr.
>>>>>>>
>>>>>>>> + if ( unlikely(((x + nr) & PGC_count_mask) <= nr) )
>>>>>>>> + return NULL;
>>>>>>>> + }
>>>>>>>> + while ( (y = cmpxchg(&page->count_info, x, x + nr)) != x );
>>>>>>>> +
>>>>>>>> + owner = page_get_owner(page);
>>>>>>>> + ASSERT(owner);
>>>>>>>> +
>>>>>>>> + return owner;
>>>>>>>> +}
>>>>>> There also looks to be a dead code concern here (towards the "nr"
>>>>>> parameters
>>>>>> here and elsewhere, when STATIC_SHM=n). Just that apparently we decided
>>>>>> to
>>>>>> leave out Misra rule 2.2 entirely.
>>>>> I think that I didn't get what is an issue when STATIC_SHM=n, functions
>>>>> is still
>>>>> going to be called through page_get_owner_and_reference(), at least, in
>>>>> page_alloc.c .
>>>> Yes, but will "nr" ever be anything other than 1 then? IOW omitting the
>>>> parameter
>>>> would be fine. And that's what "dead code" is about.
>>> Got it.
>>>
>>> So we don't have any SAF-x tag to mark this function as safe. What is the
>>> best one
>>> solution for now if nr argument will be needed in the future for
>>> STATIC_SHM=y?
>> Add the parameter at that point. Just like was done for Arm.
>
> Hmm, it seems like I am confusing something... Arm has the same defintion and
> declaration
> of page_get_owner_and_nr_reference().
But it didn't always have it. And there is at least one pending issue there.
Hence my request to use the simpler variant until someone actually makes
STATIC_SHM work on RISC-V. And hopefully by then the issue in Arm code is
sorted, and you can clone the code without raising questions.
Jan
