From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v5][PATCH 03/10] xen:x86: define a new hypercall to get RMRR mappings Date: Fri, 29 Aug 2014 11:02:53 +0800 Message-ID: <53FFED5D.7040406@intel.com> References: <1409050980-21933-1-git-send-email-tiejun.chen@intel.com> <1409050980-21933-4-git-send-email-tiejun.chen@intel.com> <53FC774D.8000501@citrix.com> <53FC9BB1020000780002D986@mail.emea.novell.com> <53FD3669.3070705@intel.com> <53FD9C03020000780002DEFF@mail.emea.novell.com> <53FD86E9.4020205@intel.com> <53FE92C6.6050605@intel.com> <53FEED4F020000780002E76C@mail.emea.novell.com> <53FED5AF.7080803@intel.com> <53FED802.7080707@intel.com> <53FEF9F8020000780002E7E2@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53FEF9F8020000780002E7E2@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: kevin.tian@intel.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, Andrew Cooper , ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/8/28 15:44, Jan Beulich wrote: >>>> On 28.08.14 at 09:19, 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, 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. > > Duplicating information for no reason. Did you check whether adding > a new method to struct iommu_ops couldn't do what you want, at > once retaining proper isolation _and_ not duplicating anything? > I tried to figure out solution as you suggestion but I'd like show my draft design before post anything to review since please give some suggestions here: 1. In the xen/include/xen/iommu.h file, struct iommu_ops { ... int (*get_device_reserved_memory)(struct list_head *dev_reserved_memory); 2. In the xen/drivers/passthrough/vtd/iommu.c file, extern int get_device_acpi_reserved_memory(struct list_head *dev_reserved_memory); const struct iommu_ops intel_iommu_ops = { ... .get_device_reserved_memory = get_device_acpi_reserved_memory, 3. In the xen/drivers/passthrough/vtd/dmar.c file, struct list_head devices_reserved_memory = LIST_HEAD_INIT ( devices_reserved_memory ); int get_device_acpi_reserved_memory(struct list_head *dev_reserved_memory) { static unsigned int device_reserved_memory_entries = 0; static unsigned int check_done = 0; struct acpi_rmrr_unit *rmrru; struct device_acpi_reserved_memory *darm = NULL; dev_reserved_memory = &devices_reserved_memory; if ( check_done ) return device_reserved_memory_entries; else { list_for_each_entry(rmrru, &acpi_rmrr_units, list) { darm = xzalloc(struct device_acpi_reserved_memory); if ( !darm ) return -ENOMEM; darm->base_address = rmrru->base_address; darm->end_address = rmrru->end_address; list_add(&darm->list, &devices_reserved_memory); device_reserved_memory_entries++; } } check_done = 1; return device_reserved_memory_entries; } 4. In the xen/include/asm-x86/acpi.h file, +struct device_acpi_reserved_memory { + struct list_head list; + u64 base_address; + u64 end_address; +}; Here a couple of questions: 1. Here I introduce this struct device_acpi_reserved_memory to avoid exposing that existing structure and list acpi_rmrr_units struct acpi_rmrr_unit { struct dmar_scope scope; struct list_head list; u64 base_address; u64 end_address; u16 segment; u8 allow_all:1; }; Because: 1> Actually we just need two fields, base_address and end_address. 2> If reuse that structure, we still have to change some head files to make sure we can use this in other files like I did in original patch #1 you don't like. So what is your idea? 2. Based on your isolation policy, I don't expose acpi_rmrr_units directly. Instead, I will copy this to another list, devices_reserved_memory as I show above. Is this reasonable and expected? 3. If #1 and #2 are fine to you, the go the follows" struct device_acpi_reserved_memory *darm; int nr_entries = 0; unsigned int i = 0; struct list_head *dev_reserved_memory = NULL; const struct iommu_ops *ops = iommu_get_ops(); if ( ops->get_device_reserved_memory ) { nr_entries = ops->get_device_reserved_memory(dev_reserved_memory); if ( !nr_entries ) return -ENOENT; else if ( nr_entries < 0 ) return -EFAULT; } else return -ENOENT; } Any comments are appreciated. Thanks Tiejun