[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH+RFC+HACK 00/16] xen: arm initial support for xgene arm64 platform

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 <Ian.Campbell@xxxxxxxxxx> 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
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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.