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

Re: [Xen-devel] Requesting for freeze exception for ARM/ITS patches

On Sat, Jul 11, 2015 at 12:48 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> 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 can fix all the required issues required for 4.6 provided if we take
firm decision
that we will have freeze exception for ITS.

I can commit to finish/clean up any pending ITS TODO's

> 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

Xen-devel mailing list



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