Is msix_flush_writes() really needed? And multi_msi_*() flawed?

Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Jan Beulich]

msix_flush_writes() is being called exclusively after calling msi_set_mask_bit(),
and that function already does follow writel() by readl() in the MSI-X case.

Also, isn't the single use of multi_msi_capable() broken (in the event that
the Multiple Message Capable field was 5, the shift would be undefined,
on x86 in particular would yield 1 as the result, where 0 would be needed),
and the subsequent twiddling of temp needlessly complicated (subtracting
one should be sufficient here).

And isn't multi_msi_enable(), though unused (since msi_{en,dis}able() are
unused), broken altogether (shifting num right by 1 instead of taking the
binary log)?

In case so:

Signed-off-by: Jan Beulich <jbeulich@novell.com>

---
 drivers/pci/msi.c |   30 ++----------------------------
 drivers/pci/msi.h |    2 +-
 2 files changed, 3 insertions(+), 29 deletions(-)

--- linux-2.6.28-rc3/drivers/pci/msi.c	2008-11-03 15:53:11.000000000 +0100
+++ 2.6.28-rc3-pci-msi-misc/drivers/pci/msi.c	2008-11-07 09:11:36.000000000 +0100
@@ -103,29 +103,6 @@ static void msix_set_enable(struct pci_d
 	}
 }
 
-static void msix_flush_writes(unsigned int irq)
-{
-	struct msi_desc *entry;
-
-	entry = get_irq_msi(irq);
-	BUG_ON(!entry || !entry->dev);
-	switch (entry->msi_attrib.type) {
-	case PCI_CAP_ID_MSI:
-		/* nothing to do */
-		break;
-	case PCI_CAP_ID_MSIX:
-	{
-		int offset = entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
-			PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
-		readl(entry->mask_base + offset);
-		break;
-	}
-	default:
-		BUG();
-		break;
-	}
-}
-
 /*
  * PCI 2.3 does not specify mask bits for each MSI interrupt.  Attempting to
  * mask all MSI interrupts by clearing the MSI enable bit does not work
@@ -255,13 +232,11 @@ void write_msi_msg(unsigned int irq, str
 void mask_msi_irq(unsigned int irq)
 {
 	msi_set_mask_bits(irq, 1, 1);
-	msix_flush_writes(irq);
 }
 
 void unmask_msi_irq(unsigned int irq)
 {
 	msi_set_mask_bits(irq, 1, 0);
-	msix_flush_writes(irq);
 }
 
 static int msi_free_irqs(struct pci_dev* dev);
@@ -389,9 +364,8 @@ static int msi_capability_init(struct pc
 		pci_read_config_dword(dev,
 			msi_mask_bits_reg(pos, entry->msi_attrib.is_64),
 			&maskbits);
-		temp = (1 << multi_msi_capable(control));
-		temp = ((temp - 1) & ~temp);
-		maskbits |= temp;
+		temp = 1U << (multi_msi_capable(control) - 1);
+		maskbits |= (temp << 1) - 1;
 		pci_write_config_dword(dev, entry->msi_attrib.is_64, maskbits);
 		entry->msi_attrib.maskbits_mask = temp;
 	}
--- linux-2.6.28-rc3/drivers/pci/msi.h	2007-02-04 19:44:54.000000000 +0100
+++ 2.6.28-rc3-pci-msi-misc/drivers/pci/msi.h	2008-11-07 09:13:09.000000000 +0100
@@ -23,7 +23,7 @@
 #define multi_msi_capable(control) \
 	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
 #define multi_msi_enable(control, num) \
-	control |= (((num >> 1) << 4) & PCI_MSI_FLAGS_QSIZE);
+	control |= ((ilog2(num) << 4) & PCI_MSI_FLAGS_QSIZE)
 #define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
 #define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
 #define msi_enable(control, num) multi_msi_enable(control, num); \

Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Grant Grundler]

On Fri, Nov 07, 2008 at 08:53:49AM +0000, Jan Beulich wrote:
> msix_flush_writes() is being called exclusively after calling msi_set_mask_bit(),
> and that function already does follow writel() by readl() in the MSI-X case.

Yes - this part is correct.
If you care to split this into seperate parts, please add:
   Reviewed-by: Grant Grundler <grundler@parisc-linux.org>

for this chunk.

> Also, isn't the single use of multi_msi_capable() broken (in the event that
> the Multiple Message Capable field was 5, the shift would be undefined,
> on x86 in particular would yield 1 as the result, where 0 would be needed),
> and the subsequent twiddling of temp needlessly complicated (subtracting
> one should be sufficient here).

Comment on this chunk below.

> And isn't multi_msi_enable(), though unused (since msi_{en,dis}able() are
> unused), broken altogether (shifting num right by 1 instead of taking the
> binary log)?
> 
> In case so:
> 
> Signed-off-by: Jan Beulich <jbeulich@novell.com>
> 
> ---
>  drivers/pci/msi.c |   30 ++----------------------------
>  drivers/pci/msi.h |    2 +-
>  2 files changed, 3 insertions(+), 29 deletions(-)
> 
> --- linux-2.6.28-rc3/drivers/pci/msi.c	2008-11-03 15:53:11.000000000 +0100
> +++ 2.6.28-rc3-pci-msi-misc/drivers/pci/msi.c	2008-11-07 09:11:36.000000000 +0100
> @@ -103,29 +103,6 @@ static void msix_set_enable(struct pci_d
>  	}
>  }
>  
> -static void msix_flush_writes(unsigned int irq)
> -{
> -	struct msi_desc *entry;
> -
> -	entry = get_irq_msi(irq);
> -	BUG_ON(!entry || !entry->dev);
> -	switch (entry->msi_attrib.type) {
> -	case PCI_CAP_ID_MSI:
> -		/* nothing to do */
> -		break;
> -	case PCI_CAP_ID_MSIX:
> -	{
> -		int offset = entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
> -			PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
> -		readl(entry->mask_base + offset);
> -		break;
> -	}
> -	default:
> -		BUG();
> -		break;
> -	}
> -}
> -
>  /*
>   * PCI 2.3 does not specify mask bits for each MSI interrupt.  Attempting to
>   * mask all MSI interrupts by clearing the MSI enable bit does not work
> @@ -255,13 +232,11 @@ void write_msi_msg(unsigned int irq, str
>  void mask_msi_irq(unsigned int irq)
>  {
>  	msi_set_mask_bits(irq, 1, 1);
> -	msix_flush_writes(irq);
>  }
>  
>  void unmask_msi_irq(unsigned int irq)
>  {
>  	msi_set_mask_bits(irq, 1, 0);
> -	msix_flush_writes(irq);
>  }
>  
>  static int msi_free_irqs(struct pci_dev* dev);
> @@ -389,9 +364,8 @@ static int msi_capability_init(struct pc
>  		pci_read_config_dword(dev,
>  			msi_mask_bits_reg(pos, entry->msi_attrib.is_64),
>  			&maskbits);
> -		temp = (1 << multi_msi_capable(control));
> -		temp = ((temp - 1) & ~temp);
> -		maskbits |= temp;
> +		temp = 1U << (multi_msi_capable(control) - 1);
> +		maskbits |= (temp << 1) - 1;

Isn't this the new code the same as:
    maskbits |= (1U << multi_msi_capable(control)) - 1;

So the "& ~temp" got dropped...which looks correct to me.
"& ~temp" isn't needed given only one bit could be set in temp. 

In any case, I'm feeling a bit dense since I'm not seeing the problem.
If multi_msi_capable(control) is 5, how is the shift of a
signed int ("1") undefined?

control is u16 and temp is "unsigned int" if that matters.


>  		pci_write_config_dword(dev, entry->msi_attrib.is_64, maskbits);
>  		entry->msi_attrib.maskbits_mask = temp;
>  	}
> --- linux-2.6.28-rc3/drivers/pci/msi.h	2007-02-04 19:44:54.000000000 +0100
> +++ 2.6.28-rc3-pci-msi-misc/drivers/pci/msi.h	2008-11-07 09:13:09.000000000 +0100
> @@ -23,7 +23,7 @@
>  #define multi_msi_capable(control) \
>  	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>  #define multi_msi_enable(control, num) \
> -	control |= (((num >> 1) << 4) & PCI_MSI_FLAGS_QSIZE);
> +	control |= ((ilog2(num) << 4) & PCI_MSI_FLAGS_QSIZE)

I've no clue what the issue is here.
Can you point at a section in the PCI sec that defines this?

thanks,
grant

>  #define is_64bit_address(control)	(!!(control & PCI_MSI_FLAGS_64BIT))
>  #define is_mask_bit_support(control)	(!!(control & PCI_MSI_FLAGS_MASKBIT))
>  #define msi_enable(control, num) multi_msi_enable(control, num); \
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Matthew Wilcox]

On Sat, Nov 08, 2008 at 01:28:22AM -0700, Grant Grundler wrote:
> On Fri, Nov 07, 2008 at 08:53:49AM +0000, Jan Beulich wrote:
> > msix_flush_writes() is being called exclusively after calling msi_set_mask_bit(),
> > and that function already does follow writel() by readl() in the MSI-X case.
> 
> Yes - this part is correct.
> If you care to split this into seperate parts, please add:
>    Reviewed-by: Grant Grundler <grundler@parisc-linux.org>
> 
> for this chunk.

It's certainly true (I'd noticed it too).  What I hadn't decided was
whether to take out the readl() or take out the msix_flush_writes().
I'm conflicted.

> > Also, isn't the single use of multi_msi_capable() broken (in the event that
> > the Multiple Message Capable field was 5, the shift would be undefined,
> > on x86 in particular would yield 1 as the result, where 0 would be needed),
> > and the subsequent twiddling of temp needlessly complicated (subtracting
> > one should be sufficient here).
> 
> Comment on this chunk below.
> 
> > And isn't multi_msi_enable(), though unused (since msi_{en,dis}able() are
> > unused), broken altogether (shifting num right by 1 instead of taking the
> > binary log)?

Yes, it is, but I delete it in my patch series that enables support for
multiple MSI.

> > @@ -389,9 +364,8 @@ static int msi_capability_init(struct pc
> >  		pci_read_config_dword(dev,
> >  			msi_mask_bits_reg(pos, entry->msi_attrib.is_64),
> >  			&maskbits);
> > -		temp = (1 << multi_msi_capable(control));
> > -		temp = ((temp - 1) & ~temp);
> > -		maskbits |= temp;
> > +		temp = 1U << (multi_msi_capable(control) - 1);
> > +		maskbits |= (temp << 1) - 1;
> 
> Isn't this the new code the same as:
>     maskbits |= (1U << multi_msi_capable(control)) - 1;

You're missing the << 32 possibility.  This all gets a bit confusing
since the number of bits has to be a power of two.  So what we're
actually looking at doing is expanding a number between 0 and 5
(inclusive) to a bitmask.  I gave up and used a lookup table ... can't
find that patch right now, but it was something like:

unsigned int msi_mask[] = {
	1, 3, 7, 15, 0xff, 0xffff, 0xffffffff
};

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Grant Grundler]

On Sat, Nov 08, 2008 at 07:10:22AM -0700, Matthew Wilcox wrote:
> It's certainly true (I'd noticed it too).  What I hadn't decided was
> whether to take out the readl() or take out the msix_flush_writes().
> I'm conflicted.

Good, I'm not. :)
I'd much rather remove msix_flush_writes(). It's clearer to me to have the
readl() next to the writel() than to bury the readl() a function call later.

...
> > > +		temp = 1U << (multi_msi_capable(control) - 1);
> > > +		maskbits |= (temp << 1) - 1;
> > 
> > Isn't this the new code the same as:
> >     maskbits |= (1U << multi_msi_capable(control)) - 1;
> 
> You're missing the << 32 possibility.

Thanks - You are right. I expected (1U<<32) to be zero (arithmetic shift left)
instead of 1 ("Rotate Left" aka "logical shift left"). Code below produced:

     5 32 0x1 0x0

when using "1U << k". Here's the test code:

grundler@mb500:~$ cat test_shift.c
#include <stdio.h>
main()
{
	unsigned int i;
	for (i=0;i<6; i++) {
		unsigned int k = 1U << i ;
		unsigned int j = 1ULL << k ;
		printf("%d %d 0x%x 0x%x\n", i, k, j, j - 1);
	}
        return 0;
}

grundler@mb500:~$ ./test_shift
0 1 0x2 0x1
1 2 0x4 0x3
2 4 0x10 0xf
3 8 0x100 0xff
4 16 0x10000 0xffff
5 32 0x0 0xffffffff

Using "1ULL" (64-bit constant) is enough to get the result I was
expecting.... note I didn't declare j as "unsigned long long j"
because gcc didn't warn about assigning "1ULL < k" (64-bit) to an
unsigned int (32-bit). gcc bug?

I think the kernel should declare maskbits as 64-bit and
use "1ULL" to just keep things obvious.

>  This all gets a bit confusing
> since the number of bits has to be a power of two.  So what we're
> actually looking at doing is expanding a number between 0 and 5
> (inclusive) to a bitmask.  I gave up and used a lookup table ... can't
> find that patch right now, but it was something like:
> 
> unsigned int msi_mask[] = {
> 	1, 3, 7, 15, 0xff, 0xffff, 0xffffffff
> };

The code, when written correctly, seems compact and simple enough.

thanks,
grant

Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Matthew Wilcox]

On Sat, Nov 08, 2008 at 11:37:45AM -0700, Grant Grundler wrote:
> On Sat, Nov 08, 2008 at 07:10:22AM -0700, Matthew Wilcox wrote:
> > It's certainly true (I'd noticed it too).  What I hadn't decided was
> > whether to take out the readl() or take out the msix_flush_writes().
> > I'm conflicted.
> 
> Good, I'm not. :)
> I'd much rather remove msix_flush_writes(). It's clearer to me to have the
> readl() next to the writel() than to bury the readl() a function call later.

That's certainly a respectable opinion.  It's one I hold myself.  Have
you considered another opinion that if you're trying to disable a
thousand or more interrupts, doing writel/readl() a thousand times is
going to be less efficient than doing writel() a thousand times and
readl() once?  We don't really have a need to do that today, but it's a
possible direction we might want to move in the future.

> Thanks - You are right. I expected (1U<<32) to be zero (arithmetic shift left)
> instead of 1 ("Rotate Left" aka "logical shift left"). Code below produced:

Er, rotate left is not logical shift left.  Also, shifting by >= the number of
bits in the element is undefined by the C spec.  Which is why Jan broke
it into two shifts.

>      5 32 0x1 0x0
> 
> when using "1U << k". Here's the test code:
> 
> grundler@mb500:~$ cat test_shift.c
> #include <stdio.h>
> main()
> {
> 	unsigned int i;
> 	for (i=0;i<6; i++) {
> 		unsigned int k = 1U << i ;
> 		unsigned int j = 1ULL << k ;
> 		printf("%d %d 0x%x 0x%x\n", i, k, j, j - 1);
> 	}
>         return 0;
> }

This will give different results on different processors, and possibly
even with different compilers.  It is entitled to emit nasal daemons.

> Using "1ULL" (64-bit constant) is enough to get the result I was
> expecting.... note I didn't declare j as "unsigned long long j"
> because gcc didn't warn about assigning "1ULL < k" (64-bit) to an
> unsigned int (32-bit). gcc bug?

Not a bug.  That kind of truncation isn't generally warned about.
Consider:

void foo(char *x, int bar)
{
	x[0] = bar >> 24;
	x[1] = bar >> 16;
	x[2] = bar >> 8;
	x[3] = bar;
}

> I think the kernel should declare maskbits as 64-bit and
> use "1ULL" to just keep things obvious.

I suspec that's rather inefficient.

> > unsigned int msi_mask[] = {
> > 	1, 3, 7, 15, 0xff, 0xffff, 0xffffffff
> > };
> 
> The code, when written correctly, seems compact and simple enough.

I'm not sure I agree.

This is all rather moot since multiple MSI isn't supported, and my
patchset deletes the function in question anyway.

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Grant Grundler]

On Sat, Nov 08, 2008 at 07:13:23PM -0700, Matthew Wilcox wrote:
> On Sat, Nov 08, 2008 at 11:37:45AM -0700, Grant Grundler wrote:
> > On Sat, Nov 08, 2008 at 07:10:22AM -0700, Matthew Wilcox wrote:
> > > It's certainly true (I'd noticed it too).  What I hadn't decided was
> > > whether to take out the readl() or take out the msix_flush_writes().
> > > I'm conflicted.
> > 
> > Good, I'm not. :)
> > I'd much rather remove msix_flush_writes(). It's clearer to me to have the
> > readl() next to the writel() than to bury the readl() a function call later.
> 
> That's certainly a respectable opinion.  It's one I hold myself.  Have
> you considered another opinion that if you're trying to disable a
> thousand or more interrupts, doing writel/readl() a thousand times is
> going to be less efficient than doing writel() a thousand times and
> readl() once?  We don't really have a need to do that today, but it's a
> possible direction we might want to move in the future.

I hadn't considered this case and agree that moving the MMIO readl
"out of the loop" would make it more efficient. But I expect we can
implement that when it becomes a problem.

BTW, I worry the solution won't be as simple as it sounds since
pushing a thousand MMIO writes to a chip will result in the MMIO writes
backing up all the write queues. ie they probably shouldn't be done
while in the interrupt context anyway.

Secondly, if these writes have to go to a thousand different devices,
we would have to flush each one anyway. If we have 2K MSI for one
device and update them all at once, it's going to be a significant
hiccup no matter how we do it. I certainly hope this is not going
to be part of anyone's performance code path (e.g. RX or TX).


> > Thanks - You are right. I expected (1U<<32) to be zero (arithmetic shift left)
> > instead of 1 ("Rotate Left" aka "logical shift left"). Code below produced:
> 
> Er, rotate left is not logical shift left.

Sorry - brainfart. "logical shift left" is the same as "arithmetic shift left".

>  Also, shifting by >= the number of
> bits in the element is undefined by the C spec.  Which is why Jan broke
> it into two shifts.

Ok.

...
> Not a bug.  That kind of truncation isn't generally warned about.
> Consider:
> 
> void foo(char *x, int bar)
> {
> 	x[0] = bar >> 24;
> 	x[1] = bar >> 16;
> 	x[2] = bar >> 8;
> 	x[3] = bar;
> }

*nod* - the shift is going in the opposite direction in this example.
But I'll agree warning on this would break quite a few compiles.

> > I think the kernel should declare maskbits as 64-bit and
> > use "1ULL" to just keep things obvious.
> 
> I suspect that's rather inefficient.

It's slightly less efficient. But it's alot clearer and I don't
have the impression this is performance sensitive code (not like
TLB miss handlers, syscall entry, or DMA mapping code).


> > > unsigned int msi_mask[] = {
> > > 	1, 3, 7, 15, 0xff, 0xffff, 0xffffffff
> > > };
> > 
> > The code, when written correctly, seems compact and simple enough.
> 
> I'm not sure I agree.
> 
> This is all rather moot since multiple MSI isn't supported, and my
> patchset deletes the function in question anyway.

ok

thanks,
grant

Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 987 bytes --]

On Fri, 2008-11-07 at 08:53 +0000, Jan Beulich wrote:
> msix_flush_writes() is being called exclusively after calling msi_set_mask_bit(),
> and that function already does follow writel() by readl() in the MSI-X case.

Which makes me wonder why the initial patch was necessary?

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=988cbb15e00e6f924d052874b40c6a5447f9fdd7
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/pci/msi.c;h=a4ef93ea4c547b78bf80b5b12c9c20101bd3a1ec;hb=988cbb15e00e6f924d052874b40c6a5447f9fdd7

AFAICS there was already a readl() in msi_set_mask_bit(), so either the
initial patch didn't do anything useful or we're missing the point.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Grant Grundler]

On Mon, Nov 10, 2008 at 10:07:50AM +1100, Michael Ellerman wrote:
> On Fri, 2008-11-07 at 08:53 +0000, Jan Beulich wrote:
> > msix_flush_writes() is being called exclusively after calling msi_set_mask_bit(),
> > and that function already does follow writel() by readl() in the MSI-X case.
> 
> Which makes me wonder why the initial patch was necessary?

"This patch performs a read flush after writes to the MSI-X table for
mask and unmask operations. "

The msi_set_mask_bit() and msi_set_enable() only use config space writes.
Are config space writes postable?

The original patch seems to suggest that.
I thought only MMIO space writes are postable and this chunk of PCI 2.3 spec
suggests that as well:

3.3.3.3.1. Basic Operation of a Delayed Transaction
    All bus commands that must complete on the destination bus before
    completing on the originating bus may be completed as a Delayed
    Transaction. These include Interrupt Acknowledge, I/O Read, I/O Write,
    Configuration Read, Configuration Write, Memory Read, Memory Read Line,
    and Memory Read Multiple commands. Memory Write and Memory Write and
    Invalidate commands can complete on the originating bus before completing
    on the destination bus (i.e., can be posted).


> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=988cbb15e00e6f924d052874b40c6a5447f9fdd7
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/pci/msi.c;h=a4ef93ea4c547b78bf80b5b12c9c20101bd3a1ec;hb=988cbb15e00e6f924d052874b40c6a5447f9fdd7
> 
> AFAICS there was already a readl() in msi_set_mask_bit(), so either the
> initial patch didn't do anything useful or we're missing the point.

My impression was the former.

It's possible the arch this was discovered on implements postable
config space writes.  I'm looking at drivers/parisc/dino.c and it
looks to me like it's using posted writes to generate config space
writes (see dino_cfg_write()). lba_pci.c in the same directory seems
to enforce the semantics correctly (see elroy_cfg_write()).
*Cough* so where did I put the brown paper bag last time...

thanks,
grant

> 
> cheers
> 
> -- 
> Michael Ellerman
> OzLabs, IBM Australia Development Lab
> 
> wwweb: http://michael.ellerman.id.au
> phone: +61 2 6212 1183 (tie line 70 21183)
> 
> We do not inherit the earth from our ancestors,
> we borrow it from our children. - S.M.A.R.T Person

Re: Is msix_flush_writes() really needed? And multi_msi_*() flawed?

[Jan Beulich]

>>> Grant Grundler <grundler@parisc-linux.org> 08.11.08 09:28 >>>
>On Fri, Nov 07, 2008 at 08:53:49AM +0000, Jan Beulich wrote:
>> @@ -389,9 +364,8 @@ static int msi_capability_init(struct pc
>>  		pci_read_config_dword(dev,
>>  			msi_mask_bits_reg(pos, entry->msi_attrib.is_64),
>>  			&maskbits);
>> -		temp = (1 << multi_msi_capable(control));
>> -		temp = ((temp - 1) & ~temp);
>> -		maskbits |= temp;
>> +		temp = 1U << (multi_msi_capable(control) - 1);
>> +		maskbits |= (temp << 1) - 1;
>
>Isn't this the new code the same as:
>    maskbits |= (1U << multi_msi_capable(control)) - 1;

No.

>So the "& ~temp" got dropped...which looks correct to me.
>"& ~temp" isn't needed given only one bit could be set in temp. 
>
>In any case, I'm feeling a bit dense since I'm not seeing the problem.
>If multi_msi_capable(control) is 5, how is the shift of a
>signed int ("1") undefined?

multi_msi_capable() already shifts 1 by the value read out of the control
word, so if the bit field read is 5, we'd shift 1 left by 32 here.

>> @@ -23,7 +23,7 @@
>>  #define multi_msi_capable(control) \
>>  	(1 << ((control & PCI_MSI_FLAGS_QMASK) >> 1))
>>  #define multi_msi_enable(control, num) \
>> -	control |= (((num >> 1) << 4) & PCI_MSI_FLAGS_QSIZE);
>> +	control |= ((ilog2(num) << 4) & PCI_MSI_FLAGS_QSIZE)
>
>I've no clue what the issue is here.
>Can you point at a section in the PCI sec that defines this?

What this apparently tries to do is set the number of vectors the device
is allowed to actually use (i.e. the field named 'Multiple Message Enable' in
section 6.8.1.3 of the 3.0 copy I'm looking at). Actually, as I now realize
this probably rather ought to be ilog2(num - 1) + 1, since the spec
requires the requested value to be rounded up, not down.

Jan