Re: [RFC PATCH v2 13/22] xen/arm: its: Add virtual ITS command support

[Julien Grall <julien.grall@citrix.com>]

Hello,

On 29/04/15 02:44, Vijay Kilari wrote:
> On Tue, Apr 28, 2015 at 9:45 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> On 28/04/15 12:36, Vijay Kilari wrote:
>>> On Tue, Apr 28, 2015 at 4:05 PM, Julien Grall <julien.grall@citrix.com> wrote:
>>>> If you properly manage the device with struct pci_dev or struct device
>>>> (which is, as talked earlier, obviously required for security) you
>>>> should avoid your so-called "dummy device". BTW, what do you mean by
>>>> "dummy device"?
>>>
>>>
>>> (a) For implementing ITS command processing completion interrupt we need
>>> a unique interrupt for each domain per vITS to update corresponding virtual ITS
>>> CREADER
>>> (b) INT command requires dev,ID there needs to be a device
>>> associated with the ID
>>> (c) The command processing completion interrupt is not coming from a
>>> valid device, we have to provide a dummy device,ID
>>> (d) I propose that the dummy device segment number is read from a
>>> macro/helper function
>>> in the platform file.
>>> For each domain we can add the bus number so for eg: 0xff is the segment
>>> number which is #define PLAT_DUMMY_SEG 0xff.
>>> The device for dom0 would be PLAT_DUMMY_SEG:00:0.0
>>> The device for domU would be PLAT_DUMMY_SEG:00:0.0 | domain_id
>>
>> There is multiple problem with this solution:
>>         - What prevents a platform to use this Device ID in the future?
> 
> Nothing prevents. But we can make a check in the ITS driver.

The number of Devbits is not fixed so you can't hardcode them (See
GITS_TYPER.Devbits).

> 
>>         - What's is the behavior of the ITS when the Device ID doesn't belong
>> to a real device?
>             ITS behavior is same for any device ID provided it falls
> in Devbits range

Can you give a reference from the spec?

>>
>>> Let me know if there is better way to generate dummy/unused device id?
>>>
>>> So creation of dummy device and setup for INT command execution
>>> can be done in physical ITS driver with its_device structure managed
>>> in vgic_its
>>>
>>> Also with this approach, vITS is not held by the VCPU till the completion
>>> of command processing, So another VCPU of the same domain can add
>>> another ITS command. If so we have to keep track number of ITS commands
>>> being processed per VCPU of the domain and increment vITS CREADER accordingly.
>>> For this, we have to add one unique interrupt ID of the device for per
>>> VCPU, So that
>>> unique interrupt is received from the dummy device for per VCPU.
>>
>> The number of LPIs supported by the ITS could be very limited. We need
>> to use them with parsimony.
> 
> Per device we can generate upto 2K MSIx. With dummy device we can
> use without restriction.

The number of MSI supported by the device is not the problem... The
problem is number of LPIs supported by the ITS (see GITS.IDbits).

> Managing all this complexity, What is the problem with Approach 2?

The problem is the polling in EL2 for several reasons:
  1) The VCPU is not preemptible when running in EL2. So the scheduler
can't schedule another VCPU on this physical CPU.
  2) The guest VCPU may want to execute other code while waiting the
completion of vITS. For instance because he choose to use receive an
interrupt for completion. I talked about it longer on previous mails.

> We have time bound polling which loops only for few ms

The few ms would be transformed to several seconds if the guest sends
lots of commands.

Furthermore, are you sure that few ms is enough? Linux seems to wait up
to 1s for each command...

> and that too
> ITS is not in critical path.
> It is only used when configuring interrupts of the device?

You need to think about security... Even though the ITS should only be
used for configuring interrupts, a malicious guest could try to exploit
weakness in the emulation.

As the 2 suggested approach don't seem to fit our usage, we need to find
another approach.

Regards,

-- 
Julien Grall