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: Tue, 09 Dec 2014 09:06:36 +0800 Message-ID: <54864B1C.6020701@intel.com> References: <1417425875-9634-1-git-send-email-tiejun.chen@intel.com> <1417425875-9634-3-git-send-email-tiejun.chen@intel.com> <20141202193943.GC357@laptop.dumpdata.com> <548517F7.6040106@intel.com> <20141208155734.GE7745@laptop.dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20141208155734.GE7745@laptop.dumpdata.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: Konrad Rzeszutek Wilk 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, jbeulich@suse.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org >>>> --- a/xen/drivers/passthrough/pci.c >>>> +++ b/xen/drivers/passthrough/pci.c >>>> @@ -34,6 +34,7 @@ >>>> #include >>>> #include >>>> #include >>>> +#include >>>> >>>> struct pci_seg { >>>> struct list_head alldevs_list; >>>> @@ -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); >>>> + >>>> + if ( d == NULL ) >>>> + return -ESRCH; >>>> + >>> >>> What if this is called on an PV domain? >> >> Currently we just support this in HVM, so I'd like to add this, >> >> if ( d == NULL ) >> return -ESRCH; >> >> + ASSERT( is_hvm_domain(d) ); >> + > > No. Please don't crash the hypervisor. Okay. > > Just return -ENOSYS or such when done for PV guests. So, + if ( !is_hvm_domain(d) ) + return -ENOSYS; > >> >>> >>> You are also missing the XSM checks. >> >> Just see this below. >> >>> >>> What if this is called multiple times. Is it OK to over-ride >>> the 'pci_force' or should it stick once? >> >> It should be fine since just xc/hvmloader need such an information while >> creating a VM. >> >> And especially, currently we just call this one time to set. So why we need >> to call this again and again? I think if anyone want to extend such a case >> you're worrying, he should know any effect before he take a action, right? > > Program defensively and also think about preemption. If this call end up Do you think we need a fine grain way, like lock here? > being preempted you might need to call it again. Or if the third-party > toolstack use this operation and call this with wacky values? Maybe can the following address this enough, 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; if ( !is_hvm_domain(d) ) return -ENOSYS; ... >> >>> >>> >>>> + d->arch.hvm_domain.pci_force = >>>> + xdsr->flags & PCI_DEV_RDM_CHECK ? true : false; >>> >>> Won't we crash here if this is called for PV guests? >> >> After the line, 'ASSERT( is_hvm_domain(d) );', is added, this problem should >> be gone. > > No it won't be. You will just crash the hypervisor. > > Please please put yourself in the mind that the toolstack can (and will) > have bugs. Thanks for your reminder. >> >>> >>>> + d->arch.hvm_domain.num_pcidevs = xdsr->num_pcidevs; >>> >>> What if the 'num_pcidevs' has some bogus value. You need to check for that. >> [snip] >>>> return 0; >>> >>> >>> Also how does this work with 32-bit dom0s? Is there a need to use the >>> compat layer? >> >> Are you saying in xsm case? Others? >> >> Actually this new DOMCTL is similar with XEN_DOMCTL_assign_device in some >> senses but I don't see such an issue you're pointing. > > I was thinking about the compat layer and making sure it works properly. Do we really need this consideration? I mean I referred to that existing XEN_DOMCTL_assign_device to implement this new DOMCTL, but looks there's nothing related to this point. Or could you make your thought clear to me with an exiting example? Then I can take a look at what exactly should be taken in my new DOMCTL since I'm a fresh man to work out this properly inside xen. Thanks Tiejun