From: "Jan Beulich" Subject: Re: [v7][RFC][PATCH 06/13] hvmloader/ram: check if guest memory is out of reserved device memory maps Date: Thu, 20 Nov 2014 08:59:18 +0000 Message-ID: <546DBB760200007800049372@mail.emea.novell.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.com> <545A57AD02000078000C1037@mail.emea.novell.com> <545B3F4A.5070808@intel.com> <545B562F02000078000453FB@mail.emea.novell.com> <545C9E97.4040800@intel.com> <545CB64E02000078000459CD@mail.emea.novell.com> <5461AD94.2070008@intel.com> <5461BF97.1070709@intel.com> <5461DED50200007800046520@mail.emea.novell.com> <5461DFAF020000780004652B@mail.emea.novell.com> <5461DA23.6020105@intel.com> <5462CE68.6010709@intel.com> <54632EA80200007800046AE5@mail.emea.novell.com> <5469AA77.2070602@intel.com> <5469D68D0200007800048515@mail.emea.novell.com> <5469D749.2040807@intel.com> <5469E74302000078000485B0@mail.emea.novell.com> <5469DCD7.4030701@intel.com> <5469EF5D0200007800048609@mail.emea.novell.com> <546AB82D.5080305@intel.com> <546B0AF00200007800048A69@mail.emea.novell.com> <546B004B.6050603@intel.com> <546B20910200007800048AC0@mail.emea.novell.com> <546BF1AD.4010801@intel.com> <546DA6CB02000078000492C5@mail.emea.novell.com> <546DA259.10609@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <546DA259.10609@intel.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tiejun Chen Cc: yang.z.zhang@intel.com, kevin.tian@intel.com, tim@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org >>> On 20.11.14 at 09:12, wrote: > On 2014/11/20 15:31, Jan Beulich wrote: >>>>> On 19.11.14 at 02:26, wrote: >>> int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt) >>> { >>> struct acpi_rmrr_unit *rmrr; >>> int rc = 0; >>> unsigned int i; >>> u16 bdf; >>> >>> for_each_rmrr_device ( rmrr, bdf, i ) >>> { >>> rc = func(PFN_DOWN(rmrr->base_address), >>> PFN_UP(rmrr->end_address) - >>> PFN_DOWN(rmrr->base_address), >>> PCI_SBDF(rmrr->segment, bdf), >>> ctxt); >>> /* Hit this entry so just go next. */ >>> if ( rc == 1 ) >>> i = rmrr->scope.devices_cnt; >>> else if ( rc < 0 ) >>> return rc; >>> } >>> >>> return rc; >>> } >> >> Better. Another improvement would be make it not depend on the >> internal workings of for_each_rmrr_device()... And in any case you > > Yes but as you see, > > #define for_each_rmrr_device(rmrr, bdf, idx) \ > list_for_each_entry(rmrr, &acpi_rmrr_units, list) \ > /* assume there never is a bdf == 0 */ \ > for (idx = 0; (bdf = rmrr->scope.devices[idx]) && \ > idx < rmrr->scope.devices_cnt; idx++) > > So, > for_each_rmrr_device ( rmrr, bdf, i ) > { > rc = func(...); > /* Hit this entry so just go next. */ > if ( rc > 0 ) > i = rmrr->scope.devices_cnt; > > If you don't intend to reset something of this internal working, its > hard to go next rmrr entry. Maybe you already have idea, so could you > give me some hints? Have a second struct acpi_rmrr_unit pointer, starting out as NULL and getting set to the current one when the callback returns a positive value. Skip further iterations as long as both pointers match. >> should not special case 1 - just return when rc is negative and skip >> the rest of the current RMRR when it's positive. And of course make >> the function's final return value predictable. >> > > Like this, > > /* Hit this entry so just go next. */ > if ( rc > 0 ) > xxxx; > else if ( rc < 0 ) > return rc; > } Yes, albeit swapping the order (and dropping the "else" along with adding unlikely() to the error case) would be preferred. Jan