From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v8][PATCH 02/17] introduce XEN_DOMCTL_set_rdm Date: Mon, 08 Dec 2014 14:06:27 +0800 Message-ID: <54853FE3.9030703@intel.com> References: <1417425875-9634-1-git-send-email-tiejun.chen@intel.com> <1417425875-9634-3-git-send-email-tiejun.chen@intel.com> <54808CC3020000780004CCC8@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: <54808CC3020000780004CCC8@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:33, Jan Beulich wrote: >>>> On 01.12.14 at 10:24, wrote: >> --- a/xen/drivers/passthrough/pci.c >> +++ b/xen/drivers/passthrough/pci.c >> @@ -34,6 +34,7 @@ >> #include >> #include >> #include >> +#include > > Please don't - we use bool_t in the hypervisor, not bool. The header Yes. > only exists for source code shared with the tools. Looks this could be fine, d->arch.hvm_domain.pci_force = xdsr->flags & PCI_DEV_RDM_CHECK; > >> @@ -1553,6 +1554,44 @@ int iommu_do_pci_domctl( >> } >> break; >> >> + case XEN_DOMCTL_set_rdm: >> + { >> + struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm; >> + struct xen_guest_pcidev_info *pcidevs = NULL; >> + struct domain *d = rcu_lock_domain_by_any_id(domctl->domain); > > "d" gets passed into this function - no need to shadow the variable You're right. > and (wrongly) re-obtain the pointer. > >> + >> + if ( d == NULL ) >> + return -ESRCH; >> + >> + d->arch.hvm_domain.pci_force = >> + xdsr->flags & PCI_DEV_RDM_CHECK ? true : false; >> + d->arch.hvm_domain.num_pcidevs = xdsr->num_pcidevs; > > You shouldn't set the count before setting the pointer. Will reorder them. > >> + d->arch.hvm_domain.pcidevs = NULL; >> + >> + if ( xdsr->num_pcidevs ) >> + { >> + pcidevs = xmalloc_array(xen_guest_pcidev_info_t, >> + xdsr->num_pcidevs); > > New domctl-s must not represent security risks: xdsr->num_pcidevs > can be (almost) arbitrarily large - do you really want to allow such > huge allocations? A reasonable upper bound could for example be Sorry, as you know this num_pcidevs is from tools, and actually it share that result from that existing hypercall, assign_device, while parsing 'pci=[]', so I couldn't understand why this can be (almost" arbitrarily large. > the total number of PCI devices the hypervisor knows about. I take a quick look at this but looks we have no this exact value that we can get directly. > >> + if ( pcidevs == NULL ) >> + { >> + rcu_unlock_domain(d); >> + return -ENOMEM; >> + } >> + >> + if ( copy_from_guest(pcidevs, xdsr->pcidevs, >> + xdsr->num_pcidevs*sizeof(*pcidevs)) ) >> + { >> + xfree(pcidevs); >> + rcu_unlock_domain(d); >> + return -EFAULT; >> + } >> + } >> + >> + d->arch.hvm_domain.pcidevs = pcidevs; > > If the operation gets issued more than once for a given domain, > you're leaking the old pointer here. Overall should think a bit > more about this multiple use case (or outright disallow it). Currently this should be disallowed, so I will do this, case XEN_DOMCTL_set_rdm: { struct xen_domctl_set_rdm *xdsr = &domctl->u.set_rdm; struct xen_guest_pcidev_info *pcidevs = NULL; if ( d->arch.hvm_domain.pcidevs ) break; ... > >> --- a/xen/drivers/passthrough/vtd/dmar.c >> +++ b/xen/drivers/passthrough/vtd/dmar.c >> @@ -674,6 +674,14 @@ acpi_parse_one_rmrr(struct acpi_dmar_header *header) >> " RMRR region: base_addr %"PRIx64 >> " end_address %"PRIx64"\n", >> rmrru->base_address, rmrru->end_address); >> + /* >> + * TODO: we may provide a precise paramter just to reserve >> + * RMRR range specific to one device. >> + */ >> + dprintk(XENLOG_WARNING VTDPREFIX, >> + "So please set pci_rdmforce to reserve these ranges" >> + " if you need such a device in hotplug case.\n"); > > It makes no sense to use dprintk() here. I also don't see how this > message relates to whatever may have been logged immediately > before, so the wording ("So please set ...") is questionable. Nor is the > reference to "hotplug case" meaningful here - in this context, only > physical (host) device hotplug can be meant without further > qualification. In the end I think trying to log something here is just > wrong - simply drop the message and make sure whatever you want Okay. > to say can be found easily by looking elsewhere. Maybe we can print something in case when we can't set those identified mapping successfully, but its too late... > >> --- a/xen/include/asm-x86/hvm/domain.h >> +++ b/xen/include/asm-x86/hvm/domain.h >> @@ -90,6 +90,10 @@ struct hvm_domain { >> /* Cached CF8 for guest PCI config cycles */ >> uint32_t pci_cf8; >> >> + bool_t pci_force; >> + uint32_t num_pcidevs; >> + struct xen_guest_pcidev_info *pcidevs; > > Without a comment all these field names are pretty questionable. Yeah, I try to add some comments, /* A global flag, we need to check/reserve all Reserved Device Memory. */ bool_t pci_force; /* * If pci_force is 0, this represents how many pci devices we need * to check/reserve Reserved Device Memory. * If pci_force is 1, this is always 0. */ uint32_t num_pcidevs; /* This represents those pci devices instances when num_pcidevs != 0. */ struct xen_guest_pcidev_info *pcidevs; Thanks Tiejun