Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support
From: Avi Kivity
Date: Mon Oct 05 2015 - 07:47:26 EST
On 10/05/2015 02:41 PM, Vlad Zolotarov wrote:
On 10/05/15 13:57, Greg KH wrote:
On Mon, Oct 05, 2015 at 01:48:39PM +0300, Vlad Zolotarov wrote:
On 10/05/15 10:56, Greg KH wrote:
On Mon, Oct 05, 2015 at 10:41:39AM +0300, Vlad Zolotarov wrote:
+struct msix_info {
+ int num_irqs;
+ struct msix_entry *table;
+ struct uio_msix_irq_ctx {
+ struct eventfd_ctx *trigger; /* MSI-x vector to
eventfd */
Why are you using eventfd for msi vectors? What's the reason for
needing this?
A small correction - for MSI-X vectors. There may be only one MSI
vector per
PCI function and if it's used it would use the same interface as a
legacy
INT#x interrupt uses at the moment.
So, for MSI-X case the reason is that there may be (in most cases
there will
be) more than one interrupt vector. Thus, as I've explained in a
PATCH1
thread we need a way to indicated each of them separately. eventfd
seems
like a good way of doing so. If u have better ideas, pls., share.
You need to document what you are doing here, I don't see any
explaination for using eventfd at all.
And no, I don't know of any other solution as I don't know what you
are
trying to do here (hint, the changelog didn't document it...)
You haven't documented how this api works at all, you are going
to have
to a lot more work to justify this, as this greatly increases the
complexity of the user/kernel api in unknown ways.
I actually do documented it a bit. Pls., check PATCH3 out.
That provided no information at all about how to use the api.
If it did, you would see that your api is broken for 32/64bit kernels
and will fall over into nasty pieces the first time you try to use it
there, which means it hasn't been tested at all :(
It has been tested of course ;)
I tested it only in 64 bit environment however where both kernel and
user
space applications were compiled on the same machine with the same
compiler
and it could be that "int" had the same number of bytes both in
kernel and
in user space application. Therefore it worked perfectly - I patched
DPDK to
use the new uio_pci_generic MSI-X API to test this and I have
verified that
all 3 interrupt modes work: MSI-X with SR-IOV VF device in Amazon
EC2 guest
and INT#x and MSI with a PF device on bare metal server.
However I agree using uint32_t for "vec" and "fd" would be much more
correct.
I don't think file descriptors are __u32 on a 64bit arch, are they?
I think they are "int" on all platforms and as far as I know u32
should be enough to contain int on any platform.
You need to make sure structures have the same layout on both 32-bit and
64-bit systems, or you'll have to code compat ioctl translations for
them. The best way to do that is to use __u32 so the sizes are obvious,
even for int, and to pad everything to 64 bit:
+struct msix_info {
+ __u32 num_irqs;
+ __u32 pad; // so pointer below is aligned to 64-bit on both 32-bit
and 64-bit userspace
+ struct msix_entry *table;
+ struct uio_msix_irq_ctx {
+ struct eventfd_ctx *trigger; /* MSI-x vector to eventfd */
And NEVER use the _t types in kernel code,
Never meant it - it was for a user space interface. For a kernel it's
u32 of course.
For interfaces, use __u32. You can't use uint32_t because if someone
uses C89 in 2015, they may not have <cstdint.h>.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/