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

[George Dunlap <george.dunlap@eu.citrix.com>]

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@citrix.com> 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