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

[Andre Przywara <andre.przywara@arm.com>]

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

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. 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]).
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.
Does that make sense?

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

OK, I will do this.

> Sending an SError would be fine, I think.

As mentioned above, I'd refrain from it at the moment.

Cheers,
Andre.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-March/496722.html
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/virt/kvm/arm/vgic/vgic-its.c#n561

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