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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> >> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> >>
> >> ---
> >>     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.


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


> > 
> >>     Changes in v3:
> >>         - Functions to allocate/release/reserved a VIRQ has been moved
> >>         in a separate patch
> > 
> > That might be a good idea, but then you need to move that patch before
> > this one, otherwise it won't compile. As is it would break the build.
> 
> This patch belongs to a separate patch series. FIY, on the cover letter
> I explicitly wrote the dependency in other to apply this series.
 
OK

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