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

[Michael S. Tsirkin]

On Thu, Jun 14, 2012 at 10:02:58AM -0600, Alex Williamson wrote:
> 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?

If you write new code you do not get to use legacy interfaces.
But - if you fix bugs you are not required to fix all the bugs in one go.
If you mix bugfixes and features all is treated as new code.

> > > > > 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?

msix.h has a prototype already I think.

>  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.

This does not matter. What matters is making things easy to review.
If you send me a patch moving functions around, I can put them
side by side and compare + and -. If you make a
small cosmetic change I can see it is equivalent.
If you add functionality I see how it works.

But if you mix these types of change it's very hard to review.

> > > 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 diff was very large, is what I mean.

> > > 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.


They still appear multiple times. And 2048 is middle of page
but PAGE/2 is clearer.

>  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...)


A valid concern, but won't help against people copying code :)
Since you now use it from the exclusive call only, rename it
MSIX_EXLUSIVE_BAR_SIZE, MSIX_EXLUSIVE_BAR_PENDING?
It's actually what it is.

-- 
MST