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