Re: [Xen-devel] [PATCH v2 16/27] ARM: vITS: handle CLEAR command

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 27/03/17 09:44, Andre Przywara wrote:
> Hi,
>
> On 24/03/17 17:17, Stefano Stabellini wrote:
> > On Fri, 24 Mar 2017, Andre Przywara wrote:
> > > Hi,
> > >
> > > On 24/03/17 14:27, Julien Grall wrote:
> > > > Hi Andre,
> > > >
> > > > On 03/16/2017 11:20 AM, Andre Przywara wrote:
> > > > > This introduces the ITS command handler for the CLEAR command, which
> > > > > clears the pending state of an LPI.
> > > > > This removes a not-yet injected, but already queued IRQ from a VCPU.
> > > > >
> > > > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > > > ---
> > > > >  xen/arch/arm/vgic-v3-its.c | 35 +++++++++++++++++++++++++++++++++--
> > > > >  1 file changed, 33 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> > > > > index 267a573..e808f43 100644
> > > > > --- a/xen/arch/arm/vgic-v3-its.c
> > > > > +++ b/xen/arch/arm/vgic-v3-its.c
> > > [....]
> > >
> > > > > @@ -236,6 +264,9 @@ static int vgic_its_handle_cmds(struct domain *d,
> > > > > struct virt_its *its,
> > > > >          cmdptr = its->cmdbuf + (its->creadr / sizeof(*its->cmdbuf));
> > > > >          switch (its_cmd_get_command(cmdptr))
> > > > >          {
> > > > > +        case GITS_CMD_CLEAR:
> > > > > +            its_handle_clear(its, cmdptr);
> > > > Should not you check the return for its_handle_clear?
> > > That sounds obvious, but actually I don't know of a good way of handling
> > > this. Blame the architecture, if you like. Passing the error value up
> > > would end up in the MMIO handler, where it is not correct to return an
> > > error (since the CWRITER MMIO access itself was successful).
> > >
> > > So I picked one of the behaviors described in 6.3.2 "Command errors",
> > > which is simply to ignore the command.
> > > If we have a nice way of injecting an SError (do we?),
> > We do with Wei's series, which is very likely to go in before this one:
> >
> > http://marc.info/?l=xen-devel&m=148940261914581
> >
> > In particular, see 1489402563-4978-7-git-send-email-Wei.Chen@xxxxxxx.
> >
> >
> > > I _could_ check GITS_TYPER.SEIS and then inject it. But effectively
> > > this would be untested code, since Linux does not use this feature.
> > Keep in mind that Xen supports a wide range of OSes.
> I clearly understand that and don't question it. What I wanted to point
> out is that using an SError to signal ITS errors is mostly uncharted
> territory (see the current discussion about handling SErrors in
> Linux[1]). So we would add code that cannot be tested.
The SError is sent or not, it is not important whether the OS is able to 
handle it or not.
And justifying with "the code cannot be tested" is not argument as I 
would have expected you to hack Linux to exercise the vITS.
> And given the
> current situation and the tech preview status of the ITS support I'd
> prefer to not go there at the moment.
>
> I would offer to annotate the error returns with the actual ITS error
> codes (as in the KVM code, for instance [2]).
I am happy with that as a first step.

> Then put a comment in the code explaining the missing error signalling
> situation, and we create a ticket to notify ourselves of fixing this in
> the future.
I think we still need to log the error. It is useful for the user to 
know what's going on. You could imagine someone trying to implement an 
OS using Xen and KVM. He would be really grateful to know what's going on.
Cheers,

--
Julien Grall

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