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: Mon, 08 Dec 2014 15:11:41 +0800 Message-ID: <54854F2D.6060204@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <548090BA020000780004CCE6@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/4 23:50, Jan Beulich wrote: >>>> On 01.12.14 at 10:24, wrote: >> --- a/xen/common/compat/memory.c >> +++ b/xen/common/compat/memory.c >> @@ -22,27 +22,66 @@ struct get_reserved_device_memory { >> unsigned int used_entries; >> }; >> >> -static int get_reserved_device_memory(xen_pfn_t start, >> - xen_ulong_t nr, void *ctxt) >> +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; >> + unsigned int i; >> + u32 sbdf; >> + struct compat_reserved_device_memory rdm = { >> + .start_pfn = start, .nr_pages = nr >> + }; >> >> - if ( grdm->used_entries < grdm->map.nr_entries ) >> - { >> - struct compat_reserved_device_memory rdm = { >> - .start_pfn = start, .nr_pages = nr >> - }; >> + if ( rdm.start_pfn != start || rdm.nr_pages != nr ) >> + return -ERANGE; >> >> - if ( rdm.start_pfn != start || rdm.nr_pages != nr ) >> - return -ERANGE; >> + d = rcu_lock_domain_by_any_id(grdm->map.domid); >> + if ( d == NULL ) >> + return -ESRCH; > > So why are you doing this in the call back (potentially many times) > instead of just once in compat_memory_op(), storing the pointer in > the context structure? Okay. > >> >> - if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries, >> - &rdm, 1) ) >> - return -EFAULT; >> + if ( d ) >> + { >> + if ( d->arch.hvm_domain.pci_force ) > > You didn't verify that the domain is a HVM/PVH one. Is this, ASSERT(is_hvm_domain(grdm.domain)), correct? > >> + { >> + if ( grdm->used_entries < grdm->map.nr_entries ) >> + { >> + if ( __copy_to_compat_offset(grdm->map.buffer, >> + grdm->used_entries, >> + &rdm, 1) ) >> + { >> + rcu_unlock_domain(d); >> + return -EFAULT; >> + } >> + } >> + ++grdm->used_entries; >> + } >> + else >> + { >> + 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 ) >> + { >> + if ( grdm->used_entries < grdm->map.nr_entries ) >> + { >> + if ( __copy_to_compat_offset(grdm->map.buffer, >> + grdm->used_entries, >> + &rdm, 1) ) >> + { >> + rcu_unlock_domain(d); >> + return -EFAULT; >> + } >> + } >> + ++grdm->used_entries; > > break; Added. > > Also trying to fold code identical on the if and else branches would > seem pretty desirable. Sorry, I can't see what I'm missing. > >> @@ -319,9 +358,13 @@ int compat_memory_op(unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) compat) >> >> if ( !rc && grdm.map.nr_entries < grdm.used_entries ) >> rc = -ENOBUFS; >> + >> grdm.map.nr_entries = grdm.used_entries; >> - if ( __copy_to_guest(compat, &grdm.map, 1) ) >> - rc = -EFAULT; >> + if ( grdm.map.nr_entries ) >> + { >> + if ( __copy_to_guest(compat, &grdm.map, 1) ) >> + rc = -EFAULT; >> + } > > Why do you need this change? If we have no any entries, why do we still copy that? > >> --- a/xen/drivers/passthrough/vtd/dmar.c >> +++ b/xen/drivers/passthrough/vtd/dmar.c >> @@ -904,17 +904,33 @@ int platform_supports_x2apic(void) >> >> int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt) >> { >> - struct acpi_rmrr_unit *rmrr; >> + struct acpi_rmrr_unit *rmrr, *rmrr_cur = NULL; >> int rc = 0; >> + unsigned int i; >> + u16 bdf; >> >> - list_for_each_entry(rmrr, &acpi_rmrr_units, list) >> + for_each_rmrr_device ( rmrr, bdf, i ) >> { >> - rc = func(PFN_DOWN(rmrr->base_address), >> - PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address), >> - ctxt); >> - if ( rc ) >> - break; >> + if ( rmrr != rmrr_cur ) >> + { >> + rc = func(PFN_DOWN(rmrr->base_address), >> + PFN_UP(rmrr->end_address) - >> + PFN_DOWN(rmrr->base_address), >> + PCI_SBDF(rmrr->segment, bdf), >> + ctxt); >> + >> + if ( unlikely(rc < 0) ) >> + return rc; >> + >> + /* Just go next. */ >> + if ( !rc ) >> + rmrr_cur = rmrr; >> + >> + /* Now just return specific to user requirement. */ >> + if ( rc > 0 ) >> + return rc; > > Nice that you check for that, but I can't see this case occurring > anymore. Did you lose some code? Also please don't write code We have three scenarios here: #1 rc < 0 means this is an error; #2 rc = 0 means the tools caller don't know how many buffers it should construct, so we need to count all entries as 'nr_entries' to return. #3 rc > 0 means in some cases, we need to return some specific values, like 1 to indicate we're hitting some RMRR range. Currently, we use gfn to check this in case of memory populating, ept violation handler and mem_access. > more complicated than necessary. The above two if()s could be > > > + if ( rc > 0 ) > + return rc; > + > + rmrr_cur = rmrr; > >> --- a/xen/include/public/memory.h >> +++ b/xen/include/public/memory.h >> @@ -586,6 +586,11 @@ typedef struct xen_reserved_device_memory >> xen_reserved_device_memory_t; >> DEFINE_XEN_GUEST_HANDLE(xen_reserved_device_memory_t); >> >> struct xen_reserved_device_memory_map { >> + /* >> + * Domain whose reservation is being changed. >> + * Unprivileged domains can specify only DOMID_SELF. >> + */ >> + domid_t domid; >> /* IN/OUT */ >> unsigned int nr_entries; >> /* OUT */ > > Your addition lacks an IN annotation. Are you saying something for 'nr_entries'? But I didn't introduce anything to change the original usage. Anyway, I try to improve this, /* * IN: on call the number of entries which can be stored in buffer. * OUT: on return the number of entries which have been stored in * buffer. If on call the number is less the number of all necessary * entries, on return the number of entries which is needed. */ > >> --- a/xen/include/xen/pci.h >> +++ b/xen/include/xen/pci.h >> @@ -31,6 +31,8 @@ >> #define PCI_DEVFN2(bdf) ((bdf) & 0xff) >> #define PCI_BDF(b,d,f) ((((b) & 0xff) << 8) | PCI_DEVFN(d,f)) >> #define PCI_BDF2(b,df) ((((b) & 0xff) << 8) | ((df) & 0xff)) >> +#define PCI_SBDF(s,bdf) (((s & 0xffff) << 16) | (bdf & 0xffff)) >> +#define PCI_SBDF2(s,b,df) (((s & 0xffff) << 16) | PCI_BDF2(b,df)) > > Missing several parentheses. > Okay, #define PCI_SBDF(s,bdf) ((((s) & 0xffff) << 16) | ((bdf) & 0xffff)) #define PCI_SBDF2(s,b,df) ((((s) & 0xffff) << 16) | PCI_BDF2(b,df)) Thanks Tiejun