| Message ID | 1378191917-6964-1-git-send-email-lig.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
Il 03/09/2013 09:05, liguang ha scritto: > Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> > --- > cputlb.c | 15 --------------- > 1 files changed, 0 insertions(+), 15 deletions(-) > > diff --git a/cputlb.c b/cputlb.c > index 977c0ca..08e50e0 100644 > --- a/cputlb.c > +++ b/cputlb.c > @@ -169,21 +169,6 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr) > return ram_addr; > } > > -static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry) > -{ > - ram_addr_t ram_addr; > - void *p; > - > - if (tlb_is_dirty_ram(tlb_entry)) { > - p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK) > - + tlb_entry->addend); > - ram_addr = qemu_ram_addr_from_host_nofail(p); > - if (!cpu_physical_memory_is_dirty(ram_addr)) { > - tlb_entry->addr_write |= TLB_NOTDIRTY; > - } > - } > -} > - > void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length) > { > CPUState *cpu; > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> and CCing qemu-trivial. Paolo
Am 03.09.2013 09:22, schrieb Paolo Bonzini: > Il 03/09/2013 09:05, liguang ha scritto: >> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> >> --- >> cputlb.c | 15 --------------- >> 1 files changed, 0 insertions(+), 15 deletions(-) >> >> diff --git a/cputlb.c b/cputlb.c >> index 977c0ca..08e50e0 100644 >> --- a/cputlb.c >> +++ b/cputlb.c >> @@ -169,21 +169,6 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr) >> return ram_addr; >> } >> >> -static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry) >> -{ >> - ram_addr_t ram_addr; >> - void *p; >> - >> - if (tlb_is_dirty_ram(tlb_entry)) { >> - p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK) >> - + tlb_entry->addend); >> - ram_addr = qemu_ram_addr_from_host_nofail(p); >> - if (!cpu_physical_memory_is_dirty(ram_addr)) { >> - tlb_entry->addr_write |= TLB_NOTDIRTY; >> - } >> - } >> -} >> - >> void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length) >> { >> CPUState *cpu; >> > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > and CCing qemu-trivial. Negative, please keep qemu-trivial out of this. My qom-cpu pull was already blocked by the s390 and ppc pulls, so let's not add yet another potentially interfering one to the mix. IF rth agrees as TCG maintainer that this is not needed in any of his upcoming refactorings then I'll queue it on qom-cpu. My upcoming qom-cpu-13 series touches upon pretty much every core CPU file perceivable, including this cputlb.c. I also don't understand why qemu-trivial is suddenly picking up Stefan's arm translation patch, it used to be for unmaintained areas only. But arm is not my problem. Thanks, Andreas
Il 03/09/2013 10:35, Andreas Färber ha scritto: > Am 03.09.2013 09:22, schrieb Paolo Bonzini: >> Il 03/09/2013 09:05, liguang ha scritto: >>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> >>> --- >>> cputlb.c | 15 --------------- >>> 1 files changed, 0 insertions(+), 15 deletions(-) >>> >>> diff --git a/cputlb.c b/cputlb.c >>> index 977c0ca..08e50e0 100644 >>> --- a/cputlb.c >>> +++ b/cputlb.c >>> @@ -169,21 +169,6 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr) >>> return ram_addr; >>> } >>> >>> -static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry) >>> -{ >>> - ram_addr_t ram_addr; >>> - void *p; >>> - >>> - if (tlb_is_dirty_ram(tlb_entry)) { >>> - p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK) >>> - + tlb_entry->addend); >>> - ram_addr = qemu_ram_addr_from_host_nofail(p); >>> - if (!cpu_physical_memory_is_dirty(ram_addr)) { >>> - tlb_entry->addr_write |= TLB_NOTDIRTY; >>> - } >>> - } >>> -} >>> - >>> void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length) >>> { >>> CPUState *cpu; >>> >> >> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> >> >> and CCing qemu-trivial. > > Negative, please keep qemu-trivial out of this. My qom-cpu pull was > already blocked by the s390 and ppc pulls, so let's not add yet another > potentially interfering one to the mix. > > IF rth agrees as TCG maintainer that this is not needed in any of his > upcoming refactorings then I'll queue it on qom-cpu. My upcoming > qom-cpu-13 series touches upon pretty much every core CPU file > perceivable, including this cputlb.c. Sure. > I also don't understand why qemu-trivial is suddenly picking up Stefan's > arm translation patch, it used to be for unmaintained areas only. But > arm is not my problem. That patch is also not "trivial", too. Paolo
On 3 September 2013 09:35, Andreas Färber <afaerber@suse.de> wrote: > I also don't understand why qemu-trivial is suddenly picking up Stefan's > arm translation patch, it used to be for unmaintained areas only. But > arm is not my problem. Yeah, I wasn't expecting that either. But I'd reviewed it and it wasn't a big change that was likely to conflict with anything else in my queue, so I didn't feel like making a fuss about it. -- PMM
03.09.2013 12:35, Andreas Färber wrote: > I also don't understand why qemu-trivial is suddenly picking up Stefan's > arm translation patch, it used to be for unmaintained areas only. But > arm is not my problem. Which patch you're talking about? Is it "target-arm: Report unimplemented opcodes (LOG_UNIMP)" ? If yes, that one appears to be trivial as it just adds some logging before failing an instruction and should not conflict with other work being done in this area. Perhaps I was too aggressive while picking up the backlog. We should just draw the line *somewhere*, -- eg, it sure is possible to reject spelling fixes for maintained areas from -trivial (like this arm tree), - will this be productive? This change (cputlb: remove dead function) appears to be "trivial enough" for me (after looking at the usage history of this function), and I'd pick it up without this Andreas's request, too. As for the "suddenly" - it's not really suddenly, it's because it has been Cc'd to -trivial (by someone who submitted lots of good trivial patches before) and actually looks trivial, too. And also because subsystem maintainer added his Reviewed-by, apparently (or hopefully) after noticing it's submitted to -trivial. I also Cc'd both maintainers in my notice that it's been applied to -trivial. Speaking of linux headers sync, I did that once indeed, but don't think it was a good idea. It is "trivial" in a sense that it just makes headers in qemu to be the same as in current kernel (this is easy to verify), and the tree - at least in some configuration - compiles. But indeed, the side effects might be quite a bit unexpected and "non-trivial" - in other words, it is a "trivial change with non-trivial possible consequences". HTH. /mjt
Am 03.09.2013 13:17, schrieb Michael Tokarev: > 03.09.2013 12:35, Andreas Färber wrote: >> I also don't understand why qemu-trivial is suddenly picking up Stefan's >> arm translation patch, it used to be for unmaintained areas only. But >> arm is not my problem. > > Which patch you're talking about? Is it "target-arm: Report unimplemented > opcodes (LOG_UNIMP)" ? Yes. > If yes, that one appears to be trivial as it just > adds some logging before failing an instruction and should not conflict > with other work being done in this area. Perhaps I was too aggressive > while picking up the backlog. We should just draw the line *somewhere*, -- Right, that line is what I'm reminding about here. I feel that lately an increasing number of contributors and reviewers are deferring patches to qemu-trivial that don't really belong there IMO. That Anthony doesn't scale to cover Blue's maintainer work as well shouldn't lead to a surge on qemu-trivial. > eg, it sure is possible to reject spelling fixes for maintained areas > from -trivial (like this arm tree), - will this be productive? No, spelling fixes are not a concern to me as they are rather unlikely to cause conflicts with patches being queued by submaintainers. :) > This change (cputlb: remove dead function) appears to be "trivial enough" > for me (after looking at the usage history of this function), and I'd > pick it up without this Andreas's request, too. Yes. This one here would've been okay usually, as there is no official maintainer for cputlb.c and it's trivial in the sense that a git-grep confirms it to be okay. I was just annoyed that I had to defer my pull twice (sent it out now) because s390x added two CPU loops and then once that was merged ppc added another loop, too. My upcoming 35+ patch series qom-cpu-13 may hopefully explain the rest once you see it. > As for the "suddenly" - it's not really suddenly, it's because it > has been Cc'd to -trivial (by someone who submitted lots of good > trivial patches before) and actually looks trivial, too. And also > because subsystem maintainer added his Reviewed-by, apparently (or > hopefully) after noticing it's submitted to -trivial. I also Cc'd > both maintainers in my notice that it's been applied to -trivial. "Suddenly" in the sense that the prupose of qemu-trivial used to be handling patches that would otherwise fall through the cracks. So by my understanding, e.g., "target-arm:" => !trivial, and I would've expected there to be some on-list communication between PMM and you before CC'ing someone on a "thanks, applied" after the fact. By contrast, if there's a change to configure or "Fix spelling of" etc. then you picking it up is highly appreciated. I just don't want qemu-trivial becoming the least-resistance way of getting patches into qemu.git that might otherwise get bounced/changed by submaintainers. Also, I am seeing Paolo pull in huge memory changes but now pinging the breakage fixes rather than assembling a pull to fix the fallout. ;) Similarly target-i386 TCG is not suited for qemu-trivial IMO, instead rth or someone who works on and/or reviews it (rth?) should volunteer as proper maintainer. With the larger part of the community using KVM these days, we simply can't have that be handled by the community at large any more. So yes, I know you were on vacation and you seem eager to take up work again, that's great; I'm just cautioning that CC'ing everything on qemu-trivial (not your fault, you're on the receiving end) can't be the new solution, so feel encouraged to push back a little. :) Cheers, Andreas
在 2013-09-03二的 18:54 +0200，Andreas Färber写道： > Am 03.09.2013 13:17, schrieb Michael Tokarev: > > 03.09.2013 12:35, Andreas Färber wrote: > >> I also don't understand why qemu-trivial is suddenly picking up Stefan's > >> arm translation patch, it used to be for unmaintained areas only. But > >> arm is not my problem. > > > > Which patch you're talking about? Is it "target-arm: Report unimplemented > > opcodes (LOG_UNIMP)" ? > > Yes. > > > If yes, that one appears to be trivial as it just > > adds some logging before failing an instruction and should not conflict > > with other work being done in this area. Perhaps I was too aggressive > > while picking up the backlog. We should just draw the line *somewhere*, -- > > Right, that line is what I'm reminding about here. I feel that lately an > increasing number of contributors and reviewers are deferring patches to > qemu-trivial that don't really belong there IMO. That Anthony doesn't > scale to cover Blue's maintainer work as well shouldn't lead to a surge > on qemu-trivial. > > > eg, it sure is possible to reject spelling fixes for maintained areas > > from -trivial (like this arm tree), - will this be productive? > > No, spelling fixes are not a concern to me as they are rather unlikely > to cause conflicts with patches being queued by submaintainers. :) > > > This change (cputlb: remove dead function) appears to be "trivial enough" > > for me (after looking at the usage history of this function), and I'd > > pick it up without this Andreas's request, too. > > Yes. This one here would've been okay usually, as there is no official > maintainer for cputlb.c and it's trivial in the sense that a git-grep > confirms it to be okay. I was just annoyed that I had to defer my pull > twice (sent it out now) because s390x added two CPU loops and then once > that was merged ppc added another loop, too. My upcoming 35+ patch > series qom-cpu-13 may hopefully explain the rest once you see it. > > > As for the "suddenly" - it's not really suddenly, it's because it > > has been Cc'd to -trivial (by someone who submitted lots of good > > trivial patches before) and actually looks trivial, too. And also > > because subsystem maintainer added his Reviewed-by, apparently (or > > hopefully) after noticing it's submitted to -trivial. I also Cc'd > > both maintainers in my notice that it's been applied to -trivial. > > "Suddenly" in the sense that the prupose of qemu-trivial used to be > handling patches that would otherwise fall through the cracks. > > So by my understanding, e.g., "target-arm:" => !trivial, and I would've > expected there to be some on-list communication between PMM and you > before CC'ing someone on a "thanks, applied" after the fact. > By contrast, if there's a change to configure or "Fix spelling of" etc. > then you picking it up is highly appreciated. I just don't want > qemu-trivial becoming the least-resistance way of getting patches into > qemu.git that might otherwise get bounced/changed by submaintainers. > > Also, I am seeing Paolo pull in huge memory changes but now pinging the > breakage fixes rather than assembling a pull to fix the fallout. ;) > > Similarly target-i386 TCG is not suited for qemu-trivial IMO, instead > rth or someone who works on and/or reviews it (rth?) should volunteer as > proper maintainer. I'd like to maintain cputlb.c, can I? > With the larger part of the community using KVM these > days, we simply can't have that be handled by the community at large any > more. > > So yes, I know you were on vacation and you seem eager to take up work > again, that's great; I'm just cautioning that CC'ing everything on > qemu-trivial (not your fault, you're on the receiving end) can't be the > new solution, so feel encouraged to push back a little. :) > > Cheers, > Andreas >
Il 03/09/2013 18:54, Andreas Färber ha scritto: > Also, I am seeing Paolo pull in huge memory changes but now pinging the > breakage fixes rather than assembling a pull to fix the fallout. ;) Well, I didn't put myself as memory maintainer for a reason. :) But it looks like a pull request is the best thing to do, so I'll find some time this week to make one. Paolo
diff --git a/cputlb.c b/cputlb.c index 977c0ca..08e50e0 100644 --- a/cputlb.c +++ b/cputlb.c @@ -169,21 +169,6 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr) return ram_addr; } -static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry) -{ - ram_addr_t ram_addr; - void *p; - - if (tlb_is_dirty_ram(tlb_entry)) { - p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK) - + tlb_entry->addend); - ram_addr = qemu_ram_addr_from_host_nofail(p); - if (!cpu_physical_memory_is_dirty(ram_addr)) { - tlb_entry->addr_write |= TLB_NOTDIRTY; - } - } -} - void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length) { CPUState *cpu;
Signed-off-by: liguang <lig.fnst@cn.fujitsu.com> --- cputlb.c | 15 --------------- 1 files changed, 0 insertions(+), 15 deletions(-)