Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings

From: "Chen, Tiejun" <tiejun.chen@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: kevin.tian@intel.com, ian.campbell@citrix.com,
	stefano.stabellini@eu.citrix.com,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	ian.jackson@eu.citrix.com, xen-devel@lists.xen.org,
	yang.z.zhang@intel.com
Subject: Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings
Date: Thu, 28 Aug 2014 15:29:03 +0800	[thread overview]
Message-ID: <53FEDA3F.80008@intel.com> (raw)
In-Reply-To: <53FED802.7080707@intel.com>

On 2014/8/28 15:19, Chen, Tiejun wrote:
> On 2014/8/28 15:09, Chen, Tiejun wrote:
>> On 2014/8/28 14:50, Jan Beulich wrote:
>>>>>> On 28.08.14 at 04:24, <tiejun.chen@intel.com> wrote:
>>>> If you guys have no more comments, could I send a new series to review?
>>>
>>> You certainly can do so at any time, but as said before I didn't get
>>> to looking at the current version yet; briefly having looked at the
>>
>> I knew this point so this is just why here I's like to ask if I can send
>> new revision. I hope I can do better as you expect.
>>
>>> first two patches I'm already pretty convinced that the structuring
>>> still isn't right (you shouldn't be exposing VT-d internals into
>>
>> If you have any comment, I think you can point inline, then I can take a
>> look at that to improve or fix anything as you expect.
>>
>> As you know, I'm not familiar with Xen codes so sometimes I can't
>> understand what you mean properly, even what you were saying. So I have
>> to ask you to explain explicitly again.
>>
>>> arbitrary parts of the hypervisor, but rather introduce a new
>
> As I remember you or Andrew told me not to use acpi_rmrr_units directly,
> instead I previously introduced a new array to store such RMRR info.
>

"
... this once again stresses what I stated previously: Piggybacking
on the E820 handling here is just the wrong approach. There's
really no correlation with E820 other than us wanting to use the
gathered information for (among other things) adjusting the guest
E820 table. But that doesn't in any way require any re-use of
non-suitable data structures.

In fact I don't see the need for this first patch anyway, as RMRRs
are already being put on a linked list as they get found. I.e. the
patch here just clones existing information.

Jan
"

"
 >> Maintain a global count as entries are added/removed from this list.  It
 >> is a waste of time recounting this list for each hypercall.
 >
 > Are you saying push this 'nr_entries' as a global count somewhere? I
 > guess I can set this when we first construct acpi_rmrr_units in ACPI
 > stuff.

Not named "nr_entries", but yes.  It is constant after boot.

"

Thanks
Tiejun

> Current first two patches are just introduced from you and Andrew's
> comments.
>
> Thanks
> Tiejun
>
>>> vendor IOMMU hook/actor to get at the reserved page information
>>> - this all still goes along the lines of you apparently not
>>> understanding that a reasonable level of abstraction helps in the
>>> long term maintenance of such code).
>>
>> If you always say I'm wrong to understand rather than show me something
>> explicitly, its still difficult to fine such codes for a fresh man.
>>
>> Thanks
>> Tiejun
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
>
>