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

[Michael S. Tsirkin]

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

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


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

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

-- 
MST