From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v8][PATCH 04/17] update the existing hypercall to support XEN_DOMCTL_set_rdm Date: Tue, 09 Dec 2014 17:35:33 +0800 Message-ID: <5486C265.9040205@intel.com> References: <1417425875-9634-1-git-send-email-tiejun.chen@intel.com> <1417425875-9634-5-git-send-email-tiejun.chen@intel.com> <548090BA020000780004CCE6@mail.emea.novell.com> <54854F2D.6060204@intel.com> <54857491020000780004D9D6@mail.emea.novell.com> <5486A8FB.4080402@intel.com> <5486BE99020000780004DF8A@mail.emea.novell.com> <5486BCE8.3010900@intel.com> <5486CD25020000780004E026@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: <5486CD25020000780004E026@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, wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, tim@xen.org, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2014/12/9 17:21, Jan Beulich wrote: >>>> On 09.12.14 at 10:12, wrote: >> On 2014/12/9 16:19, Jan Beulich wrote: >>>>>> On 09.12.14 at 08:47, wrote: >>>> On 2014/12/8 16:51, Jan Beulich wrote: >>>>> The whole "if-copy-unlock-and-return-EFAULT-otherwise-increment" >>>>> is identical and can be factored out pretty easily afaict. >>>> >>>> What about this? >>>> >>>> struct get_reserved_device_memory { >>>> struct xen_reserved_device_memory_map map; >>>> unsigned int used_entries; >>>> struct domain *domain; >>>> }; >>>> >>>> static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr, >>>> u32 id, void *ctxt) >>>> { >>>> struct get_reserved_device_memory *grdm = ctxt; >>>> struct domain *d = grdm->domain; >>>> unsigned int i, hit_one = 0; >>>> u32 sbdf; >>>> struct xen_reserved_device_memory rdm = { >>>> .start_pfn = start, .nr_pages = nr >>>> }; >>>> >>>> if ( !d->arch.hvm_domain.pci_force ) >>>> { >>>> for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ ) >>>> { >>>> sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[i].seg, >>>> d->arch.hvm_domain.pcidevs[i].bus, >>>> d->arch.hvm_domain.pcidevs[i].devfn); >>>> if ( sbdf == id ) >>>> { >>>> hit_one = 1; >>>> break; >>>> } >>>> } >>>> >>>> if ( !hit_one ) >>>> return 0; >>>> } >>> >>> Why do you always pick other than the simplest possible solution? >> >> I don't intend it to be, but I may go a complicated way, even a wrong >> way, based on my understanding. But as one main maintainer, if you >> always say to me in such a reproachful word more than once, I have to >> consider you may hint constantly I'm not a suitable candidate to finish >> this. Its fair to me, I'd really like to quit this to ask my manager if >> it can deliver to other guy to make sure this can move forward. >> >>> You don't need a separate variable here, you can simply check >>> whether i reached d->arch.hvm_domain.num_pcidevs after the >>> loop. And even if you added a variable, it would want to be a >> >> Are you saying this? >> >> if ( i == d->arch.hvm_domain.num_pcidevs ) >> return 0; > > Yes. Or use >=. Okay. > >> But if the last one happens to one hit, 'i' is equal to >> d->arch.hvm_domain.num_pcidevs. > > No, when the last one hits, i == d->arch.hvm_domain.num_pcidevs - 1. > You're right. Thanks Tiejun