From: Oleksandr <olekstysh@gmail.com>
To: Stefano Stabellini <sstabellini@kernel.org>
Cc: xen-devel@lists.xenproject.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org,
Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>,
Russell King <linux@armlinux.org.uk>,
Boris Ostrovsky <boris.ostrovsky@oracle.com>,
Juergen Gross <jgross@suse.com>, Julien Grall <julien@xen.org>
Subject: Re: [PATCH V2 4/4] arm/xen: Read extended regions from DT and init Xen resource
Date: Sat, 20 Nov 2021 15:38:59 +0200 [thread overview]
Message-ID: <a7a5b5c1-15c0-df58-c0e5-67358f92d7d1@gmail.com> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2111191820130.1412361@ubuntu-linux-20-04-desktop>
On 20.11.21 04:36, Stefano Stabellini wrote:
Hi Stefano
> On Fri, 19 Nov 2021, Oleksandr wrote:
>> On 19.11.21 03:19, Stefano Stabellini wrote:
>>> On Wed, 10 Nov 2021, Oleksandr wrote:
>>>> On 28.10.21 04:40, Stefano Stabellini wrote:
>>>>
>>>> Hi Stefano
>>>>
>>>> I am sorry for the late response.
>>>>
>>>>> On Tue, 26 Oct 2021, Oleksandr Tyshchenko wrote:
>>>>>> From: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>>
>>>>>> This patch implements arch_xen_unpopulated_init() on Arm where
>>>>>> the extended regions (if any) are gathered from DT and inserted
>>>>>> into passed Xen resource to be used as unused address space
>>>>>> for Xen scratch pages by unpopulated-alloc code.
>>>>>>
>>>>>> The extended region (safe range) is a region of guest physical
>>>>>> address space which is unused and could be safely used to create
>>>>>> grant/foreign mappings instead of wasting real RAM pages from
>>>>>> the domain memory for establishing these mappings.
>>>>>>
>>>>>> The extended regions are chosen by the hypervisor at the domain
>>>>>> creation time and advertised to it via "reg" property under
>>>>>> hypervisor node in the guest device-tree. As region 0 is reserved
>>>>>> for grant table space (always present), the indexes for extended
>>>>>> regions are 1...N.
>>>>>>
>>>>>> If arch_xen_unpopulated_init() fails for some reason the default
>>>>>> behaviour will be restored (allocate xenballooned pages).
>>>>>>
>>>>>> This patch also removes XEN_UNPOPULATED_ALLOC dependency on x86.
>>>>>>
>>>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>>>>>> ---
>>>>>> Changes RFC -> V2:
>>>>>> - new patch, instead of
>>>>>> "[RFC PATCH 2/2] xen/unpopulated-alloc: Query hypervisor to
>>>>>> provide
>>>>>> unallocated space"
>>>>>> ---
>>>>>> arch/arm/xen/enlighten.c | 112
>>>>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>> drivers/xen/Kconfig | 2 +-
>>>>>> 2 files changed, 113 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
>>>>>> index dea46ec..1a1e0d3 100644
>>>>>> --- a/arch/arm/xen/enlighten.c
>>>>>> +++ b/arch/arm/xen/enlighten.c
>>>>>> @@ -62,6 +62,7 @@ static __read_mostly unsigned int xen_events_irq;
>>>>>> static phys_addr_t xen_grant_frames;
>>>>>> #define GRANT_TABLE_INDEX 0
>>>>>> +#define EXT_REGION_INDEX 1
>>>>>> uint32_t xen_start_flags;
>>>>>> EXPORT_SYMBOL(xen_start_flags);
>>>>>> @@ -303,6 +304,117 @@ static void __init xen_acpi_guest_init(void)
>>>>>> #endif
>>>>>> }
>>>>>> +#ifdef CONFIG_XEN_UNPOPULATED_ALLOC
>>>>>> +int arch_xen_unpopulated_init(struct resource *res)
>>>>>> +{
>>>>>> + struct device_node *np;
>>>>>> + struct resource *regs, *tmp_res;
>>>>>> + uint64_t min_gpaddr = -1, max_gpaddr = 0;
>>>>>> + unsigned int i, nr_reg = 0;
>>>>>> + struct range mhp_range;
>>>>>> + int rc;
>>>>>> +
>>>>>> + if (!xen_domain())
>>>>>> + return -ENODEV;
>>>>>> +
>>>>>> + np = of_find_compatible_node(NULL, NULL, "xen,xen");
>>>>>> + if (WARN_ON(!np))
>>>>>> + return -ENODEV;
>>>>>> +
>>>>>> + /* Skip region 0 which is reserved for grant table space */
>>>>>> + while (of_get_address(np, nr_reg + EXT_REGION_INDEX, NULL,
>>>>>> NULL))
>>>>>> + nr_reg++;
>>>>>> + if (!nr_reg) {
>>>>>> + pr_err("No extended regions are found\n");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + regs = kcalloc(nr_reg, sizeof(*regs), GFP_KERNEL);
>>>>>> + if (!regs)
>>>>>> + return -ENOMEM;
>>>>>> +
>>>>>> + /*
>>>>>> + * Create resource from extended regions provided by the
>>>>>> hypervisor to
>>>>>> be
>>>>>> + * used as unused address space for Xen scratch pages.
>>>>>> + */
>>>>>> + for (i = 0; i < nr_reg; i++) {
>>>>>> + rc = of_address_to_resource(np, i + EXT_REGION_INDEX,
>>>>>> ®s[i]);
>>>>>> + if (rc)
>>>>>> + goto err;
>>>>>> +
>>>>>> + if (max_gpaddr < regs[i].end)
>>>>>> + max_gpaddr = regs[i].end;
>>>>>> + if (min_gpaddr > regs[i].start)
>>>>>> + min_gpaddr = regs[i].start;
>>>>>> + }
>>>>>> +
>>>>>> + /* Check whether the resource range is within the hotpluggable
>>>>>> range
>>>>>> */
>>>>>> + mhp_range = mhp_get_pluggable_range(true);
>>>>>> + if (min_gpaddr < mhp_range.start)
>>>>>> + min_gpaddr = mhp_range.start;
>>>>>> + if (max_gpaddr > mhp_range.end)
>>>>>> + max_gpaddr = mhp_range.end;
>>>>>> +
>>>>>> + res->start = min_gpaddr;
>>>>>> + res->end = max_gpaddr;
>>>>>> +
>>>>>> + /*
>>>>>> + * Mark holes between extended regions as unavailable. The
>>>>>> rest of
>>>>>> that
>>>>>> + * address space will be available for the allocation.
>>>>>> + */
>>>>>> + for (i = 1; i < nr_reg; i++) {
>>>>>> + resource_size_t start, end;
>>>>>> +
>>>>>> + start = regs[i - 1].end + 1;
>>>>>> + end = regs[i].start - 1;
>>>>>> +
>>>>>> + if (start > (end + 1)) {
>>>>> Should this be:
>>>>>
>>>>> if (start >= end)
>>>>>
>>>>> ?
>>>> Yes, we can do this here (since the checks are equivalent) but ...
>>>>
>>>>>> + rc = -EINVAL;
>>>>>> + goto err;
>>>>>> + }
>>>>>> +
>>>>>> + /* There is no hole between regions */
>>>>>> + if (start == (end + 1))
>>>>> Also here, shouldn't it be:
>>>>>
>>>>> if (start == end)
>>>>>
>>>>> ?
>>>> ... not here.
>>>>
>>>> As
>>>>
>>>> "(start == (end + 1))" is equal to "(regs[i - 1].end + 1 ==
>>>> regs[i].start)"
>>>>
>>>> but
>>>>
>>>> "(start == end)" is equal to "(regs[i - 1].end + 1 == regs[i].start - 1)"
>>> OK. But the check:
>>>
>>> if (start >= end)
>>>
>>> Actually covers both cases so that's the only check we need?
>> Sorry, I don't entirely understand the question.
>> Is the question to use only a single check in that loop?
>>
>> Paste the updated code which I have locally for the convenience.
>>
>> [snip]
>>
>> /*
>> * Mark holes between extended regions as unavailable. The rest of that
>> * address space will be available for the allocation.
>> */
>> for (i = 1; i < nr_reg; i++) {
>> resource_size_t start, end;
>>
>> start = regs[i - 1].end + 1;
>> end = regs[i].start - 1;
>>
>> if (start > (end + 1)) {
>> rc = -EINVAL;
>> goto err;
>> }
>>
>> /* There is no hole between regions */
>> if (start == (end + 1))
>> continue;
>>
>> tmp_res = kzalloc(sizeof(*tmp_res), GFP_KERNEL);
>> if (!tmp_res) {
>> rc = -ENOMEM;
>> goto err;
>> }
>>
>> tmp_res->name = "Unavailable space";
>> tmp_res->start = start;
>> tmp_res->end = end;
>>
>> rc = insert_resource(&xen_resource, tmp_res);
>> if (rc) {
>> pr_err("Cannot insert resource %pR (%d)\n", tmp_res, rc);
>> kfree(tmp_res);
>> goto err;
>> }
>> }
>>
>> [snip]
>>
>>
>> 1. The first check is to detect an overlap (which is a wrong configuration,
>> correct?) and bail out if true (for example, regX: 0x81000000...0x82FFFFFF and
>> regY: 0x82000000...0x83FFFFFF).
>> 2. The second check is just to skip current iteration as there is no
>> space/hole between regions (for example, regX: 0x81000000...0x82FFFFFF and
>> regY: 0x83000000...0x83FFFFFF).
>> Therefore I think they should be distinguished.
>>
>> Yes, both check could be transformed to a single one, but this way the
>> overlaps will be ignored:
>> if (start >= (end + 1))
>> continue;
>>
>> Or I really missed something?
> You are right it is better to distinguish the two cases. I suggest the
> code below because I think it is a clearer, even if it might be slightly
> less efficient. I don't feel too strongly about it though.
>
> resource_size_t start, end;
>
> /* There is no hole between regions */
> if ( regs[i - 1].end + 1 == regs[i].start )
> continue;
>
> if ( regs[i - 1].end + 1 > regs[i].start) {
> rc = -EINVAL;
> goto err;
> }
>
> start = regs[i - 1].end + 1;
> end = regs[i].start - 1;
OK, let's make code clearer, will do.
--
Regards,
Oleksandr Tyshchenko
prev parent reply other threads:[~2021-11-20 13:39 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-26 16:05 [PATCH V2 0/4] xen: Add support of extended regions (safe ranges) on Arm Oleksandr Tyshchenko
2021-10-26 16:05 ` [PATCH V2 1/4] xen/unpopulated-alloc: Drop check for virt_addr_valid() in fill_list() Oleksandr Tyshchenko
2021-10-28 18:57 ` Boris Ostrovsky
2021-10-26 16:05 ` [PATCH V2 2/4] arm/xen: Switch to use gnttab_setup_auto_xlat_frames() for DT Oleksandr Tyshchenko
2021-10-28 1:28 ` Stefano Stabellini
2021-11-10 22:14 ` Oleksandr
2021-11-19 0:32 ` Stefano Stabellini
2021-11-19 18:25 ` Oleksandr
2021-10-26 16:05 ` [PATCH V2 3/4] xen/unpopulated-alloc: Add mechanism to use Xen resource Oleksandr Tyshchenko
2021-10-28 16:37 ` Stefano Stabellini
2021-11-09 18:34 ` Oleksandr
2021-11-19 0:59 ` Stefano Stabellini
2021-11-19 18:18 ` Oleksandr
2021-11-20 2:19 ` Stefano Stabellini
2021-11-23 16:46 ` Oleksandr
2021-11-23 21:25 ` Stefano Stabellini
2021-11-24 9:33 ` Oleksandr
2021-11-24 5:16 ` Juergen Gross
2021-11-24 9:37 ` Oleksandr
2021-10-28 19:08 ` Boris Ostrovsky
2021-11-09 18:51 ` Oleksandr
2021-10-26 16:05 ` [PATCH V2 4/4] arm/xen: Read extended regions from DT and init " Oleksandr Tyshchenko
2021-10-28 1:40 ` Stefano Stabellini
2021-11-10 20:21 ` Oleksandr
2021-11-19 1:19 ` Stefano Stabellini
2021-11-19 20:23 ` Oleksandr
2021-11-20 2:36 ` Stefano Stabellini
2021-11-20 13:38 ` Oleksandr [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a7a5b5c1-15c0-df58-c0e5-67358f92d7d1@gmail.com \
--to=olekstysh@gmail.com \
--cc=boris.ostrovsky@oracle.com \
--cc=jgross@suse.com \
--cc=julien@xen.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=oleksandr_tyshchenko@epam.com \
--cc=sstabellini@kernel.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).