From: Julien Grall <julien.grall@citrix.com>
To: Ian Campbell <ian.campbell@citrix.com>,
Vijay Kilari <vijay.kilari@gmail.com>
Cc: Wei Liu <Wei.Liu2@citrix.com>,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Prasun Kapoor <Prasun.Kapoor@caviumnetworks.com>,
manish.jaggi@caviumnetworks.com,
"xen-devel@lists.xen.org" <xen-devel@lists.xen.org>,
Stefano Stabellini <stefano.stabellini@citrix.com>
Subject: Re: Requesting for freeze exception for ARM/ITS patches
Date: Sat, 11 Jul 2015 09:18:42 +0200 [thread overview]
Message-ID: <55A0C352.9050100@citrix.com> (raw)
In-Reply-To: <1436544449.10074.120.camel@citrix.com>
Hi Ian,
On 10/07/2015 18:07, Ian Campbell wrote:
> On Fri, 2015-07-10 at 16:16 +0530, Vijay Kilari wrote:
>> I would like to have freeze exception for ITS feature on ARM64.
>> Design got freeze few weeks back and I have sent v4 version of patch series
>> today.
>
> Thanks, I've been through v4 and it is certainly much improved over v3.
I haven't had time to look closer to the code. I will do Wednesday when
I'm back.
Although I skimmed quickly the series and it looks ok.
> There are some smaller issues and one slightly more major one regarding
> the mechanisms used to inject a vlpi, which I hope I've explained fully
> enough in the review (in short if you do what the design draft says it
> should be fixed, the incorrectness and complexity is all down to trying
> to do things a different way which leads to you needing to lookup things
> which you shouldn't need to lookup in places where you don't need them)
>> This patches will not impact any generic code of other platforms and have minor
>> changes generic arm related code. Also these patches are only for
>> ARM64 platform.
>
> There is some stuff which touches the non-ITS related ARM interrupt
> handling, however they are mostly adding checks in is_lpi and in some
> cases alternative code if it is true, which should be pretty safe.
I will comment on this later in the patch series. But some usage of
is_lpi are worrying for the IRQ passthrough in general.
Some instance are route_irq_to_guest. The function is re-used in
different way for LPIs. Which make the code more difficult to read and
potentially buggy.
I suggested an idea in v3
(http://lists.xenproject.org/archives/html/xen-devel/2015-06/msg03756.html)
but it seems that it has not been taking into account in v4.
> As I mentioned during review care needs to be taken to ensure that this
> code is not enabled for any guest on a platform which has no ITS and to
> only enable it for dom0 on platforms which do.
>
> Due to the structure of the patch series (which is not optimised for
> being able to follow what is going on) it wasn't clear to me if this was
> the case, but I think not. There are no checks for dom0 and only checks
> for the presence of LPI/ITS in the physical gic, not as a property of
> the individual domains.
>
> When I say "not enabled" I mean no MMIO handlers registered, no
> GITS_TRANSLATER MMIO mapping to the guest, no functional change to the
> existing GIC* registers.
>> These patches are pre-requisite for PCI support / Pass-through support
>> on ARM64 platforms.
>
> As I explained in my reply to Jan I think this is underselling it a
> little, since AIUI it should make it possible to boot Xen on ThunderX
> and do useful things (like run guests).
Well, PCI are able to support both legacy interrupt and MSI. If there is
no MSI, the PCI will use the former. The performance may be "poor" but
it will at least boot Xen on ThunderX and creating guest.
Furthermore, I think this is important to note that this feature is more
a tech preview than a production ready one.
The current implementation is working fine with the current version of
Linux, it behave as we expect. But the emulation as some TODO (I have in
mind GICR_PROPBASER) which need to be fixed in order to support any Linux.
We had a similar bug in GICv3 for the re-distributor emulation which
took us a month to fix it.
I'm not saying that I'm against a freeze exception, but we need some
promise from Cavium to finish/cleanup the implementation afterwards.
I have in mind some code placement which may be ok for Xen 4.6 (ITS in
domain_build vITS initialization directly called in the setup.c...) but
it's definitely against the principle that we are trying to keep the
common code as common as possible.
Regards,
--
Julien Grall
next prev parent reply other threads:[~2015-07-11 7:18 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-10 10:46 Requesting for freeze exception for ARM/ITS patches Vijay Kilari
2015-07-10 11:01 ` Jan Beulich
2015-07-10 15:52 ` Ian Campbell
2015-07-11 6:36 ` Julien Grall
2015-07-14 9:24 ` Vijay Kilari
2015-07-14 9:50 ` Ian Campbell
2015-07-14 9:59 ` Vijay Kilari
2015-07-10 16:07 ` Ian Campbell
2015-07-11 7:18 ` Julien Grall [this message]
2015-07-11 9:21 ` Ian Campbell
2015-07-13 21:58 ` Julien Grall
2015-07-14 10:02 ` Vijay Kilari
2015-07-14 13:31 ` Ian Campbell
2015-07-13 13:55 ` Wei Liu
2015-07-13 13:56 ` Wei Liu
2015-07-13 17:24 ` Stefano Stabellini
2015-07-13 21:50 ` Julien Grall
2015-07-14 7:49 ` Ian Campbell
2015-07-14 10:51 ` Stefano Stabellini
2015-07-16 14:08 ` Wei Liu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55A0C352.9050100@citrix.com \
--to=julien.grall@citrix.com \
--cc=Prasun.Kapoor@caviumnetworks.com \
--cc=Wei.Liu2@citrix.com \
--cc=ian.campbell@citrix.com \
--cc=manish.jaggi@caviumnetworks.com \
--cc=stefano.stabellini@citrix.com \
--cc=stefano.stabellini@eu.citrix.com \
--cc=vijay.kilari@gmail.com \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.