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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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.

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

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

The dom0 event channel thing is an issue for all platforms, I think
we've just been lucky that they mostly don't use the current hardcoded
number for something, although I had it in mind that one of them did and
was being hacked around.

> On the other hand, both of these look pretty obvious and low-risk 
> improvements.
> 
> >
> >         HACK: xen: arm: GICC_DIR register at offset 0x10000 instead of 
> > 0x1000
> >
> >                 Should be properly implemented with a view to being
> >                 accepted for 4.4. Again things are rather pointless
> >                 without.
> >
> >                 Could plausibly be reimplemented as a platform quirk,
> >                 which might be safer for 4.4.
> >
> >         HACK: xen: arm: map PCI controller ranges region MMIOs to dom0.
> >
> >                 I think this one is likely to be a step too far for 4.4.
> >                 Even if it worked (it doesn't) it is quite a big and
> >                 potentially complex change. I'm considering the option
> >                 of implementing the hardcoded list (which is here as a
> >                 HACK, see the commit message for more) via the
> >                 platform->specific_mapping callback for 4.4. In that
> >                 case it would only impact Xgene if it were broken.
> 
> This is starting to look like an awful lot of "to be sorted out".

Yes. I was a bit unhappy to find I had these in my tree -- I thought
what was outstanding was less intrusive.

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

Ian.


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