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

[Stefano Stabellini <sstabellini@kernel.org>]

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@arm.com>
> >> ---
> >>  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@arm.com.


> 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. GITS_TYPER is
emulated by Xen in the virtual ITS, right? If so, it doesn't matter the
hardware value of SEIS, we can set the virtual value to 1.


> So any idea here? I don't think the typical Xen answer of "Crash the
> guest!" is compliant with the architecture, which leaves three choices,
> and setting the box on fire is not one of them.
> 
> That's why I chose to ignore the return value at this point, but at
> least generate the error condition internally and bail out early. At
> least for the Tech Preview Dom0 edition of the ITS emulation.
> If we later gain a good method of handling (and testing!) command
> errors, we can easily add them.

At the very least, we need to print a warning (rate limited, so probably
gdprintk). All errors that cannot be handled otherwise need to result in
a warning.

Sending an SError would be fine, I think.

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