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

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

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
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel