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