From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH+RFC+HACK 00/16] xen: arm initial support for xgene arm64 platform Date: Thu, 21 Nov 2013 17:14:01 +0000 Message-ID: <528E3F59.6090106@eu.citrix.com> References: <1384958746.6071.64.camel@kazak.uk.xensource.com> <1385048295.6071.181.camel@kazak.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: <1385048295.6071.181.camel@kazak.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: Anup Patel , Tim Deegan , xen-devel , Julien Grall , Stefano Stabellini , Pranavkumar Sawargaonkar List-Id: xen-devel@lists.xenproject.org On 21/11/13 15:38, Ian Campbell wrote: > On Thu, 2013-11-21 at 15:05 +0000, George Dunlap wrote: >> On Wed, Nov 20, 2013 at 2:45 PM, Ian Campbell wrote: >>> I'm afraid this series is rather a grab bag and it is distressingly >>> large at this stage. With this series I can boot an Xgene board until it >>> fails to find its SATA controller. This is a dom0 issue for which >>> patches are pending from APM (/me nudges Anup). >>> >>> As well as the APM specific platform stuff there are also some generic >>> improvements which were either necessary or useful during this work. >>> Towards the tail end are some hacks and rfcs which need more work and/or >>> discussion. I mostly posting now because I'm aware that I've been >>> negligent in not sending these out sooner. >>> >>> WRT the freeze I think that the baseline stuff is all plausible for 4.4. >>> Firstly because I'm inclined to give new platform enablement a fairly >>> loose reign at least for the time being (and much of it was posted ages >>> ago by Anup/Pranavkumar). Secondly the non-platform related bits (other >>> than the aforementioned hacks/rfcs) fall mostly either into two buckets: >>> Either they are bugfixes or they are mostly obviously safe (additional >>> debug prints etc). >>> >>> Blow by blow analysis: > Pulling your last comment first, since it informs many of my other > answers: >> You address risks, but you don't address the fundamental benefit of >> including it now, rather than waiting to check it in for 4.5. At the >> moment, unless there is some compelling strategic reason for including >> this in 4.4, I'm inclined to say it should just wait. > The primary benefit is that this is the first real (i.e. non emulated) > 64-bit ARM server platform on the market. Having Xen running on that > early in the new year would be awesome. > > As well as the currently supported platforms and this one new one we > should also account for people who want to port Xen 4.4 to their own > platform. The closer we can make this to "drop a file in > xen/arch/arm/platforms/ and add it to the Makefile" the better IMHO. > Most of the patches below (i.e. the ones which haven't already been > okayed) are relevant in this light. Right, so this would be for people shipping "4.4+vendor patches". If we didn't accept these, they'd have to fix or backport all these things themselves. That sounds like a pretty compelling strategic reason. :-) And it justifies a number of the more potentially risky patches as improvements in their own right (i.e., not just for xgene, but for other bigger platforms). >>> xen: arm: Handle cpus nodes with #address-cells > 1 >> So we need to make a distinction here with "bug fixes": from a release >> perspective, bugs are errors in the code that affect working features. >> What is the likelihood that any currently-supported architectures >> might problems without this patch? It's not clear from looking at the >> patch. If it's low-to-none, then this wouldn't really qualify for a >> bug fix exemption to the code freeze. > In principal it's entirely possible that someone might rewrite the dts > of such a platform this way. It's a bit unlikely but the main reason > would because as well as the cpu number #address-cells also influences > things like the cpu spin address property (where it is present), which > could conceivably be about 4G (albeit unlikely on 32-bit). > > But ultimately this is a Xen bug because it does not correctly parse a > valid device tree file. So just to step back and talk about principles here: Sure, and I didn't say it wasn't a bug. But I don't think that the freeze exemption is to just fix bugs per se; it's to fix broken functionality in supported features. So just the fact that something is in theory wrong with Xen isn't enough; it has to impact features that are actually supported. Now in this case I think this distinction is unnecessary, since I buy your argument that one of the things we want to support is the "4.4+vendor patches" model; so it does impact features that we actually want to support. But if it weren't for that, I wouldn't accept it just on the basis that it's a bug in Xen, if it didn't actually affect any of the supported functionality. > >>> RFC: xen: arm: handle 40-bit addresses in the p2m >>> RFC: xen: arm: allow platform code to select dom0 event channel irq >>> >>> These should be considered for cleanup review and >>> eventual commit for 4.4. The rest of the platform >>> enablement is pretty pointless without these. >> Hmm... it seems a bit late for RFC stuff in fairly core code. These >> look like they might possibly be extending functionality for >> currently-supported architectures as well; but without concrete >> examples, this would come under "new feature" rather than "bug fix". > The 40-bit address thing is an issue on 32-bit too, it's just that the > platforms don't typically have anything mapped up that high (MMIO tends > to be lower to support non-LPAE kernels and they don't typically have > TBs of RAM). > > On the plus side the new case would never hit on the existing platforms. > If nothing else there is currently a BUG_ON checking for that. Oh, right -- it looked like you might be increasing the number of pages allocated, but now I see that you've just replaced a '1' with P2M_FIRST_ORDER (which is different to P2M_FIRST_ENTRIES). That makes more sense then. >> (Although it looks like Julien has a simple solution that makes this >> last patch unnecessary?) > I don't think Julien is right about that simpler solution being > workable, but regardless I think it would be better to err on the side > of caution and reimplement both of these as platform hooks for 4.4. The > first would be a new quirk (only implemented by this platform) and the > second would be using an existing per-platform hook. > > Unless that sounds obviously bogus to you I'll put something together > for you to pass judgement on. Sounds good. -George