Re: [PATCH V2 2/3] xen/arm: Add handling of extended regions for Dom0

[Oleksandr <olekstysh@gmail.com>]

On 21.09.21 02:21, Stefano Stabellini wrote:

Hi Stefano

> On Sun, 19 Sep 2021, Oleksandr wrote:
>>> On 18/09/2021 03:37, Stefano Stabellini wrote:
>>>> On Fri, 17 Sep 2021, Stefano Stabellini wrote:
>>>>> On Fri, 17 Sep 2021, Oleksandr wrote:
>>>>>>>> +
>>>>>>>> +    dt_dprintk("Find unallocated memory for extended regions\n");
>>>>>>>> +
>>>>>>>> +    unalloc_mem = rangeset_new(NULL, NULL, 0);
>>>>>>>> +    if ( !unalloc_mem )
>>>>>>>> +        return -ENOMEM;
>>>>>>>> +
>>>>>>>> +    /* Start with all available RAM */
>>>>>>>> +    for ( i = 0; i < bootinfo.mem.nr_banks; i++ )
>>>>>>>> +    {
>>>>>>>> +        start = bootinfo.mem.bank[i].start;
>>>>>>>> +        end = bootinfo.mem.bank[i].start +
>>>>>>>> bootinfo.mem.bank[i].size - 1;
>>>>>>>> +        res = rangeset_add_range(unalloc_mem, start, end);
>>>>>>>> +        if ( res )
>>>>>>>> +        {
>>>>>>>> +            printk(XENLOG_ERR "Failed to add:
>>>>>>>> %#"PRIx64"->%#"PRIx64"\n",
>>>>>>>> +                   start, end);
>>>>>>>> +            goto out;
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    /* Remove RAM assigned to Dom0 */
>>>>>>>> +    for ( i = 0; i < assign_mem->nr_banks; i++ )
>>>>>>>> +    {
>>>>>>>> +        start = assign_mem->bank[i].start;
>>>>>>>> +        end = assign_mem->bank[i].start +
>>>>>>>> assign_mem->bank[i].size - 1;
>>>>>>>> +        res = rangeset_remove_range(unalloc_mem, start, end);
>>>>>>>> +        if ( res )
>>>>>>>> +        {
>>>>>>>> +            printk(XENLOG_ERR "Failed to remove:
>>>>>>>> %#"PRIx64"->%#"PRIx64"\n",
>>>>>>>> +                   start, end);
>>>>>>>> +            goto out;
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    /* Remove reserved-memory regions */
>>>>>>>> +    for ( i = 0; i < bootinfo.reserved_mem.nr_banks; i++ )
>>>>>>>> +    {
>>>>>>>> +        start = bootinfo.reserved_mem.bank[i].start;
>>>>>>>> +        end = bootinfo.reserved_mem.bank[i].start +
>>>>>>>> +            bootinfo.reserved_mem.bank[i].size - 1;
>>>>>>>> +        res = rangeset_remove_range(unalloc_mem, start, end);
>>>>>>>> +        if ( res )
>>>>>>>> +        {
>>>>>>>> +            printk(XENLOG_ERR "Failed to remove:
>>>>>>>> %#"PRIx64"->%#"PRIx64"\n",
>>>>>>>> +                   start, end);
>>>>>>>> +            goto out;
>>>>>>>> +        }
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    /* Remove grant table region */
>>>>>>>> +    start = kinfo->gnttab_start;
>>>>>>>> +    end = kinfo->gnttab_start + kinfo->gnttab_size - 1;
>>>>>>>> +    res = rangeset_remove_range(unalloc_mem, start, end);
>>>>>>>> +    if ( res )
>>>>>>>> +    {
>>>>>>>> +        printk(XENLOG_ERR "Failed to remove:
>>>>>>>> %#"PRIx64"->%#"PRIx64"\n",
>>>>>>>> +               start, end);
>>>>>>>> +        goto out;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    start = EXT_REGION_START;
>>>>>>>> +    end = min((1ULL << p2m_ipa_bits) - 1, EXT_REGION_END);
>>>>>>>> +    res = rangeset_report_ranges(unalloc_mem, start, end,
>>>>>>>> +                                 add_ext_regions, ext_regions);
>>>>>>>> +    if ( res )
>>>>>>>> +        ext_regions->nr_banks = 0;
>>>>>>>> +    else if ( !ext_regions->nr_banks )
>>>>>>>> +        res = -ENOENT;
>>>>>>>> +
>>>>>>>> +out:
>>>>>>>> +    rangeset_destroy(unalloc_mem);
>>>>>>>> +
>>>>>>>> +    return res;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int __init find_memory_holes(const struct kernel_info
>>>>>>>> *kinfo,
>>>>>>>> +                                    struct meminfo *ext_regions)
>>>>>>>> +{
>>>>>>>> +    struct dt_device_node *np;
>>>>>>>> +    struct rangeset *mem_holes;
>>>>>>>> +    paddr_t start, end;
>>>>>>>> +    unsigned int i;
>>>>>>>> +    int res;
>>>>>>>> +
>>>>>>>> +    dt_dprintk("Find memory holes for extended regions\n");
>>>>>>>> +
>>>>>>>> +    mem_holes = rangeset_new(NULL, NULL, 0);
>>>>>>>> +    if ( !mem_holes )
>>>>>>>> +        return -ENOMEM;
>>>>>>>> +
>>>>>>>> +    /* Start with maximum possible addressable physical memory
>>>>>>>> range */
>>>>>>>> +    start = EXT_REGION_START;
>>>>>>>> +    end = min((1ULL << p2m_ipa_bits) - 1, EXT_REGION_END);
>>>>>>>> +    res = rangeset_add_range(mem_holes, start, end);
>>>>>>>> +    if ( res )
>>>>>>>> +    {
>>>>>>>> +        printk(XENLOG_ERR "Failed to add:
>>>>>>>> %#"PRIx64"->%#"PRIx64"\n",
>>>>>>>> +               start, end);
>>>>>>>> +        goto out;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    /* Remove all regions described by "reg" property (MMIO, RAM,
>>>>>>>> etc) */
>>>>>>> Well... The loop below is not going to handle all the regions
>>>>>>> described in
>>>>>>> the property "reg". Instead, it will cover a subset of "reg" where
>>>>>>> the
>>>>>>> memory is addressable.
>>>>>> As I understand, we are only interested in subset of "reg" where the
>>>>>> memory is
>>>>>> addressable.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> You will also need to cover "ranges" that will describe the BARs for
>>>>>>> the PCI
>>>>>>> devices.
>>>>>> Good point.
>>>>> Yes, very good point!
>>>>>
>>>>>
>>>>>> Could you please clarify how to recognize whether it is a PCI
>>>>>> device as long as PCI support is not merged? Or just to find any
>>>>>> device nodes
>>>>>> with non-empty "ranges" property
>>>>>> and retrieve addresses?
>>>>> Normally any bus can have a ranges property with the aperture and
>>>>> possible address translations, including /amba (compatible =
>>>>> "simple-bus"). However, in these cases dt_device_get_address already
>>>>> takes care of it, see xen/common/device_tree.c:dt_device_get_address.
>>>>>
>>>>> The PCI bus is special for 2 reasons:
>>>>> - the ranges property has a different format
>>>>> - the bus is hot-pluggable
>>>>>
>>>>> So I think the only one that we need to treat specially is PCI.
>>>>>
>>>>> As far as I am aware PCI is the only bus (or maybe just the only bus
>>>>> that we support?) where ranges means the aperture.
>>>> Now that I think about this, there is another "hotpluggable" scenario we
>>>> need to think about:
>>>>
>>>> [1] https://marc.info/?l=xen-devel&m=163056546214978
>>>>
>>>> Xilinx devices have FPGA regions with apertures currently not described
>>>> in device tree, where things can programmed in PL at runtime making new
>>>> devices appear with new MMIO regions out of thin air.
>>>>
>>>> Now let me start by saying that yes, the entire programmable region
>>>> aperture could probably be described in device tree, however, in
>>>> reality it is not currently done in any of the device trees we use
>>>> (including the upstream device trees in linux.git).
>>> This is rather annoying, but not unheard. There are a couple of platforms
>>> where the MMIOs are not fully described in the DT.
>>>
>>> In fact, we have a callback 'specific_mappings' which create additional
>>> mappings (e.g. on the omap5) for dom0.
>>>
>>>> So, we have a problem :-(
>>>>
>>>>
>>>> I can work toward getting the right info on device tree, but in reality
>>>> that is going to take time and for now the device tree doesn't have the
>>>> FPGA aperture in it. So if we accept this series as is, it is going to
>>>> stop features like [1] from working. >
>>>> If we cannot come up with any better plans, I think it would be better
>>>> to drop find_memory_holes, only rely on find_unallocated_memory even
>>>> when the IOMMU is on. One idea is that we could add on top of the
>>>> regions found by find_unallocated_memory any MMIO regions marked as
>>>> xen,passthrough: they are safe because they are not going to dom0 anyway.
>>> (Oleksandr, it looks like some rationale about the different approach is
>>> missing in the commit message. Can you add it?)
>> Yes sure, but let me please clarify what is different approach in this
>> context. Is it to *also* take into the account MMIO regions of the devices for
>> passthrough for case when IOMMU is off (in addition to unallocated memory)? If
>> yes, I wonder whether we will gain much with that according to that device's
>> MMIO regions are usually not big enough and we stick to allocate extended
>> regions with bigger size (> 64MB).
> That's fair enough. There are a couple of counter examples where the
> MMIO regions for the device to assign are quite large, for instance a
> GPU, Xilinx AIEngine, or the PCIe Root Complex with the entire aperture,
> but maybe they are not that common. I am not sure if it is worth
> scanning the tree for xen,passthrough regions every time at boot for
> this.

ok, I will add a few sentences to commit message about this different 
approach for now. At least this could be implemented later on if there 
is a need.


-- 
Regards,

Oleksandr Tyshchenko