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

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

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
> 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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel