Re: [PATCH v12 03/12] xen/arm: permit non direct-mapped Dom0 construction

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.12.2024 13:08, Michal Orzel wrote:
> On 13/12/2024 17:28, Carlo Nonato wrote:
>> @@ -977,6 +1022,108 @@ out:
>>      return res;
>>  }
>>
>> +void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
>> +{
>> +    struct membanks *mem = kernel_info_get_mem(kinfo);
>> +    unsigned int i, nr_banks = GUEST_RAM_BANKS;
>> +    struct membanks *hwdom_free_mem = NULL;
>> +
>> +    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
>> +           /* Don't want format this as PRIpaddr (16 digit hex) */
>> +           (unsigned long)(kinfo->unassigned_mem >> 20), d);
>> +
>> +    mem->nr_banks = 0;
>> +    /*
>> +     * Use host memory layout for hwdom. Only case for this is when LLC 
>> coloring
>> +     * is enabled.
>> +     */
>> +    if ( is_hardware_domain(d) )
>> +    {
>> +        struct membanks *gnttab = xzalloc_flex_struct(struct membanks, 
>> bank, 1);
>> +        /*
>> +         * Exclude the following regions:
>> +         * 1) Remove reserved memory
>> +         * 2) Grant table assigned to Dom0
> Can we not mention 'Dom0'? In the future hwdom may not necessarily be dom0. 
> Especially that
> in other places you mention hwdom.
> 
>> +         */
>> +        const struct membanks *mem_banks[] = {
>> +            bootinfo_get_reserved_mem(),
>> +            gnttab,
>> +        };
>> +
>> +        ASSERT(llc_coloring_enabled);
> Remove this assert. There's nothing LLC special here and this could be 
> re-used in the future
> to provide non 1:1 hwdom.
> 
>> +
>> +        if ( !gnttab )
>> +            goto fail;
>> +
>> +        gnttab->nr_banks = 1;
>> +        gnttab->bank[0].start = kinfo->gnttab_start;
>> +        gnttab->bank[0].size = kinfo->gnttab_start + kinfo->gnttab_size;
> Incorrect. You assign to 'end' to'size'. It should simply be:
> gnttab->bank[0].size = kinfo->gnttab_size.
> 
>> +
>> +        hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
>> +                                             NR_MEM_BANKS);
>> +        if ( !hwdom_free_mem )
>> +            goto fail;
>> +
>> +        hwdom_free_mem->max_banks = NR_MEM_BANKS;
>> +
>> +        if ( find_unallocated_memory(kinfo, mem_banks, 
>> ARRAY_SIZE(mem_banks),
>> +                                     add_hwdom_free_regions, 
>> hwdom_free_mem) )
>> +            goto fail;
>> +
>> +        nr_banks = hwdom_free_mem->nr_banks;
>> +        xfree(gnttab);
>> +    }
>> +
>> +    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- 
>> )
>> +    {
>> +        paddr_t bank_start, bank_size;
>> +
>> +        if ( is_hardware_domain(d) )
>> +        {
>> +            bank_start = hwdom_free_mem->bank[i].start;
>> +            bank_size = hwdom_free_mem->bank[i].size;
>> +        }
>> +        else
>> +        {
>> +            const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>> +            const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>> +
>> +            if ( i >= GUEST_RAM_BANKS )
>> +                goto fail;
>> +
>> +            bank_start = bankbase[i];
>> +            bank_size = banksize[i];
>> +        }
>> +
>> +        bank_size = MIN(bank_size, kinfo->unassigned_mem);
>> +        if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(bank_start), 
>> bank_size) )
>> +            goto fail;
>> +    }
>> +
>> +    if ( kinfo->unassigned_mem )
>> +        goto fail;
>> +
>> +    for( i = 0; i < mem->nr_banks; i++ )
>> +    {
>> +        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" 
>> (%ldMB)\n",
>> +               d,
>> +               i,
>> +               mem->bank[i].start,
>> +               mem->bank[i].start + mem->bank[i].size,
>> +               /* Don't want format this as PRIpaddr (16 digit hex) */
>> +               (unsigned long)(mem->bank[i].size >> 20));
>> +    }
>> +
>> +    xfree(hwdom_free_mem);
>> +    return;
>> +
>> +fail:

Nit: Style (missing indentation).

Jan