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:59:24 +0000 Message-ID: <50D1E45C.3090108@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> <50D1E198.8090205@citrix.com> <1355932290.14620.446.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: <1355932290.14620.446.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:51, Ian Campbell wrote: > On Wed, 2012-12-19 at 15:47 +0000, Mats Petersson wrote: >> 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... > Yes please, it didn't really matter before but I think it matters after > your changes. Ok, I will try to fix. But since I can't test it, it will still be "does it compile" testing... {Would be nice to understand what has changed - as far as I see, the old code was just as much broken as the new code} -- Mats > >> -- >> 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. >>>>> >