Re: [Qemu-devel] [PATCH 00/13] acpi: Make piix-specific and q35-specific code generic

[Eduardo Habkost]

On Fri, Dec 04, 2015 at 02:24:41PM +0100, Igor Mammedov wrote:
> On Thu, 3 Dec 2015 15:16:21 -0200
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Thu, Dec 03, 2015 at 04:19:19PM +0100, Igor Mammedov wrote:
> > > On Wed,  2 Dec 2015 20:22:45 -0200
> > > Eduardo Habkost <address@hidden> wrote:
> > > 
> > > > This series removes piix-specific and q35-specific code from
> > > > acpi-build.c, making it generic and without direct dependencies
> > > > to piix and q35 code.
> > > we are conflicting reshuffling acpi-build.c at the same time
> > > could be cleanup done on top of my refactoring
> > > drop_ASL_support_v1 branch:
> > > 
> > > https://github.com/imammedo/qemu/commits/drop_ASL_support_v1
> > 
> > Do you plan to submit it soon?
> I plan to submit it as soon as 2.5 is released since Michael is busy
> with 2.5 and possibly the only reviewer.
> But if it help I can submit it earlier and add you in CC.

I have been submitting for-2.6 patches, already. It helps get the
patches reviewed earlier so they have a chance to get applied as
soon as 2.5 is released.

> 
> > 
> > It looks like my series and yours go in opposite directions. I am
> > trying to remove piix4-specific code from acpi-build, and you are
> > addding a is_piix4 field to AcpiMiscInfo. Your series makes much
> > more difficult to remove piix4-specific code from acpi-build.c,
> > but this sounds like a reasonable price for moving the dsdt.dsl
> > logic to C. Maybe I should change my goal from "removing
> > piix4-specific code" to just simplifying the code.
> It should be ok to remove explicit detection of piix4/q35 from
> acpi-build.c, what I wouldn't wish though is to pull ACPI deps
> into boards codebase. What we have in acpi-build.c is mostly
> firmware generation code and it would be better to keep it there.

Agreed. And your refactoring makes the piix4/q35 differences much
more visible.

After making the piix4/q35 differences visible, I wonder if we
could make the reason for each "if (piix4)" line clearer. For
example, I assume some of the "if (piix4)" lines could become
"if (acpi_pci_hotplug_enabled())", but I don't know which ones.

> 
> After merging refactoring, I plan to simplify it by moving out
> generic memory/cpu hotplug parts into hw/acpi/,
> but the rest of DSDT will probably stay there and we can
> merge SSDT into DSDT which also should help to simplify it.
> 
> But for now it's dumb refactoring which is byte compatible with
> DSDTs we have in ASL to avoid regressions in such huge series.

I will let you make the cleanups you are planning, first. After
that is settled, I will check which of the patches in this series
still make sense. My original plan was to only suggest 1 or 2
simple cleanups after a discussion with Marcel. :)

-- 
Eduardo