From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" 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 16:12:09 +0800 Message-ID: <546DA259.10609@intel.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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <546DA6CB02000078000492C5@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: 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 2014/11/20 15:31, Jan Beulich wrote: >>>> On 19.11.14 at 02:26, wrote: >>>> So without lookuping devices[i], how can we call func() for each sbdf as >>>> you mentioned? >>> >>> You've got both rmrr and bdf in the body of for_each_rmrr_device(). >>> After all - as I said - you just open-coded it. >>> >> >> Yeah, so change this again, >> >> 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? > 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; } return 0; Thanks Tiejun