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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel