Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion

[Alex Williamson]

On Thu, 2012-06-14 at 18:45 +0300, Michael S. Tsirkin wrote:
> On Thu, Jun 14, 2012 at 09:09:47AM -0600, Alex Williamson wrote:
> > On Thu, 2012-06-14 at 17:50 +0300, Michael S. Tsirkin wrote:
> > > On Thu, Jun 14, 2012 at 08:21:39AM -0600, Alex Williamson wrote:
> > > > On Thu, 2012-06-14 at 13:24 +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 13, 2012 at 10:51:47PM -0600, Alex Williamson wrote:
> > > > > > These don't have to be contiguous.  Size them to only what
> > > > > > they need and use separate MemoryRegions for the vector
> > > > > > table and PBA.
> > > > > > 
> > > > > > Signed-off-by: Alex Williamson <address@hidden>
> > > > > 
> > > > > Why is this still using NATIVE?
> > > > 
> > > > Because the bug already exists,
> > > 
> > > We have lots of broken code. The way progress happens here is
> > > such code is in a kind of freeze until fixed. This way whoever needs new
> > > features gets to fix the bugs too.
> > 
> > In other words, you impose a toll and inhibit forward progress until
> > someone fixes it?  I have no place telling you how to be a maintainer,
> > but I personally find that this style makes attempting to contribute
> > code to anything pci/msi/msix related a huge pain.  There are far too
> > many of these land mines in the code and simple fixes easily explode
> > into tangentially related changes off your todo list.
> 
> I try to pick simple fixes up straight away. Pls try to keep the fixes
> simpler :)

What does that have to do with shoving todo list items down the throats
of contributors?

> > > > this patch doesn't make it worse, so at best it's a tangentially 
> > > > related additional fix.
> > > > It may seem like a s/NATIVE/LITTLE/ to you, but to me it's asking to 
> > > > completely scrub
> > > > msix.c for endian correctness.  Is this going to be the carrot you hold
> > > > out to accept the rest of the series?
> > > > 
> > > > Alex
> > > 
> > > Unfortunately no promises yet, and that is because you basically decided
> > > to rewrite lots of code in your preferred style while also adding new 
> > > functionality.
> > > If changes were done in small steps, then I could apply things we can
> > > agree on and defer the ones we don't.  Sometimes it's hard, but clearly
> > > not in this case.
> > 
> > Patches can always be reduced into smaller changes, but at some point we
> > have to call it good enough.  I split one patch into 6 and thought that
> > did a pretty good job.
> 
> It's not the mechanical splitting of patches that is needed.
> In one case you actually added a new function in place X then moved it
> to place Y. And the new order does not make sense: init then uninit looks 
> cleaner.

uninit was moved because I was able to remove duplicate code by making
init call uninit on error.  Do you prefer a prototype to avoid code
moves in that case?  Doesn't matter now, it's fixed with Jan's
suggestion and I've already split the move of another tiny function to a
separate patch.

> > Should I remove everywhere that I've added a new
> > line to avoid imposing my style on the rest of the code?
> 
> Each new line? No, that would be taking it to extreme because newlines are
> easy to ignore normally. Though if someone sends me a patch with 1000
> newlines tweaked and functional changes in the same patch, I won't apply
> it.

Well then, I'm not sure what you mean by "you basically decided to
rewrite lots of code in your preferred style".

> > The next
> > version will eliminate the add_config move thanks to Jan's constructive
> > suggestion, so I hope it meets your standards.  Thanks,
> > 
> > Alex
> 
> Please try to address other comments too, like naming
> constants. I would hate to get another revision that just ignores them.

It will unless you counter my rebuttal to why I'm not using macros
there.  To repeat:

On Wed, 2012-06-13 at 17:05 -0600, Alex Williamson wrote:
On Wed, 2012-06-13 at 23:43 +0300, Michael S. Tsirkin wrote:
> > On Tue, Jun 12, 2012 at 02:03:26PM -0600, Alex Williamson wrote:
> > > +    /*
> > > +     * Migration compatibility dictates that this remains a 4k
> > > +     * BAR with the vector table in the lower half and PBA in
> > > +     * the upper half.
> > > +     */
> > > +    if (nentries * PCI_MSIX_ENTRY_SIZE > 2048) {
> > > +        return -EINVAL;
> > > +    }
> > > +
> > > +    memory_region_init(bar, name, 4096);
> > > +
> > > +    ret = msix_init(pdev, nentries, bar, bar_nr, 0, bar, bar_nr, 2048, 
> > > 0);
> > 
> > Lots of constants.
> > Current code uses macros for these, e.g.
> > MSIX_PAGE_PENDING, MSIX_PAGE_PENDING /2.
> > 
> > Let's keep it that way.
> 
> There is absolutely no valid use for them outside of this function.  I
> explain the size in the comment immediately above where they're used.
> Macro-izing these just risks someone assuming there's a standard or
> misusing it for something else (see device assignment imposing a 4k
> MSI-X table for example...)