Re: [PATCH v3 13/24] xen/arm: Implement hypercall PHYSDEVOP_{, un}map_pirq

[Julien Grall <julien.grall@linaro.org>]

On 29/01/15 12:17, Stefano Stabellini wrote:
> On Wed, 28 Jan 2015, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 28/01/15 18:52, Stefano Stabellini wrote:
>>> On Tue, 13 Jan 2015, Julien Grall wrote:
>>>> The physdev sub-hypercalls PHYSDEVOP_{,map}_pirq allow the toolstack to
>>>> assign/deassign a physical IRQ to the guest (via the config options "irqs"
>>>> for xl). The x86 version is using them with PIRQ (IRQ bound to an event
>>>> channel). As ARM doesn't have a such concept, we could reuse it to bound
>>>> a physical IRQ to a virtual IRQ.
>>>>
>>>> For now, we allow only SPIs to be mapped to the guest.
>>>> The type MAP_PIRQ_TYPE_GSI is used for this purpose.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>>> Cc: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> ---
>>>>     I'm not sure it's the best solution to reuse hypercalls for a
>>>>     different purpose. If x86 plan to have a such concept (i.e binding a
>>>>     physical IRQ to a virtual IRQ), we could introduce new hypercalls.
>>>>     Any thoughs?
>>>
>>> I think it is OK, as long as we write down very clearly what we are
>>> doing.
>>>
>>>
>>>>     TODO: This patch is lacking of support of vIRQ != IRQ. I plan to
>>>>     handle it correctly on the next version.
>>>
>>> Why do you say that? From the code in this patch it looks like it
>>> supports vIRQ != IRQ already.
>>
>> Because PHYSDEV_map_pirq is taking a vIRQ number in parameter. This vIRQ
>> is only valid for the domain which issue the hypercall.
> 
> That's not very useful. I think that the vIRQ passed to PHYSDEV_map_pirq
> should be a vIRQ in the destination domain, not the source domain.
> 
> In fact on x86 the pirq parameter to PHYSDEV_map_pirq is interpreted as
> pirq in the destination domain too.

I'm talking about the index parameter. It's a vIRQ in the domain issue
the hypercall not the real IRQ.

>> In our use case, it's DOM0. DOM0 may not have all the time vIRQ == IRQ.
>>
>> Futhermore, on PHYSDEV_unmap_pirq I assume the DOM0 virq == guest virq.
> 
> That's bad.

I plan to support it for the next series. This change shouldn't impact
the other patches of the series, so I decided to send a new version to
gather some comments.

Regards,

-- 
Julien Grall