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: Thu, 23 Apr 2015 20:32:31 +0800 Message-ID: <5538E65F.60903@intel.com> References: <1428657724-3498-1-git-send-email-tiejun.chen@intel.com> <1428657724-3498-8-git-send-email-tiejun.chen@intel.com> <20150416154056.GI13441@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150416154056.GI13441@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: 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, JBeulich@suse.com, yang.z.zhang@intel.com List-Id: xen-devel@lists.xenproject.org On 2015/4/16 23:40, Tim Deegan wrote: > Hi, > > At 17:21 +0800 on 10 Apr (1428686518), Tiejun Chen wrote: >> +/* >> + * 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 > > Do we need a way to change this default? As I said in its comment here, this behavior should be rarely changed in the future, and even this really needs to balance something, I just think this way is enough and simply. > >> static int rmrr_identity_mapping(struct domain *d, bool_t map, >> - const struct acpi_rmrr_unit *rmrr) >> + const struct acpi_rmrr_unit *rmrr, >> + u32 flag) >> { >> unsigned long base_pfn = rmrr->base_address >> PAGE_SHIFT_4K; >> unsigned long end_pfn = PAGE_ALIGN_4K(rmrr->end_address) >> PAGE_SHIFT_4K; >> @@ -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"); > > This is a bit cryptic. How about: > "RMRR map failed. Device %04x:%02x:%02x.%u and domain %d may be unstable.", > (and pass in the devfn from the caller so we can print the details of > the device). Got it but we can't get SBDF here directly. So just now we can have this line. { if ( flag == XEN_DOMCTL_PCIDEV_RDM_TRY ) dprintk(XENLOG_ERR VTDPREFIX, "RMRR mapping failed to pfn:%"PRIx64"" " so Dom%d may be unstable.\n", base_pfn, d->domain_id); else return err; } Certainly, we can extend rmrr_identity_mapping() to own its associated SBDF as an input parameter (and bring some syncs) if you still think this is necessary. > >> @@ -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 > > "STRICT" might be a better word than "FORCE" (here and everywhere > else). "FORCE" sounds like either Xen will assign the device even if > it's unsafe, which is the opposite of what's meant IIUC. This is definitely fine to me but this is derived from our policy based on that previous design, Global RDM parameter: rdm = [ 'host, reserve=force/try' ] Per-device RDM parameter: pci = [ 'sbdf, rdm_reserve=force/try' ] Please refer to patch #1. So I guess we need a further agreement or comments from other guys :) Thanks Tiejun