Re: [RFC PATCH 15/23] X86/vioapic: Hook interrupt delivery of vIOAPIC

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Apr 18, 2017 at 04:34:52PM +0800, Lan Tianyu wrote:
> On 2017年04月17日 22:43, Konrad Rzeszutek Wilk wrote:
> > On Fri, Mar 17, 2017 at 07:27:15PM +0800, Lan Tianyu wrote:
> >> From: Chao Gao <chao.gao@intel.com>
> >>
> >> When irq remapping enabled, IOAPIC Redirection Entry maybe is in remapping
> >> format. If that, generate a irq_remapping_request and send it to domain.
> >>
> >> Signed-off-by: Chao Gao <chao.gao@intel.com>
> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com>
> >> ---
> >>  xen/arch/x86/Makefile                  |  1 +
> >>  xen/arch/x86/hvm/vioapic.c             | 10 ++++++++++
> >>  xen/arch/x86/viommu.c                  | 30 ++++++++++++++++++++++++++++++
> >>  xen/include/asm-x86/viommu.h           |  3 +++
> >>  xen/include/public/arch-x86/hvm/save.h |  1 +
> >>  5 files changed, 45 insertions(+)
> >>  create mode 100644 xen/arch/x86/viommu.c
> >>
> >> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> >> index f75eca0..d49f8c8 100644
> >> --- a/xen/arch/x86/Makefile
> >> +++ b/xen/arch/x86/Makefile
> >> @@ -66,6 +66,7 @@ obj-y += usercopy.o
> >>  obj-y += x86_emulate.o
> >>  obj-$(CONFIG_TBOOT) += tboot.o
> >>  obj-y += hpet.o
> >> +obj-y += viommu.o
> >>  obj-y += vm_event.o
> >>  obj-y += xstate.o
> >>  
> >> diff --git a/xen/arch/x86/hvm/vioapic.c b/xen/arch/x86/hvm/vioapic.c
> >> index fdbb21f..6a00644 100644
> >> --- a/xen/arch/x86/hvm/vioapic.c
> >> +++ b/xen/arch/x86/hvm/vioapic.c
> >> @@ -30,6 +30,7 @@
> >>  #include <xen/lib.h>
> >>  #include <xen/errno.h>
> >>  #include <xen/sched.h>
> >> +#include <xen/viommu.h>
> >>  #include <public/hvm/ioreq.h>
> >>  #include <asm/hvm/io.h>
> >>  #include <asm/hvm/vpic.h>
> >> @@ -285,9 +286,18 @@ static void vioapic_deliver(struct hvm_hw_vioapic *vioapic, int irq)
> >>      struct domain *d = vioapic_domain(vioapic);
> >>      struct vlapic *target;
> >>      struct vcpu *v;
> >> +    struct irq_remapping_request request;
> >>  
> >>      ASSERT(spin_is_locked(&d->arch.hvm_domain.irq_lock));
> >>  
> >> +    if ( vioapic->redirtbl[irq].ir.format )
> >> +    {
> >> +        irq_request_ioapic_fill(&request, vioapic->id,
> >> +                                vioapic->redirtbl[irq].bits);
> >> +        viommu_handle_irq_request(d, &request);
> >> +        return;
> >> +    }
> >> +
> >>      HVM_DBG_LOG(DBG_LEVEL_IOAPIC,
> >>                  "dest=%x dest_mode=%x delivery_mode=%x "
> >>                  "vector=%x trig_mode=%x",
> >> diff --git a/xen/arch/x86/viommu.c b/xen/arch/x86/viommu.c
> >> new file mode 100644
> >> index 0000000..ef78d3b
> >> --- /dev/null
> >> +++ b/xen/arch/x86/viommu.c
> >> @@ -0,0 +1,30 @@
> >> +/*
> >> + * viommu.c
> >> + *
> >> + * virtualize IOMMU.
> >> + *
> >> + * Copyright (C) 2017 Chao Gao, Intel Corporation.
> >> + *
> >> + * This program is free software; you can redistribute it and/or
> >> + * modify it under the terms and conditions of the GNU General Public
> >> + * License, version 2, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope that it will be useful,
> >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> >> + * General Public License for more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public
> >> + * License along with this program; If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include <xen/viommu.h>
> >> +
> >> +void irq_request_ioapic_fill(struct irq_remapping_request *req,
> >> +                             uint32_t ioapic_id, uint64_t rte)
> >> +{
> >> +    ASSERT(req);
> >> +    req->type = VIOMMU_REQUEST_IRQ_APIC;
> >> +    req->source_id = ioapic_id;
> >> +    req->msg.rte = rte;
> > 
> > Considering we get 'req' from the stack and it may have garbage, would
> > it be good to fill out the rest of the entries with sensible values? Or
> > is there no need for that?
> 
> Both AMD and Intel will use the function to pass interrupt remapping
> request. I am afraid different vendors may have different IOAPIC
> remapping format. How about to parse and check remapping request data in
> the vendor vIOMMU device module(E,G vvtd)? :)

Yeah that is fine. Just please put the comment in the code saying
that is the justification - or alternatively put it in the
commit description.

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