| Message ID | 1667894937-175291-1-git-send-email-chenxiang66@hisilicon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Add system call KVM_VERIFY_MSI to verify MSI vector | expand |
On Tue, 08 Nov 2022 08:08:57 +0000, chenxiang <chenxiang66@hisilicon.com> wrote: > > From: Xiang Chen <chenxiang66@hisilicon.com> > > Currently the numbers of MSI vectors come from register PCI_MSI_FLAGS > which should be power-of-2, but in some scenaries it is not the same as > the number that driver requires in guest, for example, a PCI driver wants > to allocate 6 MSI vecotrs in guest, but as the limitation, it will allocate > 8 MSI vectors. So it requires 8 MSI vectors in qemu while the driver in > guest only wants to allocate 6 MSI vectors. > > When GICv4.1 is enabled, we can see some exception print as following for > above scenaro: > vfio-pci 0000:3a:00.1: irq bypass producer (token 000000008f08224d) registration fails:66311 > > In order to verify whether a MSI vector is valid, add KVM_VERIFY_MSI to do > that. If there is a mapping, return 0, otherwise return negative value. > > This is the kernel part of adding system call KVM_VERIFY_MSI. Exposing something that is an internal implementation detail to userspace feels like the absolute wrong way to solve this issue. Can you please characterise the issue you're having? Is it that vfio tries to enable an interrupt for which there is no virtual ITS mapping? Shouldn't we instead try and manage this in the kernel? Thanks, M.
Hi chenxiang,
I love your patch! Yet something to improve:
[auto build test ERROR on kvm/queue]
[also build test ERROR on kvmarm/next mst-vhost/linux-next linus/master v6.1-rc4 next-20221108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/chenxiang/KVM-Add-system-call-KVM_VERIFY_MSI-to-verify-MSI-vector/20221108-155249
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link: https://lore.kernel.org/r/1667894937-175291-1-git-send-email-chenxiang66%40hisilicon.com
patch subject: [PATCH] KVM: Add system call KVM_VERIFY_MSI to verify MSI vector
config: i386-randconfig-a003
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/c10a785951f2d779fc45561d86948685e03c6cf2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review chenxiang/KVM-Add-system-call-KVM_VERIFY_MSI-to-verify-MSI-vector/20221108-155249
git checkout c10a785951f2d779fc45561d86948685e03c6cf2
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
>> ERROR: modpost: "kvm_verify_msi" [arch/x86/kvm/kvm.ko] undefined!
Hi chenxiang,
I love your patch! Yet something to improve:
[auto build test ERROR on kvm/queue]
[also build test ERROR on kvmarm/next mst-vhost/linux-next linus/master v6.1-rc4 next-20221108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/chenxiang/KVM-Add-system-call-KVM_VERIFY_MSI-to-verify-MSI-vector/20221108-155249
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link: https://lore.kernel.org/r/1667894937-175291-1-git-send-email-chenxiang66%40hisilicon.com
patch subject: [PATCH] KVM: Add system call KVM_VERIFY_MSI to verify MSI vector
config: x86_64-randconfig-a014
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/c10a785951f2d779fc45561d86948685e03c6cf2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review chenxiang/KVM-Add-system-call-KVM_VERIFY_MSI-to-verify-MSI-vector/20221108-155249
git checkout c10a785951f2d779fc45561d86948685e03c6cf2
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: kvm_verify_msi
>>> referenced by kvm_main.c:4748 (arch/x86/kvm/../../../virt/kvm/kvm_main.c:4748)
>>> virt/kvm/kvm_main.o:(kvm_vm_ioctl) in archive vmlinux.a
pahole: .tmp_vmlinux.btf: No such file or directory
ld.lld: error: .btf.vmlinux.bin.o: unknown file type
Hi Marc, 在 2022/11/8 20:47, Marc Zyngier 写道: > On Tue, 08 Nov 2022 08:08:57 +0000, > chenxiang <chenxiang66@hisilicon.com> wrote: >> From: Xiang Chen <chenxiang66@hisilicon.com> >> >> Currently the numbers of MSI vectors come from register PCI_MSI_FLAGS >> which should be power-of-2, but in some scenaries it is not the same as >> the number that driver requires in guest, for example, a PCI driver wants >> to allocate 6 MSI vecotrs in guest, but as the limitation, it will allocate >> 8 MSI vectors. So it requires 8 MSI vectors in qemu while the driver in >> guest only wants to allocate 6 MSI vectors. >> >> When GICv4.1 is enabled, we can see some exception print as following for >> above scenaro: >> vfio-pci 0000:3a:00.1: irq bypass producer (token 000000008f08224d) registration fails:66311 >> >> In order to verify whether a MSI vector is valid, add KVM_VERIFY_MSI to do >> that. If there is a mapping, return 0, otherwise return negative value. >> >> This is the kernel part of adding system call KVM_VERIFY_MSI. > Exposing something that is an internal implementation detail to > userspace feels like the absolute wrong way to solve this issue. > > Can you please characterise the issue you're having? Is it that vfio > tries to enable an interrupt for which there is no virtual ITS > mapping? Shouldn't we instead try and manage this in the kernel? Before i reported the issue to community, you gave a suggestion about the issue, but not sure whether i misundertood your meaning. You can refer to the link for more details about the issue. https://lkml.kernel.org/lkml/87cze9lcut.wl-maz@kernel.org/T/ Best regards, Xiang
Hi chenxiang,
I love your patch! Yet something to improve:
[auto build test ERROR on kvm/queue]
[also build test ERROR on kvmarm/next mst-vhost/linux-next linus/master v6.1-rc4 next-20221108]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/chenxiang/KVM-Add-system-call-KVM_VERIFY_MSI-to-verify-MSI-vector/20221108-155249
base: https://git.kernel.org/pub/scm/virt/kvm/kvm.git queue
patch link: https://lore.kernel.org/r/1667894937-175291-1-git-send-email-chenxiang66%40hisilicon.com
patch subject: [PATCH] KVM: Add system call KVM_VERIFY_MSI to verify MSI vector
config: x86_64-allyesconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/c10a785951f2d779fc45561d86948685e03c6cf2
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review chenxiang/KVM-Add-system-call-KVM_VERIFY_MSI-to-verify-MSI-vector/20221108-155249
git checkout c10a785951f2d779fc45561d86948685e03c6cf2
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
ld: kernel image bigger than KERNEL_IMAGE_SIZE
ld: vmlinux.o: in function `kvm_vm_ioctl':
>> kvm_main.c:(.text+0x7e06b): undefined reference to `kvm_verify_msi'
On Wed, 09 Nov 2022 06:21:18 +0000, "chenxiang (M)" <chenxiang66@hisilicon.com> wrote: > > Hi Marc, > > > 在 2022/11/8 20:47, Marc Zyngier 写道: > > On Tue, 08 Nov 2022 08:08:57 +0000, > > chenxiang <chenxiang66@hisilicon.com> wrote: > >> From: Xiang Chen <chenxiang66@hisilicon.com> > >> > >> Currently the numbers of MSI vectors come from register PCI_MSI_FLAGS > >> which should be power-of-2, but in some scenaries it is not the same as > >> the number that driver requires in guest, for example, a PCI driver wants > >> to allocate 6 MSI vecotrs in guest, but as the limitation, it will allocate > >> 8 MSI vectors. So it requires 8 MSI vectors in qemu while the driver in > >> guest only wants to allocate 6 MSI vectors. > >> > >> When GICv4.1 is enabled, we can see some exception print as following for > >> above scenaro: > >> vfio-pci 0000:3a:00.1: irq bypass producer (token 000000008f08224d) registration fails:66311 > >> > >> In order to verify whether a MSI vector is valid, add KVM_VERIFY_MSI to do > >> that. If there is a mapping, return 0, otherwise return negative value. > >> > >> This is the kernel part of adding system call KVM_VERIFY_MSI. > > Exposing something that is an internal implementation detail to > > userspace feels like the absolute wrong way to solve this issue. > > > > Can you please characterise the issue you're having? Is it that vfio > > tries to enable an interrupt for which there is no virtual ITS > > mapping? Shouldn't we instead try and manage this in the kernel? > > Before i reported the issue to community, you gave a suggestion about > the issue, but not sure whether i misundertood your meaning. > You can refer to the link for more details about the issue. > https://lkml.kernel.org/lkml/87cze9lcut.wl-maz@kernel.org/T/ Right. It would have been helpful to mention this earlier. Anyway, I would really like this to be done without involving userspace at all. But first, can you please confirm that the VM works as expected despite the message? If that's the case, we only need to handle the case where this is a multi-MSI setup, and I think this can be done in VFIO, without involving userspace. Thanks, M.
Hi Marc, 在 2022/11/10 18:28, Marc Zyngier 写道: > On Wed, 09 Nov 2022 06:21:18 +0000, > "chenxiang (M)" <chenxiang66@hisilicon.com> wrote: >> Hi Marc, >> >> >> 在 2022/11/8 20:47, Marc Zyngier 写道: >>> On Tue, 08 Nov 2022 08:08:57 +0000, >>> chenxiang <chenxiang66@hisilicon.com> wrote: >>>> From: Xiang Chen <chenxiang66@hisilicon.com> >>>> >>>> Currently the numbers of MSI vectors come from register PCI_MSI_FLAGS >>>> which should be power-of-2, but in some scenaries it is not the same as >>>> the number that driver requires in guest, for example, a PCI driver wants >>>> to allocate 6 MSI vecotrs in guest, but as the limitation, it will allocate >>>> 8 MSI vectors. So it requires 8 MSI vectors in qemu while the driver in >>>> guest only wants to allocate 6 MSI vectors. >>>> >>>> When GICv4.1 is enabled, we can see some exception print as following for >>>> above scenaro: >>>> vfio-pci 0000:3a:00.1: irq bypass producer (token 000000008f08224d) registration fails:66311 >>>> >>>> In order to verify whether a MSI vector is valid, add KVM_VERIFY_MSI to do >>>> that. If there is a mapping, return 0, otherwise return negative value. >>>> >>>> This is the kernel part of adding system call KVM_VERIFY_MSI. >>> Exposing something that is an internal implementation detail to >>> userspace feels like the absolute wrong way to solve this issue. >>> >>> Can you please characterise the issue you're having? Is it that vfio >>> tries to enable an interrupt for which there is no virtual ITS >>> mapping? Shouldn't we instead try and manage this in the kernel? >> Before i reported the issue to community, you gave a suggestion about >> the issue, but not sure whether i misundertood your meaning. >> You can refer to the link for more details about the issue. >> https://lkml.kernel.org/lkml/87cze9lcut.wl-maz@kernel.org/T/ > Right. It would have been helpful to mention this earlier. Anyway, I > would really like this to be done without involving userspace at all. > > But first, can you please confirm that the VM works as expected > despite the message? Yes, it works well except the message. > If that's the case, we only need to handle the > case where this is a multi-MSI setup, and I think this can be done in > VFIO, without involving userspace. It seems we can verify every kvm_msi for multi-MSI setup in function vfio_pci_set_msi_trigger(). If it is a invalid MSI vector, then we can decrease the numer of MSI vectors before calling vfio_msi_set_block(). > > Thanks, > > M. >
diff --git a/arch/arm64/kvm/vgic/vgic-irqfd.c b/arch/arm64/kvm/vgic/vgic-irqfd.c index 475059b..2312da6 100644 --- a/arch/arm64/kvm/vgic/vgic-irqfd.c +++ b/arch/arm64/kvm/vgic/vgic-irqfd.c @@ -98,6 +98,11 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e, return vgic_its_inject_msi(kvm, &msi); } +int kvm_verify_msi(struct kvm *kvm, struct kvm_msi *msi) +{ + return vgic_its_verify_msi(kvm, msi); +} + /** * kvm_arch_set_irq_inatomic: fast-path for irqfd injection */ diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index 24d7778..cae6183 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -767,6 +767,42 @@ int vgic_its_inject_cached_translation(struct kvm *kvm, struct kvm_msi *msi) return 0; } +int vgic_its_verify_msi(struct kvm *kvm, struct kvm_msi *msi) +{ + struct vgic_its *its; + struct its_ite *ite; + struct kvm_vcpu *vcpu; + int ret = 0; + + if (!irqchip_in_kernel(kvm) || (msi->flags & ~KVM_MSI_VALID_DEVID)) + return -EINVAL; + + if (!vgic_has_its(kvm)) + return -ENODEV; + + its = vgic_msi_to_its(kvm, msi); + if (IS_ERR(its)) + return PTR_ERR(its); + + mutex_lock(&its->its_lock); + if (!its->enabled) { + ret = -EBUSY; + goto unlock; + } + ite = find_ite(its, msi->devid, msi->data); + if (!ite || !its_is_collection_mapped(ite->collection)) { + ret = -E_ITS_INT_UNMAPPED_INTERRUPT; + goto unlock; + } + + vcpu = kvm_get_vcpu(kvm, ite->collection->target_addr); + if (!vcpu) + ret = -E_ITS_INT_UNMAPPED_INTERRUPT; +unlock: + mutex_unlock(&its->its_lock); + return ret; +} + /* * Queries the KVM IO bus framework to get the ITS pointer from the given * doorbell address. diff --git a/arch/arm64/kvm/vgic/vgic.h b/arch/arm64/kvm/vgic/vgic.h index 0c8da72..d452150 100644 --- a/arch/arm64/kvm/vgic/vgic.h +++ b/arch/arm64/kvm/vgic/vgic.h @@ -240,6 +240,7 @@ int kvm_vgic_register_its_device(void); void vgic_enable_lpis(struct kvm_vcpu *vcpu); void vgic_flush_pending_lpis(struct kvm_vcpu *vcpu); int vgic_its_inject_msi(struct kvm *kvm, struct kvm_msi *msi); +int vgic_its_verify_msi(struct kvm *kvm, struct kvm_msi *msi); int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr); int vgic_v3_dist_uaccess(struct kvm_vcpu *vcpu, bool is_write, int offset, u32 *val); diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 32f259f..7923352 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1597,7 +1597,7 @@ void kvm_unregister_irq_ack_notifier(struct kvm *kvm, int kvm_request_irq_source_id(struct kvm *kvm); void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id); bool kvm_arch_irqfd_allowed(struct kvm *kvm, struct kvm_irqfd *args); - +int kvm_verify_msi(struct kvm *kvm, struct kvm_msi *msi); /* * Returns a pointer to the memslot if it contains gfn. * Otherwise returns NULL. diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index 0d5d441..72b28f8 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -1543,6 +1543,8 @@ struct kvm_s390_ucas_mapping { #define KVM_PPC_SVM_OFF _IO(KVMIO, 0xb3) #define KVM_ARM_MTE_COPY_TAGS _IOR(KVMIO, 0xb4, struct kvm_arm_copy_mte_tags) +#define KVM_VERIFY_MSI _IOW(KVMIO, 0xb5, struct kvm_msi) + /* ioctl for vm fd */ #define KVM_CREATE_DEVICE _IOWR(KVMIO, 0xe0, struct kvm_create_device) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index e30f1b4..439bdd7 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -4728,6 +4728,15 @@ static long kvm_vm_ioctl(struct file *filp, r = kvm_send_userspace_msi(kvm, &msi); break; } + case KVM_VERIFY_MSI: { + struct kvm_msi msi; + + r = -EFAULT; + if (copy_from_user(&msi, argp, sizeof(msi))) + goto out; + r = kvm_verify_msi(kvm, &msi); + break; + } #endif #ifdef __KVM_HAVE_IRQ_LINE case KVM_IRQ_LINE_STATUS: