On Wed, May 18, 2016 at 04:38:26PM +0200, Igor Mammedov wrote: > On Wed, 18 May 2016 17:09:20 +0300 > "Michael S. Tsirkin" <m...@redhat.com> wrote: > > > On Wed, May 18, 2016 at 03:53:08PM +0200, Igor Mammedov wrote: > > > On Sun, 15 May 2016 22:23:30 +0300 > > > Marcel Apfelbaum <mar...@redhat.com> wrote: > > > > > > > Hi, > > > > > > > > First two patches allocate (max_reserved_ram - > > > > max_addr_cpu_addressable) range for PCI hotplug > > > > (for PC Machines) instead of the previous 64-bit PCI window that > > > > included only > > > > the ranges allocated by the firmware. > > > > > > > > The next two patches fix 64-bit CRS computations. > > > I'd would add test case + expected tables as the first 2 patches > > > and then finish series with expected tables update with fixed 64bit range > > > > > > > I don't think we necessarily need this noise when running tests. > > Adding tests last is OK I think, but I prefer updating expected acpi tables > > myself as these can't be merged. > the reason I've asked for tables updating is that it makes > life of reviewers easier, i.e. reviewer can just run > test and see a ASL diff before fix and after. > > Patches with updates could be discarded during merge. > /mark them with "DONT APPLY"/
I think a new test will currently fail if expected is not there though. We might want to fix that to allow the workflow you describe. > > > > > as experiment I've hacked existing piix4 case: > > > > > > @@ -744,7 +744,9 @@ static void test_acpi_piix4_tcg(void) > > > */ > > > memset(&data, 0, sizeof(data)); > > > data.machine = MACHINE_PC; > > > - test_acpi_one("-machine accel=tcg", &data); > > > + test_acpi_one("-machine accel=tcg" > > > + " -device pxb,id=bridge1,bus=pci.0,bus_nr=4" > > > + " -device ivshmem,bus=bridge1,size=4G,shm", &data); > > > free_test_data(&data); > > > } > > > > > > And it shows not related to this series, but another pxb issue > > > > > > + External (_SB_.PCI0.S18_.PCNT, MethodObj) // Warning: Unresolved > > > method, guessing 0 arguments > > > ... > > > @@ -1197,8 +1322,8 @@ DefinitionBlock > > > ("tests/acpi-test-data/pc/DSDT.aml", "DSDT", 1, "BOCHS ", "BXPCD > > > > > > Device (S18) > > > { > > > - Name (_SUN, 0x03) // _SUN: Slot User Number > > > Name (_ADR, 0x00030000) // _ADR: Address > > > + Name (_SUN, 0x03) // _SUN: Slot User Number > > > Method (_EJ0, 1, NotSerialized) // _EJx: Eject Device > > > { > > > PCEJ (BSEL, _SUN) > > > @@ -1638,6 +1763,7 @@ DefinitionBlock > > > ("tests/acpi-test-data/pc/DSDT.aml", "DSDT", 1, "BOCHS ", "BXPCD > > > BNUM = Zero > > > DVNT (PCIU, One) > > > DVNT (PCID, 0x03) > > > + ^S18.PCNT () > > > } > > > } > > > } > > > > > > so it's better to have test case in place so that changes to pxb > > > parts wouldn't go unnoticed and would be observable. > > > > > > > > > Also from above experiment I see that pxb duplicates and uses > > > the same _PRT method as PCI0, probably should be changed to > > > something like: > > > > > > Method(_PRT) > > > return ^PCI0._PRT() > > > > > > > v1 -> v2: > > > > - resolved some styling issues (Laszlo) > > > > - rebase on latest master (Laszlo) > > > > > > > > Thank you, > > > > Marcel > > > > > > > > Marcel Apfelbaum (4): > > > > hw/pc: extract reserved memory end computation to a standalone > > > > function > > > > pci: reserve 64 bit MMIO range for PCI hotplug > > > > acpi: refactor pxb crs computation > > > > hw/apci: handle 64-bit MMIO regions correctly > > > > > > > > hw/i386/acpi-build.c | 127 > > > > ++++++++++++++++++++++++++++++++++++--------------- > > > > hw/i386/pc.c | 29 ++++++++---- > > > > hw/pci/pci.c | 16 ++++++- > > > > include/hw/i386/pc.h | 1 + > > > > 4 files changed, 127 insertions(+), 46 deletions(-) > > > > > >