Re: [PATCH v4 07/17] xen/arm: ITS: Add virtual ITS commands support

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

On 17/07/15 15:12, Vijay Kilari wrote:
> Hi Julien,

Hi Vijay,

> On Wed, Jul 15, 2015 at 6:27 PM, Julien Grall <julien.grall@citrix.com> wrote:
>> On 10/07/2015 09:42, vijay.kilari@gmail.com wrote:
>>>
>>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>
>>> Add Virtual ITS command processing support to Virtual ITS driver
>>>
>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>>
>>> ---
>>> v4: - Use helper function to read from command queue
>>>      - Add MOVALL
>>>      - Removed check for entry in device in domain RB-tree
>>> ---
> [..]
>>> +
>>> +static int vgic_its_process_mapvi(struct vcpu *v, struct vgic_its *vits,
>>> +                                  its_cmd_block *virt_cmd)
>>> +{
>>> +    struct vitt entry;
>>> +    struct domain *d = v->domain;
>>> +    uint8_t vcol_id, cmd;
>>> +    uint32_t vid, dev_id, event;
>>> +
>>> +    vcol_id = virt_cmd->mapvi.col;
>>> +    vid = virt_cmd->mapvi.phy_id;
>>> +    cmd = virt_cmd->mapvi.cmd;
>>> +    dev_id = virt_cmd->mapvi.devid;
>>> +
>>> +    DPRINTK("%pv: vITS: MAPVI: dev_id 0x%x vcol_id %d vid %d \n",
>>> +             v, dev_id, vcol_id, vid);
>>> +
>>> +    if ( vcol_id > (d->max_vcpus + 1) ||  vid > its_get_nr_events() )
>>> +        return -EINVAL;
>>
>>
>> As said on v3, checking the validity is pointless as a malicious guest can
>> rewrite the
>> ITT. We only need to check it when the LPI is effectively injected.
>>
>> If you think this is necessary please explain why...
> 
>    vcol_id is read from ITS command but not from guest memory.
> So command values are validated  here instead of doing at time time
> of LPI injection.
> 
> If not done here, then we still allow malicious guest to run and during
> LPI injection if invalid col_id is found in ITT we just drop LPI.

You are storing the collection in the ITT. The ITT is part of the guest
memory.

The guest can modify at his convenience the region before we receive an
LPI and handle it. So we *must* check the validity of the collection
(and anything else in the ITT entry) everytime we receveive an LPI.

Therefore this check here is pointless.

>>
>> Furthermore, its_get_nr_events is for the hardware and not the virtual ITS.
>> I would prefer to see a field in the vits structure which contains the
>> number of event ID bits for a given domain.
> 
>  Why do we need to restrict number of LPIs to domain?

Why do you speak about LPIs here? its_get_nr_events return the maximum
number of eventID and not intID. LPIs are validate against the later.

Although, to answer to your question, the reason are various, one of
them is to save Xen memory, everytime we are creating domain we have to
allocate a pending_irq structure per  vLPIs.

It's pointless to allocate all of them if we know that only a few will
be used.

FWIW, we are doing the same for SPIs.

> [...]
> 
>>> +
>>> +    if ( vits_get_vitt_entry(d, dev_id, event, &vitt_entry) )
>>> +        return -EINVAL;
>>> +
>>> +    if ( !vitt_entry.valid )
>>> +    {
>>> +        dprintk(XENLOG_G_ERR,
>>> +                "%pv: vITS: INT CMD invalid event %d for dev 0x%x\n",
>>> +                v, event, dev_id);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    col_id = vitt_entry.vcollection;
>>> +    if ( col_id < d->max_vcpus )
>>> +    {
>>> +        dprintk(XENLOG_G_ERR,
>>> +                "%pv: vITS: INT CMD invalid col_id  %d for dev 0x%x\n",
>>> +                v, col_id, dev_id);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    vgic_vcpu_inject_irq(d->vcpu[col_id], vitt_entry.vlpi);
>>
>>
>> As said on v3, the design document [1] suggested to implement the INT
>> command using vgic_vcpu_inject_lpi. Is there any issue to do it?
> 
> IIRC,  INT command contains vlpi which does not have its_device because irq_desc
> is not reserved for this. Hence it is handled similar to event_ch int.

Why do you need an irq_desc in vgic_vcpu_inject_lpi? If you follow what
Ian suggested on patch #12 [1], you can directly call the function and
avoid to duplicate most of the code.

> 
>>
>> Also, you have to translate the col_id into to a VCPU ID.
> 
> This is virtual collection id which itself is vcpu id. isn't it?

No. A collection can move between CPU, so you will end up to have vCPUID
!= vCID.

Your MAPC command is correctly storing the mapping in collections, so
please use it to retrieve the correct vCPU.

Regards,

[1] http://lists.xen.org/archives/html/xen-devel/2015-07/msg02160.html


-- 
Julien Grall