From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mats Petersson Subject: Re: ARM fixes for my improved privcmd patch. Date: Wed, 19 Dec 2012 15:47:36 +0000 Message-ID: <50D1E198.8090205@citrix.com> References: <50CB5B32.50406@citrix.com> <1355763448.14620.111.camel@zakaz.uk.xensource.com> <50CF587B.5090602@citrix.com> <1355829451.14620.188.camel@zakaz.uk.xensource.com> <50D05358.30303@citrix.com> <1355830856.14620.206.camel@zakaz.uk.xensource.com> <50D074F5.6060202@citrix.com> <20121218160704.GA3543@phenom.dumpdata.com> <50D0C528.4090602@citrix.com> <1355914793.14620.319.camel@zakaz.uk.xensource.com> <50D1AEBD.1090202@citrix.com> <1355919745.14620.363.camel@zakaz.uk.xensource.com> <50D1D880.10701@citrix.com> <1355931948.14620.442.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1355931948.14620.442.camel@zakaz.uk.xensource.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: Ian Campbell Cc: "xen-devel@lists.xensource.com" , Konrad Rzeszutek Wilk List-Id: xen-devel@lists.xenproject.org On 19/12/12 15:45, Ian Campbell wrote: > On Wed, 2012-12-19 at 15:08 +0000, Mats Petersson wrote: >> On 19/12/12 12:22, Ian Campbell wrote: >>> On Wed, 2012-12-19 at 12:10 +0000, Mats Petersson wrote: >>> >>>>>> + only likely to return EFAULT or some other "things are very >>>>>> + bad" error code, which the rest of the calling code won't >>>>>> + be able to fix up. So we just exit with the error we got. >>>>> It expect it is more important to accumulate the individual errors from >>>>> remap_pte_fn into err_ptr. >>>> Yes, but since that exits on error with EFAULT, the calling code won't >>>> "accept" the errors, and thus the whole house of cards fall apart at >>>> this point. >>>> >>>> There should probably be a task to fix this up properly, hence the >>>> comment. But right now, any error besides ENOENT is "unacceptable" by >>>> the callers of this code. If you want me to add this to the comment, I'm >>>> happy to. But as long as remap_pte_fn returns EFAULT on error, nothing >>>> will work after an error. >>> Are you sure? privcmd.c has some special casing for ENOENT but it looks >>> like it should just pass through other errors back to userspace. >>> >>> In any case surely this needs fixing? >>> >>> On the X86 side err_ptr is the result of the mmupdate hypercall which >>> can already be other than ENOENT, including EFAULT, ESRCH etc. >> Yes, but the ONLY error that is "acceptable" (as in, doesn't lead to the >> calling code revoking the mapping and returning an error) is ENOENT. > Hr, Probably the right thing is for map_foreign_page to propagate the > result of XENMEM_add_to_physmap_range and for remap_pte_fn to store it > in err_ptr. That -EFAULT thing just looks wrong to me. Ok, so you want me to fix that up, I suppose? I mean, I just copied the behaviour that was already there - just massaged the code around a bit... -- Mats > >> Or at least, that's how I believe it should SHOULD be - since only >> ENOENT is a "success" error code, anything else pretty much means that >> the operation requested didn't work properly. If you are aware of any >> use-case where EFAULT, ESRCH or other error codes would still result in >> a valid, usable memory mapping - I have a fair understanding of the xc_* >> code, and although I may not know every piece of that code, I'm fairly >> certainly that is the expected behaviour. >> >> -- >> Mats >>> Ian. >>> >