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

[Luca Fancellu <Luca.Fancellu@xxxxxxx>]

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:

/*
 * 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.
 * 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