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: Mon, 01 Sep 2014 17:44:10 +0800 Message-ID: <54043FEA.6080100@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> <53FFED5D.7040406@intel.com> <54006196020000780002EFC6@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: <54006196020000780002EFC6@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/29 17:18, Jan Beulich wrote: >>>> On 29.08.14 at 05:02, wrote: >> 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? > > This still allocates another instance of structures to create a second > linked list. Did you consider get_device_reserved_memory() to take Do you mean we still use this existing type combo, acpi_rmrr_units and acpi_rmrr_units? > a callback function instead? > But we should do something like this, 1. .get_device_reserved_memory = get_drm_all, 2. static int get_drm_all(struct list_head *dev_reserved_memory) { return (get_drm_callback(dev_reserved_memory)); } 3. get_drm_callback = get_device_acpi_reserved_memory; 4. static int get_device_acpi_reserved_memory(struct list_head *dev_reserved_memory) { ... dev_reserved_memory = &acpi_rmrr_units; ... } Then while calling the hypercall, struct list_head *dev_reserved_memory = NULL; nr_entries = ops->get_device_reserved_memory(dev_reserved_memory); if (!nr_entries) list_for_each_entry( darm, dev_reserved_memory, list ) { xxx.start_pfn = ...; xxx.nr_pages = ...; if ( copy_to_guest_offset(buffer, i, &xxx, 1) ) ... } Thanks Tiejun