From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [v7][RFC][PATCH 08/13] xen/x86/p2m: set p2m_access_n for reserved device memory mapping Date: Tue, 04 Nov 2014 18:41:39 +0800 Message-ID: <5458AD63.4020301@intel.com> References: <1414136077-18599-1-git-send-email-tiejun.chen@intel.com> <1414136077-18599-9-git-send-email-tiejun.chen@intel.com> <544A88560200007800042056@mail.emea.novell.com> <544E0ACA.8050201@intel.com> <544E2D8002000078000425A9@mail.emea.novell.com> <544F531C.7060401@intel.com> <544F7A310200007800042BAC@mail.emea.novell.com> <5450A330.6020102@intel.com> <5450BF63020000780004305E@mail.emea.novell.com> <5451EB48.9010103@intel.com> <545211DA0200007800043645@mail.emea.novell.com> <5452F8D8.9050009@intel.com> <545355720200007800043D97@mail.emea.novell.com> <54571E91.4030903@intel.com> <5457523A02000078000443C7@mail.emea.novell.com> <54575013.50702@intel.com> <545760FD0200007800044474@mail.emea.novell.com> <54576B9B.1060601@intel.com> <54577AD302000078000445EB@mail.emea.novell.com> <54582D59.4020201@intel.com> <545896080200007800044A67@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: <545896080200007800044A67@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/4 16:02, Jan Beulich wrote: >>>> On 04.11.14 at 02:35, wrote: >> On 2014/11/3 19:53, Jan Beulich wrote: >>>>>> On 03.11.14 at 12:48, wrote: >>>> On 2014/11/3 18:03, Jan Beulich wrote: >>>>>>>> On 03.11.14 at 10:51, wrote: >>>>>> On 2014/11/3 17:00, Jan Beulich wrote: >>>>>>>>>> On 03.11.14 at 07:20, wrote: >>>>>>>> #2 the error handling >>>>>>>> >>>>>>>> In an error case what should I do? Currently we still create these >>>>>>>> mapping as normal. This means these mfns will be valid so later we can't >>>>>>>> set them again then device can't be assigned as passthrough. I think >>>>>>>> this makes sense. Or we should just stop them from setting 1:1 mapping? >>>>>>> >>>>>>> You should, with very few exceptions, not ignore errors (which >>>>>>> includes "handling" them by just logging a message. Instead, you >>>>>>> should propagate the error back up the call chain. >>>>>>> >>>>>> >>>>>> Do you mean in your patch, >>>>>> >>>>>> +int iommu_get_reserved_device_memory(iommu_grdm_t *func, void *ctxt) >>>>>> +{ >>>>>> + const struct iommu_ops *ops = iommu_get_ops(); >>>>>> + >>>>>> + if ( !iommu_enabled || !ops->get_reserved_device_memory ) >>>>>> + return 0; >>>>>> + >>>>>> + return ops->get_reserved_device_memory(func, ctxt); >>>>>> +} >>>>>> + >>>>>> >>>>>> I shouldn't return that directly. Then instead, we should handle all >>>>>> error scenarios here? >>>>> >>>>> No. All error scenarios are already being handled here (by >>>>> propagating the error code to the caller). >>>> >>>> Sorry, how to propagate the error code? >>> >>> Return it to the caller (and it will do so onwards, until it reaches >>> [presumably] the entity having invoked a hypercall). >> >> I guess you mean we should return out in this error case, > > Yes! > >> @@ -686,8 +686,25 @@ guest_physmap_add_entry(struct domain *d, unsigned long gfn, >> /* Now, actually do the two-way mapping */ >> if ( mfn_valid(_mfn(mfn)) ) >> { >> - rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, >> - p2m->default_access); >> + rc = 0; >> + a = p2m->default_access; >> + if ( !is_hardware_domain(d) ) >> + { >> + rc = iommu_get_reserved_device_memory(p2m_check_reserved_device_memory, >> + &gfn); >> + /* We always avoid populating reserved device memory. */ >> + if ( rc == 1 ) >> + goto out; > > But you'll need to make sure that you don't return 1 to the callers: You're right. > They expect 0 or negative error codes. But with the model of > not even populating these regions (or relocating the memory > before [at boot time] assigning a device associated with an RMRR) > I think this needs to become an error anyway. Looks -EINVAL might be used. > >> + else if ( rc < 0 ) >> + { >> + printk(XENLOG_G_WARNING >> + "Dom%d can't check reserved device memory.\n", > > Actually, d being the subject domain, please make this more like > "Can't check reserved device memory for Dom%d\n". Fixed. Thanks Tiejun > > Jan > >> + d->domain_id); >> + goto out; >> + } >> + } >> + >> + rc = p2m_set_entry(p2m, gfn, _mfn(mfn), page_order, t, a); >> if ( rc ) >> goto out; /* Failed to update p2m, bail without updating m2p. */ >> >> Thanks >> Tiejun > > > > >