Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Wed, 21 Nov 2018 15:38:16 +0100 Samuel Ortiz wrote: > Igor, > > On Wed, Nov 21, 2018 at 03:15:26PM +0100, Igor Mammedov wrote: > > On Wed, 21 Nov 2018 07:35:47 -0500 > > "Michael S. Tsirkin" wrote: > > > > > On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote: > > > > On Fri, 16 Nov 2018 17:37:54 +0100 > > > > Paolo Bonzini wrote: > > > > > > > > > On 16/11/18 17:29, Igor Mammedov wrote: > > > > > > General suggestions for this series: > > > > > > 1. Preferably don't do multiple changes within a patch > > > > > > neither post huge patches (unless it's pure code movement). > > > > > > (it's easy to squash patches later it necessary) > > > > > > 2. Start small, pick a table generalize it and send as > > > > > > one small patchset. Tables are often independent > > > > > > and it's much easier on both author/reviewer to agree upon > > > > > > changes and rewrite it if necessary. > > > > > > > > > > How would that be done? This series is on the bigger side, agreed, > > > > > but > > > > > most of it is really just code movement. It's a starting point, > > > > > having > > > > > a generic ACPI library is way beyond what this is trying to do. > > > > I've tried to give suggestions how to restructure series > > > > on per patch basis. In my opinion it quite possible to split > > > > series in several smaller ones and it should really help with > > > > making series cleaner and easier/faster to review/amend/merge > > > > vs what we have in v5. > > > > (it's more frustrating to rework large series vs smaller one) > > > > > > > > If something isn't clear, it's easy to reach out to me here > > > > or directly (email/irc/github) for clarification/feed back. > > > > > > I assume the #1 goal is to add reduced HW support. So another > > > option to speed up merging is to just go ahead and duplicate a > > > bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other > > > file. > > > This way it might be easier to see what's common code and what isn't. > > > And I think offline Igor said he might prefer that way. Right Igor? > > You mean probably 'x86 reduced hw' support. That's was what I've > > already suggested for PCI AML code during patch review. Just don't > > call it generic when it's not and place code in hw/i386/ directory beside > > acpi-build.c. It might apply to some other tables (i.e. complex cases). > > > > On per patch review I gave suggestions how to amend series to make > > it acceptable without doing complex refactoring and pointed out > > places we probably shouldn't refactor now and just duplicate as > > it's too complex or not clear how to generalize it yet. > > > > Problem with duplication is that a random contributor is not > > around to clean code up after a feature is merged and we end up > > with a bunch of messy code. > > > > A word to the contributors, > > Don't do refactoring in silence, keep discussing approaches here, > > suggest alternatives. That way it's easier to reach a compromise > > and merge it with less iterations. And if you do split it in smaller > > parts, the process should go even faster. > > > > I'll sent a small RSDP refactoring series for reference. > I was already working on the RSDP changes. Let me know if I should drop > that work too. Go ahead, you can reuse RSDP fixes I've just posted (you are CCed) if you haven't written them yet on your own. > Cheers, > Samuel.
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Wed, Nov 21, 2018 at 03:15:26PM +0100, Igor Mammedov wrote: > On Wed, 21 Nov 2018 07:35:47 -0500 > "Michael S. Tsirkin" wrote: > > > On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote: > > > On Fri, 16 Nov 2018 17:37:54 +0100 > > > Paolo Bonzini wrote: > > > > > > > On 16/11/18 17:29, Igor Mammedov wrote: > > > > > General suggestions for this series: > > > > > 1. Preferably don't do multiple changes within a patch > > > > > neither post huge patches (unless it's pure code movement). > > > > > (it's easy to squash patches later it necessary) > > > > > 2. Start small, pick a table generalize it and send as > > > > > one small patchset. Tables are often independent > > > > > and it's much easier on both author/reviewer to agree upon > > > > > changes and rewrite it if necessary. > > > > > > > > How would that be done? This series is on the bigger side, agreed, but > > > > most of it is really just code movement. It's a starting point, having > > > > a generic ACPI library is way beyond what this is trying to do. > > > I've tried to give suggestions how to restructure series > > > on per patch basis. In my opinion it quite possible to split > > > series in several smaller ones and it should really help with > > > making series cleaner and easier/faster to review/amend/merge > > > vs what we have in v5. > > > (it's more frustrating to rework large series vs smaller one) > > > > > > If something isn't clear, it's easy to reach out to me here > > > or directly (email/irc/github) for clarification/feed back. > > > > I assume the #1 goal is to add reduced HW support. So another > > option to speed up merging is to just go ahead and duplicate a > > bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other > > file. > > This way it might be easier to see what's common code and what isn't. > > And I think offline Igor said he might prefer that way. Right Igor? > You mean probably 'x86 reduced hw' support. That's what this is going to eventually look like, unfortunately. And there's no technical reasons why we could not have a arch agnostic hw-reduced support, so this should only be an intermediate step. > That's was what I've > already suggested for PCI AML code during patch review. Just don't > call it generic when it's not and place code in hw/i386/ directory beside > acpi-build.c. It might apply to some other tables (i.e. complex cases). > > On per patch review I gave suggestions how to amend series to make > it acceptable without doing complex refactoring and pointed out > places we probably shouldn't refactor now and just duplicate as > it's too complex or not clear how to generalize it yet. I think I got the idea, and I will try to rework this serie according to these directions. > Problem with duplication is that a random contributor is not > around to clean code up after a feature is merged and we end up > with a bunch of messy code. I'd argue that the same could be said of a potential "x86 hw-reduced" solution. The same random contributor may not be around to push it to the next step and make it more generic. I'd also argue we're not planning to be random contributors, dropping code to the mailing list and leaving. > A word to the contributors, > Don't do refactoring in silence, keep discussing approaches here, > suggest alternatives. Practically speaking, a large chunk of the NEMU work rely on having a generic hardware-reduced ACPI implementation. We could not have blocked the project waiting for an upstream acceptable solution for it and we had to pick one route. Retroactively I think we should have gone the self-contained, fully duplicated route and move on with the rest of the NEMU work. Upstream discussions could have then happened in parallel without much disruption for the project. Cheers, Samuel.
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
Igor, On Wed, Nov 21, 2018 at 03:15:26PM +0100, Igor Mammedov wrote: > On Wed, 21 Nov 2018 07:35:47 -0500 > "Michael S. Tsirkin" wrote: > > > On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote: > > > On Fri, 16 Nov 2018 17:37:54 +0100 > > > Paolo Bonzini wrote: > > > > > > > On 16/11/18 17:29, Igor Mammedov wrote: > > > > > General suggestions for this series: > > > > > 1. Preferably don't do multiple changes within a patch > > > > > neither post huge patches (unless it's pure code movement). > > > > > (it's easy to squash patches later it necessary) > > > > > 2. Start small, pick a table generalize it and send as > > > > > one small patchset. Tables are often independent > > > > > and it's much easier on both author/reviewer to agree upon > > > > > changes and rewrite it if necessary. > > > > > > > > How would that be done? This series is on the bigger side, agreed, but > > > > most of it is really just code movement. It's a starting point, having > > > > a generic ACPI library is way beyond what this is trying to do. > > > I've tried to give suggestions how to restructure series > > > on per patch basis. In my opinion it quite possible to split > > > series in several smaller ones and it should really help with > > > making series cleaner and easier/faster to review/amend/merge > > > vs what we have in v5. > > > (it's more frustrating to rework large series vs smaller one) > > > > > > If something isn't clear, it's easy to reach out to me here > > > or directly (email/irc/github) for clarification/feed back. > > > > I assume the #1 goal is to add reduced HW support. So another > > option to speed up merging is to just go ahead and duplicate a > > bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other > > file. > > This way it might be easier to see what's common code and what isn't. > > And I think offline Igor said he might prefer that way. Right Igor? > You mean probably 'x86 reduced hw' support. That's was what I've > already suggested for PCI AML code during patch review. Just don't > call it generic when it's not and place code in hw/i386/ directory beside > acpi-build.c. It might apply to some other tables (i.e. complex cases). > > On per patch review I gave suggestions how to amend series to make > it acceptable without doing complex refactoring and pointed out > places we probably shouldn't refactor now and just duplicate as > it's too complex or not clear how to generalize it yet. > > Problem with duplication is that a random contributor is not > around to clean code up after a feature is merged and we end up > with a bunch of messy code. > > A word to the contributors, > Don't do refactoring in silence, keep discussing approaches here, > suggest alternatives. That way it's easier to reach a compromise > and merge it with less iterations. And if you do split it in smaller > parts, the process should go even faster. > > I'll sent a small RSDP refactoring series for reference. I was already working on the RSDP changes. Let me know if I should drop that work too. Cheers, Samuel.
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Wed, 21 Nov 2018 07:35:47 -0500 "Michael S. Tsirkin" wrote: > On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote: > > On Fri, 16 Nov 2018 17:37:54 +0100 > > Paolo Bonzini wrote: > > > > > On 16/11/18 17:29, Igor Mammedov wrote: > > > > General suggestions for this series: > > > > 1. Preferably don't do multiple changes within a patch > > > > neither post huge patches (unless it's pure code movement). > > > > (it's easy to squash patches later it necessary) > > > > 2. Start small, pick a table generalize it and send as > > > > one small patchset. Tables are often independent > > > > and it's much easier on both author/reviewer to agree upon > > > > changes and rewrite it if necessary. > > > > > > How would that be done? This series is on the bigger side, agreed, but > > > most of it is really just code movement. It's a starting point, having > > > a generic ACPI library is way beyond what this is trying to do. > > I've tried to give suggestions how to restructure series > > on per patch basis. In my opinion it quite possible to split > > series in several smaller ones and it should really help with > > making series cleaner and easier/faster to review/amend/merge > > vs what we have in v5. > > (it's more frustrating to rework large series vs smaller one) > > > > If something isn't clear, it's easy to reach out to me here > > or directly (email/irc/github) for clarification/feed back. > > I assume the #1 goal is to add reduced HW support. So another > option to speed up merging is to just go ahead and duplicate a > bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other > file. > This way it might be easier to see what's common code and what isn't. > And I think offline Igor said he might prefer that way. Right Igor? You mean probably 'x86 reduced hw' support. That's was what I've already suggested for PCI AML code during patch review. Just don't call it generic when it's not and place code in hw/i386/ directory beside acpi-build.c. It might apply to some other tables (i.e. complex cases). On per patch review I gave suggestions how to amend series to make it acceptable without doing complex refactoring and pointed out places we probably shouldn't refactor now and just duplicate as it's too complex or not clear how to generalize it yet. Problem with duplication is that a random contributor is not around to clean code up after a feature is merged and we end up with a bunch of messy code. A word to the contributors, Don't do refactoring in silence, keep discussing approaches here, suggest alternatives. That way it's easier to reach a compromise and merge it with less iterations. And if you do split it in smaller parts, the process should go even faster. I'll sent a small RSDP refactoring series for reference. > > > Paolo > > > > > > > 3. when you think about refactoring acpi into a generic API > > > > think about it as routines that go into a separate library > > > > (pure acpi spec code) and qemu/acpi glue routines and > > > > divide them correspondingly. > > >
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Wed, Nov 21, 2018 at 02:50:30PM +0100, Samuel Ortiz wrote: > Hi Michael, > > On Wed, Nov 21, 2018 at 07:35:47AM -0500, Michael S. Tsirkin wrote: > > On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote: > > > On Fri, 16 Nov 2018 17:37:54 +0100 > > > Paolo Bonzini wrote: > > > > > > > On 16/11/18 17:29, Igor Mammedov wrote: > > > > > General suggestions for this series: > > > > > 1. Preferably don't do multiple changes within a patch > > > > > neither post huge patches (unless it's pure code movement). > > > > > (it's easy to squash patches later it necessary) > > > > > 2. Start small, pick a table generalize it and send as > > > > > one small patchset. Tables are often independent > > > > > and it's much easier on both author/reviewer to agree upon > > > > > changes and rewrite it if necessary. > > > > > > > > How would that be done? This series is on the bigger side, agreed, but > > > > most of it is really just code movement. It's a starting point, having > > > > a generic ACPI library is way beyond what this is trying to do. > > > I've tried to give suggestions how to restructure series > > > on per patch basis. In my opinion it quite possible to split > > > series in several smaller ones and it should really help with > > > making series cleaner and easier/faster to review/amend/merge > > > vs what we have in v5. > > > (it's more frustrating to rework large series vs smaller one) > > > > > > If something isn't clear, it's easy to reach out to me here > > > or directly (email/irc/github) for clarification/feed back. > > > > I assume the #1 goal is to add reduced HW support. > >From our perspective, yes. From the project's point of view, it's about > making the current ACPI code more generic and not bound to any specific > machine type. > > > So another > > option to speed up merging is to just go ahead and duplicate a > > bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other > > file. > It's precisely what we wanted to avoid in the very first place and we > assumed this would be largely frowned upon by the community. It's also a > burden for everyone to maintain that amount of duplicated code. Also I > suppose this would also mean we'd have to eventually de-duplicate and > factorize things in. For sure, that's the plan. > Honestly I'd rather not rush things out and work on code sharing first. > I'll answer Igor's numerous comments today and will start addressing > some of his concerns right aways as well. > > Cheers, > Samuel. OK, no problem then - just trying to make sure you aren't blocked. -- MST
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
Hi Michael, On Wed, Nov 21, 2018 at 07:35:47AM -0500, Michael S. Tsirkin wrote: > On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote: > > On Fri, 16 Nov 2018 17:37:54 +0100 > > Paolo Bonzini wrote: > > > > > On 16/11/18 17:29, Igor Mammedov wrote: > > > > General suggestions for this series: > > > > 1. Preferably don't do multiple changes within a patch > > > > neither post huge patches (unless it's pure code movement). > > > > (it's easy to squash patches later it necessary) > > > > 2. Start small, pick a table generalize it and send as > > > > one small patchset. Tables are often independent > > > > and it's much easier on both author/reviewer to agree upon > > > > changes and rewrite it if necessary. > > > > > > How would that be done? This series is on the bigger side, agreed, but > > > most of it is really just code movement. It's a starting point, having > > > a generic ACPI library is way beyond what this is trying to do. > > I've tried to give suggestions how to restructure series > > on per patch basis. In my opinion it quite possible to split > > series in several smaller ones and it should really help with > > making series cleaner and easier/faster to review/amend/merge > > vs what we have in v5. > > (it's more frustrating to rework large series vs smaller one) > > > > If something isn't clear, it's easy to reach out to me here > > or directly (email/irc/github) for clarification/feed back. > > I assume the #1 goal is to add reduced HW support. >From our perspective, yes. From the project's point of view, it's about making the current ACPI code more generic and not bound to any specific machine type. > So another > option to speed up merging is to just go ahead and duplicate a > bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other > file. It's precisely what we wanted to avoid in the very first place and we assumed this would be largely frowned upon by the community. It's also a burden for everyone to maintain that amount of duplicated code. Also I suppose this would also mean we'd have to eventually de-duplicate and factorize things in. Honestly I'd rather not rush things out and work on code sharing first. I'll answer Igor's numerous comments today and will start addressing some of his concerns right aways as well. Cheers, Samuel.
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Mon, Nov 19, 2018 at 04:31:10PM +0100, Igor Mammedov wrote: > On Fri, 16 Nov 2018 17:37:54 +0100 > Paolo Bonzini wrote: > > > On 16/11/18 17:29, Igor Mammedov wrote: > > > General suggestions for this series: > > > 1. Preferably don't do multiple changes within a patch > > > neither post huge patches (unless it's pure code movement). > > > (it's easy to squash patches later it necessary) > > > 2. Start small, pick a table generalize it and send as > > > one small patchset. Tables are often independent > > > and it's much easier on both author/reviewer to agree upon > > > changes and rewrite it if necessary. > > > > How would that be done? This series is on the bigger side, agreed, but > > most of it is really just code movement. It's a starting point, having > > a generic ACPI library is way beyond what this is trying to do. > I've tried to give suggestions how to restructure series > on per patch basis. In my opinion it quite possible to split > series in several smaller ones and it should really help with > making series cleaner and easier/faster to review/amend/merge > vs what we have in v5. > (it's more frustrating to rework large series vs smaller one) > > If something isn't clear, it's easy to reach out to me here > or directly (email/irc/github) for clarification/feed back. I assume the #1 goal is to add reduced HW support. So another option to speed up merging is to just go ahead and duplicate a bunch of code e.g. in pc_virt.c acpi/reduced.c or in any other file. This way it might be easier to see what's common code and what isn't. And I think offline Igor said he might prefer that way. Right Igor? > > > > Paolo > > > > > 3. when you think about refactoring acpi into a generic API > > > think about it as routines that go into a separate library > > > (pure acpi spec code) and qemu/acpi glue routines and > > > divide them correspondingly. > >
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On 19/11/18 19:14, Michael S. Tsirkin wrote: > On Mon, Nov 19, 2018 at 06:14:26PM +0100, Paolo Bonzini wrote: >> On 19/11/18 16:31, Igor Mammedov wrote: >>> I've tried to give suggestions how to restructure series >>> on per patch basis. In my opinion it quite possible to split >>> series in several smaller ones and it should really help with >>> making series cleaner and easier/faster to review/amend/merge >>> vs what we have in v5. >> >> This is true, on the other hand the series makes sense together and, >> even if the patches are more or less independent, they also all follow >> the same "plan". For reviewing v6, are you aware of Patchew's series >> diff functionality? It can tell you which patches had comments in v5, >> reorder patches if applicable, and display deleted and new patches at >> the right point in the series. >> >> v4->v5 is a bit messed up because Samuel probably added a diff order >> setup >> (https://patchew.org/QEMU/20181101102303.16439-1-sa...@linux.intel.com/diff/20181105014047.26447-1-sa...@linux.intel.com/) >> but it's very useful in general. >> >> Paolo > > Oh I didn't realize difforder breaks patchew. Or is the problem > only if one switches from no order to difforder? No, it's just that switching it on makes the inter-version diff much larger, because all hunks are reordered. difforder is not a problem. Paolo
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On 20/11/18 13:57, Igor Mammedov wrote: > On Mon, 19 Nov 2018 18:14:26 +0100 > Paolo Bonzini wrote: > >> On 19/11/18 16:31, Igor Mammedov wrote: >>> I've tried to give suggestions how to restructure series >>> on per patch basis. In my opinion it quite possible to split >>> series in several smaller ones and it should really help with >>> making series cleaner and easier/faster to review/amend/merge >>> vs what we have in v5. >> >> This is true, on the other hand the series makes sense together and, >> even if the patches are more or less independent, they also all follow >> the same "plan". For reviewing v6, are you aware of Patchew's series >> diff functionality? It can tell you which patches had comments in v5, >> reorder patches if applicable, and display deleted and new patches at >> the right point in the series. > Thanks, I'll give it a try. > > Suggestion to split series mostly comes from contributor's point of view, > it much easier to amend small series than a larger one. That's true, on the other hand rules exist to have exceptions. :) IIRC your AML builder patch was also a huge series, this is not very different. Paolo
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Mon, 19 Nov 2018 18:14:26 +0100 Paolo Bonzini wrote: > On 19/11/18 16:31, Igor Mammedov wrote: > > I've tried to give suggestions how to restructure series > > on per patch basis. In my opinion it quite possible to split > > series in several smaller ones and it should really help with > > making series cleaner and easier/faster to review/amend/merge > > vs what we have in v5. > > This is true, on the other hand the series makes sense together and, > even if the patches are more or less independent, they also all follow > the same "plan". For reviewing v6, are you aware of Patchew's series > diff functionality? It can tell you which patches had comments in v5, > reorder patches if applicable, and display deleted and new patches at > the right point in the series. Thanks, I'll give it a try. Suggestion to split series mostly comes from contributor's point of view, it much easier to amend small series than a larger one. > v4->v5 is a bit messed up because Samuel probably added a diff order > setup > (https://patchew.org/QEMU/20181101102303.16439-1-sa...@linux.intel.com/diff/20181105014047.26447-1-sa...@linux.intel.com/) > but it's very useful in general. > Paolo
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Mon, Nov 19, 2018 at 06:14:26PM +0100, Paolo Bonzini wrote: > On 19/11/18 16:31, Igor Mammedov wrote: > > I've tried to give suggestions how to restructure series > > on per patch basis. In my opinion it quite possible to split > > series in several smaller ones and it should really help with > > making series cleaner and easier/faster to review/amend/merge > > vs what we have in v5. > > This is true, on the other hand the series makes sense together and, > even if the patches are more or less independent, they also all follow > the same "plan". For reviewing v6, are you aware of Patchew's series > diff functionality? It can tell you which patches had comments in v5, > reorder patches if applicable, and display deleted and new patches at > the right point in the series. > > v4->v5 is a bit messed up because Samuel probably added a diff order > setup > (https://patchew.org/QEMU/20181101102303.16439-1-sa...@linux.intel.com/diff/20181105014047.26447-1-sa...@linux.intel.com/) > but it's very useful in general. > > Paolo Oh I didn't realize difforder breaks patchew. Or is the problem only if one switches from no order to difforder? -- MST
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On 19/11/18 16:31, Igor Mammedov wrote: > I've tried to give suggestions how to restructure series > on per patch basis. In my opinion it quite possible to split > series in several smaller ones and it should really help with > making series cleaner and easier/faster to review/amend/merge > vs what we have in v5. This is true, on the other hand the series makes sense together and, even if the patches are more or less independent, they also all follow the same "plan". For reviewing v6, are you aware of Patchew's series diff functionality? It can tell you which patches had comments in v5, reorder patches if applicable, and display deleted and new patches at the right point in the series. v4->v5 is a bit messed up because Samuel probably added a diff order setup (https://patchew.org/QEMU/20181101102303.16439-1-sa...@linux.intel.com/diff/20181105014047.26447-1-sa...@linux.intel.com/) but it's very useful in general. Paolo
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Fri, 16 Nov 2018 17:37:54 +0100 Paolo Bonzini wrote: > On 16/11/18 17:29, Igor Mammedov wrote: > > General suggestions for this series: > > 1. Preferably don't do multiple changes within a patch > > neither post huge patches (unless it's pure code movement). > > (it's easy to squash patches later it necessary) > > 2. Start small, pick a table generalize it and send as > > one small patchset. Tables are often independent > > and it's much easier on both author/reviewer to agree upon > > changes and rewrite it if necessary. > > How would that be done? This series is on the bigger side, agreed, but > most of it is really just code movement. It's a starting point, having > a generic ACPI library is way beyond what this is trying to do. I've tried to give suggestions how to restructure series on per patch basis. In my opinion it quite possible to split series in several smaller ones and it should really help with making series cleaner and easier/faster to review/amend/merge vs what we have in v5. (it's more frustrating to rework large series vs smaller one) If something isn't clear, it's easy to reach out to me here or directly (email/irc/github) for clarification/feed back. > > Paolo > > > 3. when you think about refactoring acpi into a generic API > > think about it as routines that go into a separate library > > (pure acpi spec code) and qemu/acpi glue routines and > > divide them correspondingly. >
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On 16/11/18 17:29, Igor Mammedov wrote: > General suggestions for this series: > 1. Preferably don't do multiple changes within a patch > neither post huge patches (unless it's pure code movement). > (it's easy to squash patches later it necessary) > 2. Start small, pick a table generalize it and send as > one small patchset. Tables are often independent > and it's much easier on both author/reviewer to agree upon > changes and rewrite it if necessary. How would that be done? This series is on the bigger side, agreed, but most of it is really just code movement. It's a starting point, having a generic ACPI library is way beyond what this is trying to do. Paolo > 3. when you think about refactoring acpi into a generic API > think about it as routines that go into a separate library > (pure acpi spec code) and qemu/acpi glue routines and > divide them correspondingly.
Re: [Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
On Mon, 5 Nov 2018 02:40:23 +0100 Samuel Ortiz wrote: > This patch set provides an ACPI code reorganization in preparation for > adding a shared hardware-reduced ACPI API to QEMU. > > The changes are coming from the NEMU [1] project where we're defining > a new x86 machine type: i386/virt. This is an EFI only, ACPI > hardware-reduced platform that is built on top of a generic > hardware-reduced ACPI API [2]. This API was initially based off the > generic parts of the arm/virt-acpi-build.c implementation, and the goal > is for both i386/virt and arm/virt to duplicate as little code as > possible by using this new, shared API. > > As a preliminary for adding this hardware-reduced ACPI API to QEMU, we did > some ACPI code reorganization with the following goals: > > * Share as much as possible of the current ACPI build APIs between > legacy and hardware-reduced ACPI. > * Share the ACPI build code across machine types and architectures and > remove the typical PC machine type dependency. > > The patches are also available in their own git branch [3]. I think, I'm done with reviewing this patchset, to sum up thanks for trying generalize acpi parts. It is implemented not exactly generic way and patches aren't split perfectly but we can work on it. General suggestions for this series: 1. Preferably don't do multiple changes within a patch neither post huge patches (unless it's pure code movement). (it's easy to squash patches later it necessary) 2. Start small, pick a table generalize it and send as one small patchset. Tables are often independent and it's much easier on both author/reviewer to agree upon changes and rewrite it if necessary. 3. when you think about refactoring acpi into a generic API think about it as routines that go into a separate library (pure acpi spec code) and qemu/acpi glue routines and divide them correspondingly. > [1] https://github.com/intel/nemu > [2] https://github.com/intel/nemu/blob/topic/virt-x86/hw/acpi/reduced.c > [3] https://github.com/intel/nemu/tree/topic/upstream/acpi > > v1 -> v2: >* Drop the hardware-reduced implementation for now. Our next patch >* set > will add hardware-reduced and convert arm/virt to it. >* Implement the ACPI build methods as a QOM Interface Class and >* convert > the PC machine type to it. >* acpi_conf_pc_init() uses a PCMachineState pointer and not a > MachineState one as its argument. > > v2 -> v3: >* Cc all relevant maintainers, no functional changes. > > v3 -> v4: >* Renamed all AcpiConfiguration pointers from conf to acpi_conf. >* Removed the ACPI_BUILD_ALIGN_SIZE export. >* Temporarily updated the arm virt build_rsdp() prototype for > bisectability purposes. >* Removed unneeded pci headers from acpi-build.c. >* Refactor the acpi PCI host getter so that it truly is architecture > agnostic, by carrying the PCI host pointer through the > AcpiConfiguration structure. >* Splitted the PCI host AML builder API export patch from the PCI > host and holes getter one. >* Reduced the build_srat() export scope to hw/i386 instead of the > broader hw/acpi. SRAT builders are truly architecture specific > and can hardly be generalized. >* Completed the ACPI builder documentation. > > v4 -> v5: >* Reorganize the ACPI RSDP export and XSDT implementation into 3 > patches. >* Fix the hw/i386/acpi header inclusions. > > Samuel Ortiz (16): > hw: i386: Decouple the ACPI build from the PC machine type > hw: acpi: Export ACPI build alignment API > hw: acpi: The RSDP build API can return void > hw: acpi: Export the RSDP build API > hw: acpi: Implement XSDT support for RSDP > hw: acpi: Factorize the RSDP build API implementation > hw: i386: Move PCI host definitions to pci_host.h > hw: acpi: Export the PCI host and holes getters > hw: acpi: Do not create hotplug method when handler is not defined > hw: i386: Make the hotpluggable memory size property more generic > hw: i386: Export the i386 ACPI SRAT build method > hw: i386: Export the MADT build method > hw: acpi: Define ACPI tables builder interface > hw: i386: Implement the ACPI builder interface for PC > hw: pci-host: piix: Return PCI host pointer instead of PCI bus > hw: i386: Set ACPI configuration PCI host pointer > > Sebastien Boeuf (2): > hw: acpi: Export the PCI hotplug API > hw: acpi: Retrieve the PCI bus from AcpiPciHpState > > Yang Zhong (6): > hw: acpi: Generalize AML build routines > hw: acpi: Factorize _OSC AML across architectures > hw: acpi: Export and generalize the PCI host AML API > hw: acpi: Export the MCFG getter > hw: acpi: Fix memory hotplug AML generation error > hw: i386: Refactor PCI host getter > > hw/i386/acpi-build.h |9 +- > include/hw/acpi/acpi-defs.h| 14 + > include/hw/acpi/acpi.h | 44 ++ > include/hw/acpi/aml-build.h| 47
[Qemu-devel] [PATCH v5 00/24] ACPI reorganization for hardware-reduced API addition
This patch set provides an ACPI code reorganization in preparation for adding a shared hardware-reduced ACPI API to QEMU. The changes are coming from the NEMU [1] project where we're defining a new x86 machine type: i386/virt. This is an EFI only, ACPI hardware-reduced platform that is built on top of a generic hardware-reduced ACPI API [2]. This API was initially based off the generic parts of the arm/virt-acpi-build.c implementation, and the goal is for both i386/virt and arm/virt to duplicate as little code as possible by using this new, shared API. As a preliminary for adding this hardware-reduced ACPI API to QEMU, we did some ACPI code reorganization with the following goals: * Share as much as possible of the current ACPI build APIs between legacy and hardware-reduced ACPI. * Share the ACPI build code across machine types and architectures and remove the typical PC machine type dependency. The patches are also available in their own git branch [3]. [1] https://github.com/intel/nemu [2] https://github.com/intel/nemu/blob/topic/virt-x86/hw/acpi/reduced.c [3] https://github.com/intel/nemu/tree/topic/upstream/acpi v1 -> v2: * Drop the hardware-reduced implementation for now. Our next patch * set will add hardware-reduced and convert arm/virt to it. * Implement the ACPI build methods as a QOM Interface Class and * convert the PC machine type to it. * acpi_conf_pc_init() uses a PCMachineState pointer and not a MachineState one as its argument. v2 -> v3: * Cc all relevant maintainers, no functional changes. v3 -> v4: * Renamed all AcpiConfiguration pointers from conf to acpi_conf. * Removed the ACPI_BUILD_ALIGN_SIZE export. * Temporarily updated the arm virt build_rsdp() prototype for bisectability purposes. * Removed unneeded pci headers from acpi-build.c. * Refactor the acpi PCI host getter so that it truly is architecture agnostic, by carrying the PCI host pointer through the AcpiConfiguration structure. * Splitted the PCI host AML builder API export patch from the PCI host and holes getter one. * Reduced the build_srat() export scope to hw/i386 instead of the broader hw/acpi. SRAT builders are truly architecture specific and can hardly be generalized. * Completed the ACPI builder documentation. v4 -> v5: * Reorganize the ACPI RSDP export and XSDT implementation into 3 patches. * Fix the hw/i386/acpi header inclusions. Samuel Ortiz (16): hw: i386: Decouple the ACPI build from the PC machine type hw: acpi: Export ACPI build alignment API hw: acpi: The RSDP build API can return void hw: acpi: Export the RSDP build API hw: acpi: Implement XSDT support for RSDP hw: acpi: Factorize the RSDP build API implementation hw: i386: Move PCI host definitions to pci_host.h hw: acpi: Export the PCI host and holes getters hw: acpi: Do not create hotplug method when handler is not defined hw: i386: Make the hotpluggable memory size property more generic hw: i386: Export the i386 ACPI SRAT build method hw: i386: Export the MADT build method hw: acpi: Define ACPI tables builder interface hw: i386: Implement the ACPI builder interface for PC hw: pci-host: piix: Return PCI host pointer instead of PCI bus hw: i386: Set ACPI configuration PCI host pointer Sebastien Boeuf (2): hw: acpi: Export the PCI hotplug API hw: acpi: Retrieve the PCI bus from AcpiPciHpState Yang Zhong (6): hw: acpi: Generalize AML build routines hw: acpi: Factorize _OSC AML across architectures hw: acpi: Export and generalize the PCI host AML API hw: acpi: Export the MCFG getter hw: acpi: Fix memory hotplug AML generation error hw: i386: Refactor PCI host getter hw/i386/acpi-build.h |9 +- include/hw/acpi/acpi-defs.h| 14 + include/hw/acpi/acpi.h | 44 ++ include/hw/acpi/aml-build.h| 47 ++ include/hw/acpi/builder.h | 100 +++ include/hw/i386/acpi.h | 28 + include/hw/i386/pc.h | 49 +- include/hw/mem/memory-device.h |2 + include/hw/pci/pci_host.h |6 + hw/acpi/aml-build.c| 981 + hw/acpi/builder.c | 97 +++ hw/acpi/cpu.c |8 +- hw/acpi/cpu_hotplug.c |9 +- hw/acpi/memory_hotplug.c | 21 +- hw/acpi/pcihp.c| 10 +- hw/arm/virt-acpi-build.c | 93 +-- hw/i386/acpi-build.c | 1072 +++- hw/i386/pc.c | 198 +++--- hw/i386/pc_piix.c | 36 +- hw/i386/pc_q35.c | 22 +- hw/i386/xen/xen-hvm.c | 19 +- hw/pci-host/piix.c | 32 +- stubs/pci-host-piix.c |6 - hw/acpi/Makefile.objs |1 + stubs/Makefile.objs|1 - 25 files changed, 1644 insertions(+), 1261 deletions(-) create mode 100644 include/hw/acpi/builder.h create mode 100644 include/hw/i386/acpi.h
