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

[Andre Przywara <andre.przywara@xxxxxxx>]

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?), I _could_ check
GITS_TYPER.SEIS and then inject it. But effectively this would be
untested code, since Linux does not use this feature.

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.

Cheers,
Andre.

> 
>> +            break;
>>          case GITS_CMD_SYNC:
>>              /* We handle ITS commands synchronously, so we ignore
>> SYNC. */
>>          break;
>>
> 
> Regards,
> 

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