[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegi
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion |
Date: |
Thu, 14 Jun 2012 10:02:58 -0600 |
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...)
- Re: [Qemu-devel] [PATCH v2 2/6] ivshmem: Convert to msix_init_exclusive_bar() interface, (continued)
- [Qemu-devel] [PATCH v2 3/6] virtio: Convert to msix_init_exclusive_bar() interface, Alex Williamson, 2012/06/14
- [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion, Alex Williamson, 2012/06/14
- Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion, Jan Kiszka, 2012/06/14
- Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion, Michael S. Tsirkin, 2012/06/14
- Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion, Alex Williamson, 2012/06/14
- Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion, Michael S. Tsirkin, 2012/06/14
- Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion, Alex Williamson, 2012/06/14
- Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion, Michael S. Tsirkin, 2012/06/14
- Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion,
Alex Williamson <=
- Re: [Qemu-devel] [PATCH v2 4/6] msix: Split PBA into it's own MemoryRegion, Michael S. Tsirkin, 2012/06/14
[Qemu-devel] [PATCH v2 5/6] msix: Allow full specification of MSIX layout, Alex Williamson, 2012/06/14
[Qemu-devel] [PATCH v2 6/6] msix: Fix last PCIDevice naming inconsitency, Alex Williamson, 2012/06/14