Message-Id: <5A3944B40200007800198827@prv-mh.provo.novell.com>
Date: Tue, 19 Dec 2017 08:56:20 -0700
From: "Jan Beulich" <JBeulich@suse.com>
To: "Boris Ostrovsky" <boris.ostrovsky@oracle.com>
Cc: <christian.koenig@amd.com>, <helgaas@kernel.org>,
        <xen-devel@lists.xen.org>, "Juergen Gross" <jgross@suse.com>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [Xen-devel] [PATCH v2] xen/balloon: Mark unallocated host
 memory as UNUSABLE
References: <1513635771-11779-1-git-send-email-boris.ostrovsky@oracle.com>
 <5A38DA840200007800198579@prv-mh.provo.novell.com>
 <063fdabc-5d9a-5137-9f27-3289488a3e44@oracle.com>
 <5A3932D402000078001987C5@prv-mh.provo.novell.com>
 <78016d07-d4dc-e6d1-fba2-995d2299bd8f@oracle.com>
In-Reply-To: <78016d07-d4dc-e6d1-fba2-995d2299bd8f@oracle.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 8BIT
Content-Disposition: inline
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2295
Lines: 57

>>> On 19.12.17 at 16:03, <boris.ostrovsky@oracle.com> wrote:
> On 12/19/2017 09:40 AM, Jan Beulich wrote:
>>>>> On 19.12.17 at 15:25, <boris.ostrovsky@oracle.com> wrote:
>>> On 12/19/2017 03:23 AM, Jan Beulich wrote:
>>>>> +	memmap.nr_entries = ARRAY_SIZE(xen_e820_table->entries);
>>>> Is it really reasonable to have a static upper bound here? As we
>>>> know especially EFI systems can come with a pretty scattered
>>>> (pseudo) E820 table. Even if (iirc) this has a static upper bound
>>>> right now in the hypervisor too, it would be nice if the kernel
>>>> didn't need further changes once the hypervisor is being made
>>>> more flexible.
>>> This is how we obtain the map in xen_memory_setup(). Are you suggesting
>>> that we should query for the size first?
>> That would be better, I think.
> 
> 
> I think we will first need to fix xen_memory_setup() to do that too and
> that would be a separate patch.
> 
> I am also not clear how this will work on earlier version of the
> hypervisor that didn't support querying for size. From what I am seeing
> in 4.4 we will get -EFAULT if the buffer is NULL.

That's not nice, I agree, but can be dealt with.

>>>>> +	/* Mark non-RAM regions as not available. */
>>>>> +	for (; i < memmap.nr_entries; i++) {
>>>>> +		entry = &xen_e820_table->entries[i];
>>>>> +
>>>>> +		if (entry->type == E820_TYPE_RAM)
>>>>> +			continue;
>>>> I can't seem to match up this with ...
>>>>
>>>>> +		if (entry->addr >= hostmem_resource->end)
>>>>> +			break;
>>>>> +
>>>>> +		res = kzalloc(sizeof(*res), GFP_KERNEL);
>>>>> +		if (!res)
>>>>> +			goto out;
>>>>> +
>>>>> +		res->name = "Host memory";
>>>> ... this. Do you mean != instead (with the comment ahead of the
>>>> loop also clarified, saying something like "host RAM regions which
>>>> aren't RAM for us")? And perhaps better "Host RAM"?
>>> Right, this is not memory but rather something else (and so "!=" is
>>> correct). "Unavailable host RAM"?
>> If you like to be even more specific than what I had suggested -
>> sure.
> 
> But did you want to have some changes in the preceding comment? Not sure
> I read your comment correctly.

Well, "non-RAM" is ambiguous in this context, so yes, I'd prefer it
to be clarified. Whether you use what I've suggested or something
else I don't care much.

Jan