Re: [PATCH v2 10/13] xen/arm: remove shm holes from extended regions

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Luca,

On 10/04/2024 16:08, Luca Fancellu wrote:
> 
> 
> Hi Michal,
> 
>>>
>>> For direct-map domain with iommu on, after we get guest shm info from 
>>> "kinfo",
>>> we use "remove_shm_from_rangeset" to remove static shm.
>>>
>>> For direct-map domain with iommu off, as static shm has already been taken
>>> care of through reserved memory banks, we do nothing.
>> Stale info given that shmem is no longer part of reserved memory banks. It's 
>> been taken care
>> of by removing shmem regions in find_unallocated_memory()
> 
> Sure, will amend for this:
> 
>>>
>>> +int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
>>> +                                    struct rangeset *rangeset)
>>> +{
>>> +    const struct membanks *shm_mem = &kinfo->shm_mem.common;
>>> +    unsigned int i;
>>> +
>>> +    /* Remove static shared memory regions */
>>> +    for ( i = 0; i < shm_mem->nr_banks; i++ )
>>> +    {
>>> +        paddr_t start, end;
>>> +        int res;
>>> +
>>> +        start = shm_mem->bank[i].start;
>>> +        end = shm_mem->bank[i].start + shm_mem->bank[i].size - 1;
>> If you look at other rangeset_remove_range use cases and error messages, 1 
>> is subtracted
>> in PFN_DOWN() so that the error message contains end unchanged. Please 
>> adhere to that so that
>> printed messages are consistent.
> 
> Yes I will change it to have -1 inside PFN_DOWN(), here and in the other 
> occurrences
>>>
>>> +    /* Remove static shared memory regions */
>>> +    res = remove_shm_from_rangeset(kinfo, guest_holes);
>>> +    if ( res )
>>> +        goto out;
>>> +
>> Could you please add a comment explaining here what's done below?
> 
> Is it ok something like this:
I'm ok with your proposal in the other e-mail.

> 
> /*
>  * Take the interval of memory starting from the first extended region bank
>  * start address and ending to the end of the last extended region bank.
I would stop here. The rest reads quite difficult.

>  * The interval will be passed to rangeset_report_ranges to allow it to
>  * create, by the add_ext_regions callback, a set of extended memory region
>  * banks from the guest_holes rangeset, which contains the original extended
>  * memory region ranges where the static shared memory ranges are carved
>  * out.
>  */
> 
>>> +    i = ext_regions->nr_banks - 1;
>>> +    start = ext_regions->bank[0].start;
>>> +    end = ext_regions->bank[i].start + ext_regions->bank[i].size - 1;
>>> +
>>> +    /* Reset original extended regions to hold new value */
>>> +    ext_regions->nr_banks = 0;
>>> +    res = rangeset_report_ranges(guest_holes, PFN_DOWN(start), 
>>> PFN_DOWN(end),
>>> +                                 add_ext_regions, ext_regions);
>>> +    if ( res )
>>> +        ext_regions->nr_banks = 0;
>>> +    else if ( !ext_regions->nr_banks )
>>> +        res = -ENOENT;
>>> +
>>> + out:
>>> +    rangeset_destroy(guest_holes);
>>> +
>>> +    return res;
>>> +}
>>> +
>>> /*
>>>  * Local variables:
>>>  * mode: C
>>> --
>>> 2.34.1
>>>
>>
>> ~Michal
>>
> 
> Cheers,
> Luca

~Michal