Requesting for freeze exception for ARM/ITS patches

Requesting for freeze exception for ARM/ITS patches

[Vijay Kilari]

Hi Wei,

    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.

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.

These patches are pre-requisite for PCI support / Pass-through support
on ARM64 platforms.

The risk is minor and as of today only used by Cavium ThunderX platform.

Regards
Vijay

Re: Requesting for freeze exception for ARM/ITS patches

[Jan Beulich]

>>> On 10.07.15 at 12:46, <vijay.kilari@gmail.com> 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.
> 
> 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.
> 
> These patches are pre-requisite for PCI support / Pass-through support
> on ARM64 platforms.

But what good will it do for the release when it's only a prereq for
further work?

Jan

Re: Requesting for freeze exception for ARM/ITS patches

[Ian Campbell]

On Fri, 2015-07-10 at 12:01 +0100, Jan Beulich wrote:
> >>> On 10.07.15 at 12:46, <vijay.kilari@gmail.com> 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.
> > 
> > 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.
> > 
> > These patches are pre-requisite for PCI support / Pass-through support
> > on ARM64 platforms.
> 
> But what good will it do for the release when it's only a prereq for
> further work?

I think this may have been true of v3 but I think it is underselling v4.
AFAICT with the final patch in v4 Xen will actually be capable of
booting on a ThunderX platform and starting guests. It will simply lack
PCI passthrough capabilities (as do all ARM platforms today).

I think that's a reasonable thing to try and get into 4.6.

Ian.

Re: Requesting for freeze exception for ARM/ITS patches

[Julien Grall]

Hi,

On 10/07/2015 17:52, Ian Campbell wrote:
> On Fri, 2015-07-10 at 12:01 +0100, Jan Beulich wrote:
>>>>> On 10.07.15 at 12:46, <vijay.kilari@gmail.com> 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.
>>>
>>> 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.
>>>
>>> These patches are pre-requisite for PCI support / Pass-through support
>>> on ARM64 platforms.
>>
>> But what good will it do for the release when it's only a prereq for
>> further work?
>
> I think this may have been true of v3 but I think it is underselling v4.
> AFAICT with the final patch in v4 Xen will actually be capable of
> booting on a ThunderX platform and starting guests. It will simply lack
> PCI passthrough capabilities (as do all ARM platforms today).

I think you are overselling this patch series. PCI devices provide the 
support of both legacy interrupt and MSI.

The former is handled by your PCI patch series [1] pushed upstreamed few 
days ago. This should be enough for booting ThunderX and creating guests.

The ITS add the support of MSI which is useful for performance question 
and PCI passthrough.

> I think that's a reasonable thing to try and get into 4.6.

If we ever decide to get this into 4.6 we need some guarantee from 
Cavium to finish the work afterwards.

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-07/msg00656.html

-- 
Julien Grall

Re: Requesting for freeze exception for ARM/ITS patches

[Vijay Kilari]

Hi Ian,

On Sat, Jul 11, 2015 at 12:06 PM, Julien Grall <julien.grall@citrix.com> wrote:
> Hi,
>
> On 10/07/2015 17:52, Ian Campbell wrote:
>>
>> On Fri, 2015-07-10 at 12:01 +0100, Jan Beulich wrote:
>>>>>>
>>>>>> On 10.07.15 at 12:46, <vijay.kilari@gmail.com> 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.
>>>>
>>>> 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.
>>>>
>>>> These patches are pre-requisite for PCI support / Pass-through support
>>>> on ARM64 platforms.
>>>
>>>
>>> But what good will it do for the release when it's only a prereq for
>>> further work?
>>
>>
>> I think this may have been true of v3 but I think it is underselling v4.
>> AFAICT with the final patch in v4 Xen will actually be capable of
>> booting on a ThunderX platform and starting guests. It will simply lack
>> PCI passthrough capabilities (as do all ARM platforms today).
>
>
> I think you are overselling this patch series. PCI devices provide the
> support of both legacy interrupt and MSI.
>
> The former is handled by your PCI patch series [1] pushed upstreamed few
> days ago. This should be enough for booting ThunderX and creating guests.
>
> The ITS add the support of MSI which is useful for performance question and
> PCI passthrough.
>
>> I think that's a reasonable thing to try and get into 4.6.
>
>
> If we ever decide to get this into 4.6 we need some guarantee from Cavium to
> finish the work afterwards.
>
> Regards,
>
> [1] http://lists.xen.org/archives/html/xen-devel/2015-07/msg00656.html

I am trying to boot latest staging Xen branch on ThunderX with ITS patches.

I face below issues with above [1] patch series

1)  If pcie support only MSI, then INT mapping is not specified in DT. However
  the below code returns error if INT mapping is not found and does not map.
In ThunderX INT mapping is not specified for pcie nodes.

static int map_device_children(struct domain *d,
                                const struct dt_device_node *dev)
{
     int ret;

     if ( dt_device_type_is_equal(dev, "pci") )
     {
         DPRINT("Mapping children of %s to guest\n", dt_node_full_name(dev));

         ret = dt_for_each_irq_map(dev, &map_dt_irq_to_domain, d);
         if ( ret < 0 )
             return ret;  // Returns error here
...
}

2) Dom0 fails to boot with GICv3. It hangs just after GICv3 initialization.

To know which is last distributor/re-distributor registers read, I
have added debug prints
in GICD & GICR mmio handlers in Xen. But with your patches Linux driver never
traps to Xen to read/write GICD/GICR registers. If I revert back this
patch series,
I see the traps.

Regards
Vijay

Re: Requesting for freeze exception for ARM/ITS patches

[Ian Campbell]

On Tue, 2015-07-14 at 14:54 +0530, Vijay Kilari wrote:
> I am trying to boot latest staging Xen branch on ThunderX with ITS patches.
> 
> I face below issues with above [1] patch series
> 
> 1)  If pcie support only MSI, then INT mapping is not specified in DT. However
>   the below code returns error if INT mapping is not found and does not map.
> In ThunderX INT mapping is not specified for pcie nodes.
> 
> static int map_device_children(struct domain *d,
>                                 const struct dt_device_node *dev)
> {
>      int ret;
> 
>      if ( dt_device_type_is_equal(dev, "pci") )
>      {
>          DPRINT("Mapping children of %s to guest\n", dt_node_full_name(dev));
> 
>          ret = dt_for_each_irq_map(dev, &map_dt_irq_to_domain, d);
>          if ( ret < 0 )
>              return ret;  // Returns error here

Hrm, I suppose dt_for_each_irq_map ought to return success if the device
in question has no interrupt-map at all.

At first glance it seems like:
    if ( imap == NULL )
    {
        dt_dprintk(" -> no map, ignoring\n");
        goto fail;
    }
Should become:
    if ( imap == NULL )
    {
        dt_dprintk(" -> no map, ignoring\n");
        return 0;
    }

Can you test that and if it is correct submit it as a patch please.

> ...
> }
> 
> 2) Dom0 fails to boot with GICv3. It hangs just after GICv3 initialization.
> 
> To know which is last distributor/re-distributor registers read, I
> have added debug prints
> in GICD & GICR mmio handlers in Xen. But with your patches Linux driver never
> traps to Xen to read/write GICD/GICR registers. If I revert back this
> patch series,
> I see the traps.

Where "this patch series" is this:

$ git log --oneline d7f132c762d1359f03b2b5b89406daf39d8aefc0..467e5cbb2ffc5e0994c4cb584b7ace6a01a727af 
467e5cb xen: arm: consolidate mmio and irq mapping to dom0
f65399f xen: arm: Import of_bus PCI entry from Linux (as a dt_bus entry)
864f82a xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
eed5e39 xen: arm: drop redundant extra call to vgic_reserve_virq
f9d08f4 xen: dt: add dt_for_each_range helper
5cefb30 xen: dt: add dt_for_each_irq_map helper
$

?

That's rather strange, nothing here should be interacting with the vgic
trapping for GICD/GICR.

About the only thing I can imagine is that something has incorrectly
created a p2m mapping over the GICD/GICR addresses. If that is the case
then it ought to be pretty apparent from the logs with DT_DEBUG enabled
in domain_build.c.

If it isn't apparent from the debug log then please could you bisect
down to a specific patch.

Ian.

Re: Requesting for freeze exception for ARM/ITS patches

[Vijay Kilari]

On Tue, Jul 14, 2015 at 3:20 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2015-07-14 at 14:54 +0530, Vijay Kilari wrote:
>> I am trying to boot latest staging Xen branch on ThunderX with ITS patches.
>>
>> I face below issues with above [1] patch series
>>
>> 1)  If pcie support only MSI, then INT mapping is not specified in DT. However
>>   the below code returns error if INT mapping is not found and does not map.
>> In ThunderX INT mapping is not specified for pcie nodes.
>>
>> static int map_device_children(struct domain *d,
>>                                 const struct dt_device_node *dev)
>> {
>>      int ret;
>>
>>      if ( dt_device_type_is_equal(dev, "pci") )
>>      {
>>          DPRINT("Mapping children of %s to guest\n", dt_node_full_name(dev));
>>
>>          ret = dt_for_each_irq_map(dev, &map_dt_irq_to_domain, d);
>>          if ( ret < 0 )
>>              return ret;  // Returns error here
>
> Hrm, I suppose dt_for_each_irq_map ought to return success if the device
> in question has no interrupt-map at all.

Yes, I do so.

>
> At first glance it seems like:
>     if ( imap == NULL )
>     {
>         dt_dprintk(" -> no map, ignoring\n");
>         goto fail;
>     }
> Should become:
>     if ( imap == NULL )
>     {
>         dt_dprintk(" -> no map, ignoring\n");
>         return 0;
>     }
>
> Can you test that and if it is correct submit it as a patch please.
>
>> ...
>> }
>>
>> 2) Dom0 fails to boot with GICv3. It hangs just after GICv3 initialization.
>>
>> To know which is last distributor/re-distributor registers read, I
>> have added debug prints
>> in GICD & GICR mmio handlers in Xen. But with your patches Linux driver never
>> traps to Xen to read/write GICD/GICR registers. If I revert back this
>> patch series,
>> I see the traps.
>
> Where "this patch series" is this:
>
> $ git log --oneline d7f132c762d1359f03b2b5b89406daf39d8aefc0..467e5cbb2ffc5e0994c4cb584b7ace6a01a727af
> 467e5cb xen: arm: consolidate mmio and irq mapping to dom0
> f65399f xen: arm: Import of_bus PCI entry from Linux (as a dt_bus entry)
> 864f82a xen: arm: map child MMIO and IRQs to dom0 for PCI bus DT nodes.
> eed5e39 xen: arm: drop redundant extra call to vgic_reserve_virq
> f9d08f4 xen: dt: add dt_for_each_range helper
> 5cefb30 xen: dt: add dt_for_each_irq_map helper
> $
>
> ?
>
> That's rather strange, nothing here should be interacting with the vgic
> trapping for GICD/GICR.
>
> About the only thing I can imagine is that something has incorrectly
> created a p2m mapping over the GICD/GICR addresses. If that is the case
> then it ought to be pretty apparent from the logs with DT_DEBUG enabled
> in domain_build.c.
>
> If it isn't apparent from the debug log then please could you bisect
> down to a specific patch.

I found the reason. There is some discrepancy with DT which is overlapping
with GIC address space. It took a day to figure out.

Re: Requesting for freeze exception for ARM/ITS patches

[Ian Campbell]

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.

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.

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

If you can get us a v5 with all of the issues I pointed out done _early_
next week then, with my ARM maintainers hat on, I think a freeze
exception could be something which could then be something which could
realistically be considered.

However the final call on this belongs to Wei.

Note I tried to apply this series to xen.git#staging and it had rejects,
I think it was written against #master and will need rebasing. Please
base v5 on #staging instead.

Ian.

> The risk is minor and as of today only used by Cavium ThunderX platform.
> 
> Regards
> Vijay

Re: Requesting for freeze exception for ARM/ITS patches

[Julien Grall]

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

Re: Requesting for freeze exception for ARM/ITS patches

[Ian Campbell]

On Sat, 2015-07-11 at 09:18 +0200, Julien Grall wrote:
> > 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.

AIUI ThunderX's on-SoC PCI devices do not provide legacy PCI INTX
interrupts, only MSI/LPI interrupts. Perhaps someone from Cavium can
confirm whether or not this is the case.

Ian.

Re: Requesting for freeze exception for ARM/ITS patches

[Julien Grall]

Hi,

On 11/07/2015 11:21, Ian Campbell wrote:
> On Sat, 2015-07-11 at 09:18 +0200, Julien Grall wrote:
>>> 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.
>
> AIUI ThunderX's on-SoC PCI devices do not provide legacy PCI INTX
> interrupts, only MSI/LPI interrupts. Perhaps someone from Cavium can
> confirm whether or not this is the case.

I haven't found any PCI controller in the Linux upstream device tree 
bindings of cavium. Although I found a thread for 2014 about it [1].

As the thread is quite old now, can some from Cavium confirm this will 
be valid on the production hardware?

Regards,

[1] https://lkml.org/lkml/2014/9/25/73

-- 
Julien Grall

Re: Requesting for freeze exception for ARM/ITS patches

[Vijay Kilari]

On Sat, Jul 11, 2015 at 12:48 PM, Julien Grall <julien.grall@citrix.com> 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

Re: Requesting for freeze exception for ARM/ITS patches

[Ian Campbell]

On Tue, 2015-07-14 at 15:32 +0530, Vijay Kilari wrote:
> 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.

_If_ v5 fixes the issues which Stefano and I have highlighted as being
required for 4.6 then I expect I would be able to support a freeze
exception for this code.

In that case I think we can take it on trust that you will fix any
remaining issues in 4.7.

In any case, even if the ITS code is not granted a freeze exception it
is still worth iterating on this code until it is such a state that it
could be committed to trunk (for 4.7) as soon as that development window
opens. Once it is ready I can keep it in my queue until the tree
reopens. So there is no need to stop development activities because of
the freeze.

Ian.


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

Re: Requesting for freeze exception for ARM/ITS patches

[Wei Liu]

On Fri, Jul 10, 2015 at 04:16:07PM +0530, Vijay Kilari wrote:
> Hi Wei,
> 
>     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.
> 
> 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.
> 
> These patches are pre-requisite for PCI support / Pass-through support
> on ARM64 platforms.
> 
> The risk is minor and as of today only used by Cavium ThunderX platform.
> 


I'm not a ARM expert, but last time I checked most patches in v3 are not
acked.

I also got conflict statements from maintainers and core developer. I
will wait a bit for them clarify the situation.

But as Ian said, if you can't post v4 and get most if all of your
patches acked / reviewed early this week, my answer to this request
would be no.

Wei.

> Regards
> Vijay

Re: Requesting for freeze exception for ARM/ITS patches

[Wei Liu]

On Mon, Jul 13, 2015 at 02:55:04PM +0100, Wei Liu wrote:
> On Fri, Jul 10, 2015 at 04:16:07PM +0530, Vijay Kilari wrote:
> > Hi Wei,
> > 
> >     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.
> > 
> > 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.
> > 
> > These patches are pre-requisite for PCI support / Pass-through support
> > on ARM64 platforms.
> > 
> > The risk is minor and as of today only used by Cavium ThunderX platform.
> > 
> 
> 
> I'm not a ARM expert, but last time I checked most patches in v3 are not
> acked.

I meant v4. Sorry.

Re: Requesting for freeze exception for ARM/ITS patches

[Stefano Stabellini]

On Mon, 13 Jul 2015, Wei Liu wrote:
> On Fri, Jul 10, 2015 at 04:16:07PM +0530, Vijay Kilari wrote:
> > Hi Wei,
> > 
> >     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.
> > 
> > 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.
> > 
> > These patches are pre-requisite for PCI support / Pass-through support
> > on ARM64 platforms.
> > 
> > The risk is minor and as of today only used by Cavium ThunderX platform.
> > 
> 
> 
> I'm not a ARM expert, but last time I checked most patches in v3 are not
> acked.
> 
> I also got conflict statements from maintainers and core developer. I
> will wait a bit for them clarify the situation.
> 
> But as Ian said, if you can't post v4 and get most if all of your
> patches acked / reviewed early this week, my answer to this request
> would be no.

I pretty much agree with Ian: I went through the patches and the impact
of the series on non-ITS platforms will be null after Vijay addresses:

- the lpi irq_desc and irq_pending allocation issues
- improve lpi_supported to check for ITS presence

these two changes should be trivial and are certainly necessary for a
freeze exception in my view.


On this basis, if Vijay manages to resend a v5 version on time with
those two issues covered, making sure that the new code is not enabled
unless an its is present, then I think that a freeze exception would be
OK as the risk would be zero.

Re: Requesting for freeze exception for ARM/ITS patches

[Julien Grall]

Hi,

On 13/07/2015 19:24, Stefano Stabellini wrote:
> On Mon, 13 Jul 2015, Wei Liu wrote:
>> On Fri, Jul 10, 2015 at 04:16:07PM +0530, Vijay Kilari wrote:
>>> Hi Wei,
>>>
>>>      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.
>>>
>>> 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.
>>>
>>> These patches are pre-requisite for PCI support / Pass-through support
>>> on ARM64 platforms.
>>>
>>> The risk is minor and as of today only used by Cavium ThunderX platform.
>>>
>>
>>
>> I'm not a ARM expert, but last time I checked most patches in v3 are not
>> acked.
>>
>> I also got conflict statements from maintainers and core developer. I
>> will wait a bit for them clarify the situation.
>>
>> But as Ian said, if you can't post v4 and get most if all of your
>> patches acked / reviewed early this week, my answer to this request
>> would be no.
>
> I pretty much agree with Ian: I went through the patches and the impact
> of the series on non-ITS platforms will be null after Vijay addresses:
>
> - the lpi irq_desc and irq_pending allocation issues
> - improve lpi_supported to check for ITS presence
>
> these two changes should be trivial and are certainly necessary for a
> freeze exception in my view.

IHMO, this is not the only two changes that should be done in order to 
grant a freeze exception.

Until now, all the implementation specific to the hardware has been 
separated by callback called from the generic code. This is important 
for the maintainability of the project, make the code readable and 
easier to modify.

While I agree that it would be nice to see this patch series in Xen 4.6, 
I think there is too much ITS code in the common code.

For instance, there is vits_init function called directly from setup.c 
which look like a violation layer of the generic code for the GIC.

Furthermore, this version of the patch series reuse some function for a 
completely different behavior: I have in mind route_irq_to_guest where 
the primary goal is to route an IRQ to a vIRQ. With one line change (and 
not the parameter renamed), it has been reused to assign an LPI to the 
guest and virq re-used as event ID.

This is 2 different meanings which make possible misuse of the function 
and/or would introduce bug if changes of the implementation is 
necessary. I made this point on the v3 [1], but unfortunately I didn't 
see any change in the v4.

Finally, there is some assumption in the code that only the hardware 
domain will be used (see #15 where a 1:1 mapping is used) but not 
documented. This is a call for unwanted bug when guest ITS will 
supported. Note that, I already made this comment on the v1 and v2 of 
this series.

I will also mention the assumption on the behavior of some functions 
that may not work on LPI and will do right things by luck (see 
release_irq_to_guest).

I don't even talk about the assumption on the behavior of certain 
functions that may not work on LPI by luck (see release_irq_to_guest).

I believe that we should keep the code comprehensible and have a good 
code quality. We took time to do it when GICv3 has been introduced. We 
should do the same for the ITS, even though it means to wait the next 
release.

I'm not asking for a perfect series, but at least keep the ITS code in 
the ITS files and not spreading the hardware specific code accross 
multiple Xen common code file.

Note that, I'm not a maintainers, I will let them and the release 
manager take the final decision.

Regards,

-- 
Julien Grall

Re: Requesting for freeze exception for ARM/ITS patches

[Ian Campbell]

On Mon, 2015-07-13 at 18:24 +0100, Stefano Stabellini wrote:
> On Mon, 13 Jul 2015, Wei Liu wrote:
> > On Fri, Jul 10, 2015 at 04:16:07PM +0530, Vijay Kilari wrote:
> > > Hi Wei,
> > > 
> > >     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.
> > > 
> > > 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.
> > > 
> > > These patches are pre-requisite for PCI support / Pass-through support
> > > on ARM64 platforms.
> > > 
> > > The risk is minor and as of today only used by Cavium ThunderX platform.
> > > 
> > 
> > 
> > I'm not a ARM expert, but last time I checked most patches in v3 are not
> > acked.
> > 
> > I also got conflict statements from maintainers and core developer. I
> > will wait a bit for them clarify the situation.
> > 
> > But as Ian said, if you can't post v4 and get most if all of your
> > patches acked / reviewed early this week, my answer to this request
> > would be no.
> 
> I pretty much agree with Ian: I went through the patches and the impact
> of the series on non-ITS platforms will be null after Vijay addresses:
> 
> - the lpi irq_desc and irq_pending allocation issues
> - improve lpi_supported to check for ITS presence
> 
> these two changes should be trivial and are certainly necessary for a
> freeze exception in my view.
> 
> 
> On this basis, if Vijay manages to resend a v5 version on time with
> those two issues covered, making sure that the new code is not enabled
> unless an its is present, then I think that a freeze exception would be
> OK as the risk would be zero.

I don't think we should be limiting ourselves to only fixing issues
which reduce the risk on non-ITS platforms. So the two issues which you
highlight above are necessary but not sufficient for a freeze exception
in my view.

For example I am firmly of the opinion that the VPLI injection code
needs to be corrected as discussed during review.

Likewise I said that care needs to be taken wrt when any of this code is
enabled, which includes not exposing it to domU even on platforms which
support ITS. I also view this as a requirement for a freeze exception.
In other words only dom0 and only on an ITS enabled system should be
exposed to any aspect of the ITS support.

Ian.

Re: Requesting for freeze exception for ARM/ITS patches

[Stefano Stabellini]

On Tue, 14 Jul 2015, Ian Campbell wrote:
> On Mon, 2015-07-13 at 18:24 +0100, Stefano Stabellini wrote:
> > On Mon, 13 Jul 2015, Wei Liu wrote:
> > > On Fri, Jul 10, 2015 at 04:16:07PM +0530, Vijay Kilari wrote:
> > > > Hi Wei,
> > > > 
> > > >     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.
> > > > 
> > > > 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.
> > > > 
> > > > These patches are pre-requisite for PCI support / Pass-through support
> > > > on ARM64 platforms.
> > > > 
> > > > The risk is minor and as of today only used by Cavium ThunderX platform.
> > > > 
> > > 
> > > 
> > > I'm not a ARM expert, but last time I checked most patches in v3 are not
> > > acked.
> > > 
> > > I also got conflict statements from maintainers and core developer. I
> > > will wait a bit for them clarify the situation.
> > > 
> > > But as Ian said, if you can't post v4 and get most if all of your
> > > patches acked / reviewed early this week, my answer to this request
> > > would be no.
> > 
> > I pretty much agree with Ian: I went through the patches and the impact
> > of the series on non-ITS platforms will be null after Vijay addresses:
> > 
> > - the lpi irq_desc and irq_pending allocation issues
> > - improve lpi_supported to check for ITS presence
> > 
> > these two changes should be trivial and are certainly necessary for a
> > freeze exception in my view.
> > 
> > 
> > On this basis, if Vijay manages to resend a v5 version on time with
> > those two issues covered, making sure that the new code is not enabled
> > unless an its is present, then I think that a freeze exception would be
> > OK as the risk would be zero.
> 
> I don't think we should be limiting ourselves to only fixing issues
> which reduce the risk on non-ITS platforms. So the two issues which you
> highlight above are necessary but not sufficient for a freeze exception
> in my view.
> 
> For example I am firmly of the opinion that the VPLI injection code
> needs to be corrected as discussed during review.
> 
> Likewise I said that care needs to be taken wrt when any of this code is
> enabled, which includes not exposing it to domU even on platforms which
> support ITS. I also view this as a requirement for a freeze exception.
> In other words only dom0 and only on an ITS enabled system should be
> exposed to any aspect of the ITS support.
 
I agree.

Re: Requesting for freeze exception for ARM/ITS patches

[Wei Liu]

On Fri, Jul 10, 2015 at 04:16:07PM +0530, Vijay Kilari wrote:
> Hi Wei,
> 
>     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.
> 
> 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.
> 
> These patches are pre-requisite for PCI support / Pass-through support
> on ARM64 platforms.
> 
> The risk is minor and as of today only used by Cavium ThunderX platform.
> 

I haven't seen a new series with all comments addressed at this stage.
I'm afraid I can't grant this series freeze exception. But keep in mind
you can continue to work on this series and get it applied once next
development window opens.

Wei.

> Regards
> Vijay