| Message ID | 1421159133-31526-14-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 19/01/15 16:54, Jan Beulich wrote: >>>> On 13.01.15 at 15:25, <julien.grall@linaro.org> 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'm not sure either - much depends on the possible confusion this > may cause to the callers (i.e. if they live in common code, they > may expect the hypercall to do a certain thing, whereas if the > callers are all [expected to be] in arch code, then I'd consider such > overloading okay). PHYSDEVOP_{,un}map_pirq hypercalls are used in common code such as libxl (tools/libxl/libxc_create.c and pci code). There is a similar problem with XEN_DOMCTL_irq_permission. AFAIK, Linux is also using PHYSDEVOP_{,un}map_pirq to map an interrupt into itself. It may have confusion, if we decide to implement PIRQ in ARM. I believe it will never happen because the interrupt can be delivered via a virtual interface. Regards,
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. 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. > >> 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. Regards,
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,
Hi Ian, On 20/02/15 16:53, Ian Campbell wrote: > On Thu, 2015-01-29 at 12:33 +0000, Stefano Stabellini wrote: >> On Thu, 29 Jan 2015, Julien Grall wrote: >>> 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. > > Would adding MAP_PIRQ_TYPE_PPI (even as an alias for TYPE_GSI) be > helpful? > > I have a feeling not, since type is, I think, declaring the "namespace" > of the index parameter, whereas the pirq is the one containing the vGIC > provided virq (or the pirq-type evtchn on x86). Does that make sense? > > Are we absolutely 100% sure that we will never ever want to map hardware > IRQs to guests on ARMs using pirq-type event channels? Because that is > what we are essentially ruling out here. That would happen if we decide to implement an interrupt controller which doesn't support virtualization. Regards,
Hi Ian, On 23/02/15 15:28, Ian Campbell wrote: > On Mon, 2015-02-23 at 09:33 +0000, Jan Beulich wrote: >>>>> On 20.02.15 at 17:53, <ian.campbell@citrix.com> wrote: >>> Jan, do you have any feeling for how this is going to play out on x86 >>> with the vapic stuff? >> >> The vapic logic shouldn't require any physdevop involvement, so if >> I read right what you propose (not having such a requirement / >> connection on ARM) either, I agree that a new domctl should be all >> that's needed (if XEN_DOMCTL_{,un}bind_pt_irq can't be re-used). > > Actually, I think bind_pt_irq with a new PT_IRQ_TYPE_* would be a good > option. > > An ARM SPI is a bit like an ISA IRQ, but not close enough to reuse IMHO > (and the datatype would need widening). We have to think about MSI and other type too... In any case a DOMCTL is not suitable here because a guest kernel may need to map/unmap IRQ too (think about ACPI or device passthrough). Regards,
On 23/02/15 16:04, Ian Campbell wrote: > On Mon, 2015-02-23 at 15:53 +0000, Julien Grall wrote: >> Hi Ian, >> >> On 23/02/15 15:28, Ian Campbell wrote: >>> On Mon, 2015-02-23 at 09:33 +0000, Jan Beulich wrote: >>>>>>> On 20.02.15 at 17:53, <ian.campbell@citrix.com> wrote: >>>>> Jan, do you have any feeling for how this is going to play out on x86 >>>>> with the vapic stuff? >>>> >>>> The vapic logic shouldn't require any physdevop involvement, so if >>>> I read right what you propose (not having such a requirement / >>>> connection on ARM) either, I agree that a new domctl should be all >>>> that's needed (if XEN_DOMCTL_{,un}bind_pt_irq can't be re-used). >>> >>> Actually, I think bind_pt_irq with a new PT_IRQ_TYPE_* would be a good >>> option. >>> >>> An ARM SPI is a bit like an ISA IRQ, but not close enough to reuse IMHO >>> (and the datatype would need widening). >> >> We have to think about MSI and other type too... >> >> In any case a DOMCTL is not suitable here because a guest kernel may >> need to map/unmap IRQ too (think about ACPI or device passthrough). > > I don't follow, setting up device passthrough is very much a toolstack > operation, isn't it? Where does the guest kernel get involved? Sorry I meant the DOM0 kernel. Not really. On platform device pass-through there is no way to know the IRQ, so for now the routing is done by the toolstack. But we could decide to implement a driver in DOM0 which will unbind/bind/reset device. In this case it will require to assign/deassign the IRQ from DOM0. There is also the case of MSI. > As for ACPI, I think dom0 propagating ACPI derived platform info to Xen > should be handled differently (at the hypercall interface at least) > separate from passthrough. There is no difference between routing because of ACPI and/or because pass-through. So this should be done the same way. Regards,
Hi Ian, On 23/02/2015 16:34, Ian Campbell wrote: > On Mon, 2015-02-23 at 16:11 +0000, Julien Grall wrote: >> On 23/02/15 16:04, Ian Campbell wrote: >>> On Mon, 2015-02-23 at 15:53 +0000, Julien Grall wrote: >>>> Hi Ian, >>>> >>>> On 23/02/15 15:28, Ian Campbell wrote: >>>>> On Mon, 2015-02-23 at 09:33 +0000, Jan Beulich wrote: >>>>>>>>> On 20.02.15 at 17:53, <ian.campbell@citrix.com> wrote: >>>>>>> Jan, do you have any feeling for how this is going to play out on x86 >>>>>>> with the vapic stuff? >>>>>> >>>>>> The vapic logic shouldn't require any physdevop involvement, so if >>>>>> I read right what you propose (not having such a requirement / >>>>>> connection on ARM) either, I agree that a new domctl should be all >>>>>> that's needed (if XEN_DOMCTL_{,un}bind_pt_irq can't be re-used). >>>>> >>>>> Actually, I think bind_pt_irq with a new PT_IRQ_TYPE_* would be a good >>>>> option. >>>>> >>>>> An ARM SPI is a bit like an ISA IRQ, but not close enough to reuse IMHO >>>>> (and the datatype would need widening). >>>> >>>> We have to think about MSI and other type too... >>>> >>>> In any case a DOMCTL is not suitable here because a guest kernel may >>>> need to map/unmap IRQ too (think about ACPI or device passthrough). >>> >>> I don't follow, setting up device passthrough is very much a toolstack >>> operation, isn't it? Where does the guest kernel get involved? >> >> Sorry I meant the DOM0 kernel. >> >> Not really. On platform device pass-through there is no way to know the >> IRQ, so for now the routing is done by the toolstack. >> >> But we could decide to implement a driver in DOM0 which will >> unbind/bind/reset device. > > Sure, but... > >> In this case it will require to >> assign/deassign the IRQ from DOM0. > > ...why does that follow? Because we may decide to re-use the device to DOM0. In the case of the DOMCTL, it's not possible to do it. >> There is also the case of MSI. > > Handled via XEN_DOMCTL_bind_pt_irq for the toolstack configuration > angle, the actual guest usage of them is a separate interface which > doesn't yet concern us, at least not in this series. That would require some rework in the toolstack to make the IRQ routing (xc_physdev...) per architecture. Also what about IRQ permission? Should we just ignore it and not give permission to the guest domain? >>> As for ACPI, I think dom0 propagating ACPI derived platform info to Xen >>> should be handled differently (at the hypercall interface at least) >>> separate from passthrough. >> >> There is no difference between routing because of ACPI and/or because >> pass-through. So this should be done the same way. > > I'm not convinced. Routing all the IRQs is only one aspect of dom0 > propagating ACPI derived platform info to Xen. > > I suppose we will see once I look at the ACPI series. In the meantime I > think XEN_DOMCTL_bind_pt_irq matches your requirements in for this > series (and is a domctl so we aren't tied to it once we have a better > understanding of the other stuff). ACPI is one part of the problem, MSI with PCI is another problem. Anyway, I suspect we will have the same talk before 4.6 release so we just defer the problem. Regards,
Hi, On 23/02/15 16:02, Ian Campbell wrote: > On Mon, 2015-02-23 at 15:51 +0000, Julien Grall wrote: >> On 20/02/15 16:53, Ian Campbell wrote: > >>> Are we absolutely 100% sure that we will never ever want to map hardware >>> IRQs to guests on ARMs using pirq-type event channels? Because that is >>> what we are essentially ruling out here. >> >> That would happen if we decide to implement an interrupt controller >> which doesn't support virtualization. > > Good point. It's pretty unlikely but not absolutely impossible. So we > should avoid reusing the pirq evtchn type for this. Jan suggested > XENDOMCTL_bind_pt_irq which is looking better and better... I looked to the interface of XENDOMCTL_bind_pt_irq and I'm not sure about the meaning of machine_irq and isa_irq. AFAIU the code: machine_irq => guest PIRQ isa_irq => host IRQ am I right? Regards,
On 04/03/15 14:55, Jan Beulich wrote: >>>> On 04.03.15 at 15:37, <julien.grall@linaro.org> wrote: >> I looked to the interface of XENDOMCTL_bind_pt_irq and I'm not sure >> about the meaning of machine_irq and isa_irq. >> >> AFAIU the code: >> machine_irq => guest PIRQ > > Yes (i.e. the Xen assigned representation of an IRQ the guest has > been granted access to). Thank you for the confirmation. >> isa_irq => host IRQ > > But only for ISA ones, not PCI (i.e. you likely don't care about this > at all). > > Note that the PCI case here still lacks a segment number - if you in > the end decided you want/need non-zero segments on ARM, then > this will need extension (which it would need anyway if anyone was > serious about to running Xen on such [x86] hardware). I don't care about PCI right now. I'm looking for mapping SPI (kind of ISA) to the guest. I'm planning to introduce a new type of this purpose. Regards,
diff --git a/xen/arch/arm/physdev.c b/xen/arch/arm/physdev.c index 61b4a18..0cf9bbd 100644 --- a/xen/arch/arm/physdev.c +++ b/xen/arch/arm/physdev.c @@ -8,13 +8,145 @@ #include <xen/types.h> #include <xen/lib.h> #include <xen/errno.h> +#include <xen/iocap.h> +#include <xen/guest_access.h> +#include <xsm/xsm.h> +#include <asm/current.h> #include <asm/hypercall.h> +#include <public/physdev.h> +static int physdev_map_pirq(domid_t domid, int type, int index, int *pirq_p) +{ + struct domain *d; + int ret; + int irq = index; + int virq; + + d = rcu_lock_domain_by_any_id(domid); + if ( d == NULL ) + return -ESRCH; + + ret = xsm_map_domain_pirq(XSM_TARGET, d); + if ( ret ) + goto free_domain; + + /* For now we only suport GSI */ + if ( type != MAP_PIRQ_TYPE_GSI ) + { + ret = -EINVAL; + dprintk(XENLOG_G_ERR, + "dom%u: wrong map_pirq type 0x%x, only MAP_PIRQ_TYPE_GSI is supported.\n", + d->domain_id, type); + goto free_domain; + } + + if ( !is_assignable_irq(irq) ) + { + ret = -EINVAL; + dprintk(XENLOG_G_ERR, "IRQ%u is not routable to a guest\n", irq); + goto free_domain; + } + + ret = -EPERM; + if ( !irq_access_permitted(current->domain, irq) ) + goto free_domain; + + if ( *pirq_p < 0 ) + { + BUG_ON(irq < 16); /* is_assignable_irq already denies SGIs */ + virq = vgic_allocate_virq(d, (irq >= 32)); + + ret = -ENOSPC; + if ( virq < 0 ) + goto free_domain; + } + else + { + ret = -EBUSY; + virq = *pirq_p; + + if ( !vgic_reserve_virq(d, virq) ) + goto free_domain; + } + + gdprintk(XENLOG_DEBUG, "irq = %u virq = %u\n", irq, virq); + + ret = route_irq_to_guest(d, virq, irq, "routed IRQ"); + + if ( !ret ) + *pirq_p = virq; + else + vgic_free_virq(d, virq); + +free_domain: + rcu_unlock_domain(d); + + return ret; +} + +int physdev_unmap_pirq(domid_t domid, int pirq) +{ + struct domain *d; + int ret; + + d = rcu_lock_domain_by_any_id(domid); + if ( d == NULL ) + return -ESRCH; + + ret = xsm_unmap_domain_pirq(XSM_TARGET, d); + if ( ret ) + goto free_domain; + + ret = release_guest_irq(d, pirq); + if ( ret ) + goto free_domain; + + vgic_free_virq(d, pirq); + +free_domain: + rcu_unlock_domain(d); + + return ret; +} int do_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg) { - printk("%s %d cmd=%d: not implemented yet\n", __func__, __LINE__, cmd); - return -ENOSYS; + int ret; + + switch ( cmd ) + { + case PHYSDEVOP_map_pirq: + { + physdev_map_pirq_t map; + + ret = -EFAULT; + if ( copy_from_guest(&map, arg, 1) != 0 ) + break; + + ret = physdev_map_pirq(map.domid, map.type, map.index, &map.pirq); + + if ( __copy_to_guest(arg, &map, 1) ) + ret = -EFAULT; + } + break; + + case PHYSDEVOP_unmap_pirq: + { + physdev_unmap_pirq_t unmap; + + ret = -EFAULT; + if ( copy_from_guest(&unmap, arg, 1) != 0 ) + break; + + ret = physdev_unmap_pirq(unmap.domid, unmap.pirq); + } + + default: + ret = -ENOSYS; + break; + } + + return ret; } /*
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? TODO: This patch is lacking of support of vIRQ != IRQ. I plan to handle it correctly on the next version. Changes in v3: - Functions to allocate/release/reserved a VIRQ has been moved in a separate patch - Make clear that only MAP_PIRQ_GSI is only supported for now Changes in v2: - Add PHYSDEVOP_unmap_pirq - Rework commit message - Add functions to allocate/release a VIRQ - is_routable_irq has been renamed into is_assignable_irq --- xen/arch/arm/physdev.c | 136 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 134 insertions(+), 2 deletions(-)