From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mario Smarduch Subject: Re: [PATCH 2/2] add initial kvm dev passhtrough support Date: Wed, 12 Jun 2013 08:56:21 +0200 Message-ID: <51B81B95.4090406@huawei.com> References: <51B6D52E.4030707@huawei.com> <2DDB038789B01B4B80B0D3F1FF7CBDC2214E9024@lhreml509-mbb.china.huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "christoffer.dall@linaro.com" , Marc Zyngier , "kvmarm@lists.cs.columbia.edu" , "linux-arm-kernel@lists.infradead.org" , "kvm@vger.kernel.org" To: Antonios Motakis Return-path: Received: from lhrrgout.huawei.com ([194.213.3.17]:11618 "EHLO lhrrgout.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755094Ab3FLG5U (ORCPT ); Wed, 12 Jun 2013 02:57:20 -0400 In-Reply-To: <2DDB038789B01B4B80B0D3F1FF7CBDC2214E9024@lhreml509-mbb.china.huawei.com> Sender: kvm-owner@vger.kernel.org List-ID: Resending, initial email from my exchange client got rejected due to HTML content On 6/12/2013 8:45 AM, Mario Smarduch wrote: > =20 >=20 Hi Antonios,=20 thanks for your feedback, initially we=92ll work with static bindi= ng gain performance data given latency/throughput is key, later add d= ynamic binding (as well as re-optimize affinity code). And as you already know move towards VFIO, which is a longer term effort. >=20 >=20 > +struct kvm_arm_assigned_dev_kernel { > + struct list_head list; > + struct kvm_arm_assigned_device dev; > + irqreturn_t (*irq_handler)(int, void *); > + void *irq_arg; > +}; > + >=20 > =20 >=20 > Instead of irq_arg, isn't something such as target_vcpu more clear? >=20 > =20 >=20 MS> Agree. >=20 > =20 >=20 > diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c > index 17c5ac7..f4cb804 100644 > --- a/arch/arm/kvm/vgic.c > +++ b/arch/arm/kvm/vgic.c > @@ -449,6 +449,41 @@ static u32 vgic_get_target_reg(struct kvm *k= vm, int irq) > return val; > } >=20 > +/* Follow the IRQ vCPU affinity so passthrough device interrupts= are injected > + * on physical CPU they execute. > + */ > +static void vgic_set_passthru_affinity(struct kvm *kvm, int irq,= u32 target) > +{ > + struct list_head *dev_list_ptr =3D &kvm->arch.assigned_de= v_head; > + struct list_head *ptr; > + struct kvm_arm_assigned_dev_kernel *assigned_dev; > + struct vgic_dist *dist =3D &kvm->arch.vgic; > + char *buf; > + int cpu, hwirq; > + > + mutex_lock(&kvm->arch.dev_pasthru_lock); > + list_for_each(ptr, dev_list_ptr) { > + assigned_dev =3D list_entry(ptr, > + struct kvm_arm_assigned_dev_kerne= l, list); > + if (assigned_dev->dev.guest_res.girq =3D=3D irq) = { > + if (assigned_dev->irq_arg) > + free_irq(irq, assigned_dev->irq_a= rg); > + cpu =3D kvm->vcpus[target]->cpu; > + hwirq =3D assigned_dev->dev.dev_res.hosti= rq.hwirq; > + irq_set_affinity(hwirq, cpumask_of(cpu)); > + assigned_dev->irq_arg =3D kvm->vcpus[targ= et]; > + buf =3D assigned_dev->dev.dev_res.hostirq= =2Ehost_name; > + sprintf(buf, "%s-KVM Pass-through", > + assigned_dev->dev.dev_res= =2Edevname); > + gic_spi_set_priodrop(hwirq); > + dist->guest_irq[hwirq - VGIC_NR_PRIVATE_I= RQS] =3D irq; > + request_irq(hwirq, assigned_dev->irq_hand= ler, 0, buf, > + assigned_= dev->irq_arg); > + } > + } > + mutex_unlock(&kvm->arch.dev_pasthru_lock); > +} > + >=20 > =20 >=20 > Maybe vgic_set_pasthru_affinity is not an ideal name for the function= , since you do more than that here. >=20 > After looking at your code I think things will be much easier if you = decouple the host irq affinity bits from here. After that there is not = much stopping from affinity following the CPU a vCPU will execute. >=20 > I would rename this to something to reflect that you enable priodrop = for this IRQ here, for example only vgic_set_passthrough could suffice = (I'm don't like the pasthru abbreviation a lot). Then the affinity bits= can be put in a different function. >=20 > =20 >=20 MJS> Agree naming could be better. >=20 >=20 >=20 > In arch/arm/kvm/arm.c kvm_arch_vcpu_load() you can follow up whenever= a vcpu is moved to a different cpu. However in practice I don't know i= f the additional complexity of having the irq affinity follow the vcpu = significantly improves irq latency. >=20 > =20 >=20 MJS> This should save a costly IPI if for example Phys IRQ is taken on= CPU 0 and target vCPU on CPU 1. I agree kvm_arch_vcpu_load() is a good place = if you=20 let vCPUs float. vigic_set_passthrough_affinity can be optimized more t= o eliminate=20 the free_irq(), requesnt_irq(). For now it=92s a simple implementation = we=92re assuming static binding, start gathering performance/latency data.=20 Will change the name as you suggest. >=20 >=20 >=20 >=20 > --=20 >=20 > *Antonios Motakis*, Virtual Open Systems* > */Open Source KVM Virtualization Development > /www.virtualopensystems.com >=20