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: Mon, 17 Nov 2014 15:57:43 +0800 Message-ID: <5469AA77.2070602@intel.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.com> <545354500200007800043D94@mail.emea.novell.com> <5457174C.8020400@intel.com> <5457515102000078000443B0@mail.emea.novell.com> <54574D8F.8060407@intel.com> <54575E2D0200007800044443@mail.emea.novell.com> <545767C4.7070806@intel.com> <5457787002000078000445C7@mail.emea.novell.com> <54576DF7.8060408@intel.com> <545784830200007800044627@mail.emea.novell.com> <54585EAA.20904@intel.com> <545894610200007800044A5B@mail.emea.novell.com> <545992A2.8070309@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <54632EA80200007800046AE5@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/12 16:55, Jan Beulich wrote: >>>> On 12.11.14 at 04:05, wrote: >> I don't see any feedback to this point, so I think you still prefer we >> should do all check in the callback function. > > As a draft this looks reasonable, but there are various bugs to be > dealt with along with cosmetic issues (I'll point out the former, but > I'm tired of pointing out the latter once again - please go back to > earlier reviews of patches to refresh e.g. what types to use for > loop variables). 'earlier reviews' means this subject email? I go back to check this but just see this comment related to our loop codes: " >>> + for ( i = 0; i < xdsr->num_pcidevs; ++i ) >>> + { >>> + if ( __copy_from_guest_offset(pcidevs, xdsr->pcidevs, i, 1) ) >>> + { >>> + xfree(pcidevs); >>> + return -EFAULT; >>> + } >>> + } >> >> I don't see the need for a loop here. And you definitely can't use the >> double-underscore-prefixed variant the way you do. > > Do you mean this line? > > copy_from_guest_offset(pcidevs, xdsr->pcidevs, 0, > xdsr->num_pcidevs*sizeof(xen_guest_pcidev_info_t)) Almost: copy_from_guest(pcidevs, xdsr->pcidevs, xdsr->num_pcidevs * sizeof(*pcidevs)) " Or I need to set as 'unsigned int' here? Anyway I did this in the following codes firstly. If I'm still wrong I will correct that. > >> I tried to address this but obviously we have to pass each 'pdf' to >> callback functions, > > Yes, but at the generic IOMMU layer this shouldn't be named "bdf", > but something more neutral (maybe "id"). And you again lost the > segment there. > >> @@ -36,9 +40,24 @@ static int get_reserved_device_memory(xen_pfn_t start, >> if ( rdm.start_pfn != start || rdm.nr_pages != nr ) >> return -ERANGE; >> >> - if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries, >> - &rdm, 1) ) >> - return -EFAULT; >> + if ( d->arch.hvm_domain.pci_force ) >> + { >> + if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries, >> + &rdm, 1) ) >> + return -EFAULT; >> + } >> + else >> + { >> + for ( i = 0; i < d->arch.hvm_domain.num_pcidevs; i++ ) >> + { >> + pt_bdf = PCI_BDF2(d->arch.hvm_domain.pcidevs[i].bus, >> + d->arch.hvm_domain.pcidevs[i].devfn); >> + if ( pt_bdf == bdf ) >> + if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries, >> + &rdm, 1) ) >> + return -EFAULT; > > I think d->arch.hvm_domain.pcidevs[] shouldn't contain duplicates, > and hence there's no point continuing the loop if you found a match. > >> + } >> + } >> } >> >> ++grdm->used_entries; > > This should no longer get incremented unconditionally. > >> @@ -314,6 +333,7 @@ int compat_memory_op(unsigned int cmd, >> XEN_GUEST_HANDLE_PARAM(void) compat) >> return -EFAULT; >> >> grdm.used_entries = 0; >> + grdm.domain = current->domain; >> rc = iommu_get_reserved_device_memory(get_reserved_device_memory, >> &grdm); >> > > Maybe I misguided you with an earlier response, or maybe the > earlier reply was in a different context: There's no point > communicating current->domain to the callback function; there > would be a point communicating the domain if it was _not_ > always current->domain. > So we need the caller to pass domain ID, right? diff --git a/xen/common/compat/memory.c b/xen/common/compat/memory.c index af613b9..314d7e6 100644 --- a/xen/common/compat/memory.c +++ b/xen/common/compat/memory.c @@ -22,10 +22,13 @@ 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, u16 seg, + u16 *ids, int cnt, void *ctxt) { struct get_reserved_device_memory *grdm = ctxt; + struct domain *d = get_domain_by_id(grdm->map.domid); + unsigned int i, j; + u32 sbdf, pt_sbdf; if ( grdm->used_entries < grdm->map.nr_entries ) { @@ -36,13 +39,37 @@ static int get_reserved_device_memory(xen_pfn_t start, if ( rdm.start_pfn != start || rdm.nr_pages != nr ) return -ERANGE; - if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries, - &rdm, 1) ) - return -EFAULT; + if ( d->arch.hvm_domain.pci_force ) + { + if ( __copy_to_compat_offset(grdm->map.buffer, grdm->used_entries, + &rdm, 1) ) + return -EFAULT; + ++grdm->used_entries; + } + else + { + for ( i = 0; i < cnt; i++ ) + { + sbdf = PCI_SBDF(seg, ids[i]); + for ( j = 0; j < d->arch.hvm_domain.num_pcidevs; j++ ) + { + pt_sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[j].seg, + d->arch.hvm_domain.pcidevs[j].bus, + d->arch.hvm_domain.pcidevs[j].devfn); + if ( pt_sbdf == sbdf ) + { + if ( __copy_to_compat_offset(grdm->map.buffer, + grdm->used_entries, + &rdm, 1) ) + return -EFAULT; + ++grdm->used_entries; + break; + } + } + } + } } - ++grdm->used_entries; - return 0; } #endif diff --git a/xen/common/memory.c b/xen/common/memory.c index 2177c56..f27b17f 100644 --- a/xen/common/memory.c +++ b/xen/common/memory.c @@ -698,10 +698,13 @@ 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, u16 seg, + u16 *ids, int cnt, void *ctxt) { struct get_reserved_device_memory *grdm = ctxt; + struct domain *d = get_domain_by_id(grdm->map.domid); + unsigned int i, j; + u32 sbdf, pt_sbdf; if ( grdm->used_entries < grdm->map.nr_entries ) { @@ -709,13 +712,37 @@ static int get_reserved_device_memory(xen_pfn_t start, .start_pfn = start, .nr_pages = nr }; - if ( __copy_to_guest_offset(grdm->map.buffer, grdm->used_entries, - &rdm, 1) ) - return -EFAULT; + if ( d->arch.hvm_domain.pci_force ) + { + if ( __copy_to_guest_offset(grdm->map.buffer, grdm->used_entries, + &rdm, 1) ) + return -EFAULT; + ++grdm->used_entries; + } + else + { + for ( i = 0; i < cnt; i++ ) + { + sbdf = PCI_SBDF(seg, ids[i]); + for ( j = 0; j < d->arch.hvm_domain.num_pcidevs; j++ ) + { + pt_sbdf = PCI_SBDF2(d->arch.hvm_domain.pcidevs[j].seg, + d->arch.hvm_domain.pcidevs[j].bus, + d->arch.hvm_domain.pcidevs[j].devfn); + if ( pt_sbdf == sbdf ) + { + if ( __copy_to_guest_offset(grdm->map.buffer, + grdm->used_entries, + &rdm, 1) ) + return -EFAULT; + ++grdm->used_entries; + break; + } + } + } + } } - ++grdm->used_entries; - return 0; } #endif diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c index 141e735..fa813c5 100644 --- a/xen/drivers/passthrough/vtd/dmar.c +++ b/xen/drivers/passthrough/vtd/dmar.c @@ -903,6 +903,9 @@ int intel_iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt) { rc = func(PFN_DOWN(rmrr->base_address), PFN_UP(rmrr->end_address) - PFN_DOWN(rmrr->base_address), + rmrr->segment, + rmrr->scope.devices, + rmrr->scope.devices_cnt, ctxt); if ( rc ) break; diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h index f1b6a48..f8964e1 100644 --- a/xen/include/public/memory.h +++ b/xen/include/public/memory.h @@ -588,6 +588,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 */ diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 409f6f8..75c6759 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -120,7 +120,8 @@ void iommu_dt_domain_destroy(struct domain *d); struct page_info; -typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, void *ctxt); +typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, u16 seg, u16 *ids, + int cnt, void *ctxt); struct iommu_ops { int (*init)(struct domain *d); diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h index 91520bc..ba881ef 100644 --- 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)) struct pci_dev_info { bool_t is_extfn; Thanks Tiejun