From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [RFC][PATCH 07/13] xen/passthrough: extend hypercall to support rdm reservation policy Date: Mon, 11 May 2015 16:37:35 +0800 Message-ID: <55506A4F.3010401@intel.com> References: <1428657724-3498-1-git-send-email-tiejun.chen@intel.com> <1428657724-3498-8-git-send-email-tiejun.chen@intel.com> <55351D010200007800073CA3@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: <55351D010200007800073CA3@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: tim@xen.org, kevin.tian@intel.com, wei.liu2@citrix.com, ian.campbell@citrix.com, andrew.cooper3@citrix.com, Ian.Jackson@eu.citrix.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org Sorry for this delay response. On 2015/4/20 21:36, Jan Beulich wrote: >>>> On 10.04.15 at 11:21, wrote: >> --- a/xen/drivers/passthrough/vtd/iommu.c >> +++ b/xen/drivers/passthrough/vtd/iommu.c >> @@ -1793,8 +1793,14 @@ static void iommu_set_pgd(struct domain *d) >> hd->arch.pgd_maddr = pagetable_get_paddr(pagetable_from_mfn(pgd_mfn)); >> } >> >> +/* >> + * In some cases, e.g. add a device to hwdomain, and remove a device from >> + * user domain, 'try' is fine enough since this is always safe to hwdomain. >> + */ >> +#define XEN_DOMCTL_PCIDEV_RDM_DEFAULT XEN_DOMCTL_PCIDEV_RDM_TRY > > Then why invent this one instead of just using ..._TRY at the respective > call sites. I just want to own one place to guarantee this behavior if we want to change something... But now I can use XEN_DOMCTL_PCIDEV_RDM_TRY directly. > >> @@ -1851,7 +1857,14 @@ static int rmrr_identity_mapping(struct domain *d, bool_t map, >> if ( !is_hardware_domain(d) ) >> { >> if ( (err = set_identity_p2m_entry(d, base_pfn, p2m_access_rw)) ) >> - return err; >> + { >> + if ( flag == XEN_DOMCTL_PCIDEV_RDM_TRY ) >> + { >> + printk(XENLOG_G_WARNING "Some devices may work failed .\n"); >> + } > > Iirc someone else already pointed out that this message needs to > change to something understandable. Perhaps it should also log > the PFN causing the error. And the braces here should be dropped > (if you inverted the condition you wouldn't even need an "else"; or > wait - this shouldn't be just "else", as you shouldn't imply anything > other than ..._TRY means ..._FORCE, albeit the respective check > perhaps belongs in the generic IOMMU code, not here). I guess we already achieved this agreement in other thread with discussion involved with you and Tim. > >> --- a/xen/include/public/domctl.h >> +++ b/xen/include/public/domctl.h >> @@ -493,6 +493,10 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_sendtrigger_t); >> /* XEN_DOMCTL_deassign_device */ >> struct xen_domctl_assign_device { >> uint32_t machine_sbdf; /* machine PCI ID of assigned device */ >> + /* IN */ >> +#define XEN_DOMCTL_PCIDEV_RDM_TRY 0 >> +#define XEN_DOMCTL_PCIDEV_RDM_FORCE 1 >> + uint32_t sbdf_flag; /* flag of assigned device */ > > Why do you call this "sbdf_flag" - it holds nothing SBDF-like. > I can remove 'sbdf_' to make sure this is not confused. Thanks Tiejun