Re: [PATCH v3 2/3] uio_pci_generic: add MSI/MSI-X support

From: Vlad Zolotarov
Date: Mon Oct 05 2015 - 07:53:35 EST




On 10/05/15 14:47, Avi Kivity wrote:
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:

Sure, but the structure below is not the one that is passed in ioctl() - it's an internal uio_pci_generic state and there is nothing to worry about.
The one in question is struct uio_pci_generic_irq_set from uio_pci_generic.h:

struct uio_pci_generic_irq_set {
int vec; /* index of the IRQ to connect to starting from 0 */
int fd;
};

It should be
struct uio_pci_generic_irq_set {
__u32 vec; /* index of the IRQ to connect to starting from 0 */
__u32 fd;
};

instead.



+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/