[Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Marcel Apfelbaum]

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.

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(-)

-- 
2.4.3

[Qemu-devel] [PATCH V2 1/4] hw/pc: extract reserved memory end computation to a standalone function

[Marcel Apfelbaum]

This code will be reused when calculating 64-bit MMIO hotplug ranges.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/pc.c         | 29 +++++++++++++++++++++--------
 include/hw/i386/pc.h |  1 +
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 99437e0..a7791e3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1280,6 +1280,7 @@ void pc_memory_init(PCMachineState *pcms,
     FWCfgState *fw_cfg;
     MachineState *machine = MACHINE(pcms);
     PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    uint64_t res_mem_end;
 
     assert(machine->ram_size == pcms->below_4g_mem_size +
                                 pcms->above_4g_mem_size);
@@ -1375,15 +1376,10 @@ void pc_memory_init(PCMachineState *pcms,
 
     rom_set_fw(fw_cfg);
 
-    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
+    res_mem_end = pc_machine_get_reserved_memory_end(pcms);
+    if (res_mem_end) {
         uint64_t *val = g_malloc(sizeof(*val));
-        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
-        uint64_t res_mem_end = pcms->hotplug_memory.base;
-
-        if (!pcmc->broken_reserved_end) {
-            res_mem_end += memory_region_size(&pcms->hotplug_memory.mr);
-        }
-        *val = cpu_to_le64(ROUND_UP(res_mem_end, 0x1ULL << 30));
+        *val = cpu_to_le64(res_mem_end);
         fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, sizeof(*val));
     }
 
@@ -1853,6 +1849,23 @@ bool pc_machine_is_smm_enabled(PCMachineState *pcms)
     return false;
 }
 
+uint64_t pc_machine_get_reserved_memory_end(PCMachineState *pcms)
+{
+    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
+    uint64_t res_mem_end = 0;
+
+    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
+        res_mem_end = pcms->hotplug_memory.base;
+
+        if (!pcmc->broken_reserved_end) {
+            res_mem_end += memory_region_size(&pcms->hotplug_memory.mr);
+        }
+        res_mem_end = ROUND_UP(res_mem_end, 0x1ULL << 30);
+    }
+
+    return res_mem_end;
+}
+
 static void pc_machine_get_smm(Object *obj, Visitor *v, const char *name,
                                void *opaque, Error **errp)
 {
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 96f0b66..7c25814 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -223,6 +223,7 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
 extern int fd_bootchk;
 
 bool pc_machine_is_smm_enabled(PCMachineState *pcms);
+uint64_t pc_machine_get_reserved_memory_end(PCMachineState *pcms);
 void pc_register_ferr_irq(qemu_irq irq);
 void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH V2 1/4] hw/pc: extract reserved memory end computation to a standalone function

[Igor Mammedov]

On Sun, 15 May 2016 22:23:31 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> This code will be reused when calculating 64-bit MMIO hotplug ranges.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/i386/pc.c         | 29 +++++++++++++++++++++--------
>  include/hw/i386/pc.h |  1 +
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 99437e0..a7791e3 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1280,6 +1280,7 @@ void pc_memory_init(PCMachineState *pcms,
>      FWCfgState *fw_cfg;
>      MachineState *machine = MACHINE(pcms);
>      PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    uint64_t res_mem_end;
>  
>      assert(machine->ram_size == pcms->below_4g_mem_size +
>                                  pcms->above_4g_mem_size);
> @@ -1375,15 +1376,10 @@ void pc_memory_init(PCMachineState *pcms,
>  
>      rom_set_fw(fw_cfg);
>  
> -    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
> +    res_mem_end = pc_machine_get_reserved_memory_end(pcms);
> +    if (res_mem_end) {
>          uint64_t *val = g_malloc(sizeof(*val));
> -        PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> -        uint64_t res_mem_end = pcms->hotplug_memory.base;
> -
> -        if (!pcmc->broken_reserved_end) {
> -            res_mem_end += memory_region_size(&pcms->hotplug_memory.mr);
> -        }
> -        *val = cpu_to_le64(ROUND_UP(res_mem_end, 0x1ULL << 30));
> +        *val = cpu_to_le64(res_mem_end);
>          fw_cfg_add_file(fw_cfg, "etc/reserved-memory-end", val, sizeof(*val));
>      }
>  
> @@ -1853,6 +1849,23 @@ bool pc_machine_is_smm_enabled(PCMachineState *pcms)
>      return false;
>  }
>  
> +uint64_t pc_machine_get_reserved_memory_end(PCMachineState *pcms)
> +{
> +    PCMachineClass *pcmc = PC_MACHINE_GET_CLASS(pcms);
> +    uint64_t res_mem_end = 0;
> +
> +    if (pcmc->has_reserved_memory && pcms->hotplug_memory.base) {
> +        res_mem_end = pcms->hotplug_memory.base;
> +
> +        if (!pcmc->broken_reserved_end) {
> +            res_mem_end += memory_region_size(&pcms->hotplug_memory.mr);
> +        }
> +        res_mem_end = ROUND_UP(res_mem_end, 0x1ULL << 30);
> +    }
> +
> +    return res_mem_end;
> +}
> +
>  static void pc_machine_get_smm(Object *obj, Visitor *v, const char *name,
>                                 void *opaque, Error **errp)
>  {
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 96f0b66..7c25814 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -223,6 +223,7 @@ void i8042_setup_a20_line(ISADevice *dev, qemu_irq *a20_out);
>  extern int fd_bootchk;
>  
>  bool pc_machine_is_smm_enabled(PCMachineState *pcms);
> +uint64_t pc_machine_get_reserved_memory_end(PCMachineState *pcms);
>  void pc_register_ferr_irq(qemu_irq irq);
>  void pc_acpi_smi_interrupt(void *opaque, int irq, int level);
>  

[Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

Using the firmware assigned MMIO ranges for 64-bit PCI window
leads to zero space for hot-plugging PCI devices over 4G.

PC machines can use the whole CPU addressable range after
the space reserved for memory-hotplug.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/pci/pci.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index bb605ef..44dd949 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -41,6 +41,7 @@
 #include "hw/hotplug.h"
 #include "hw/boards.h"
 #include "qemu/cutils.h"
+#include "hw/i386/pc.h"
 
 //#define DEBUG_PCI
 #ifdef DEBUG_PCI
@@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
 
 void pci_bus_get_w64_range(PCIBus *bus, Range *range)
 {
-    range->begin = range->end = 0;
-    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
+    Object *machine = qdev_get_machine();
+    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
+        PCMachineState *pcms = PC_MACHINE(machine);
+        range->begin = pc_machine_get_reserved_memory_end(pcms);
+        if (!range->begin) {
+            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
+                                    1ULL << 30);
+        }
+        range->end = 1ULL << 40; /* 40 bits physical */
+    } else {
+        range->begin = range->end = 0;
+        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
+    }
 }
 
 static bool pcie_has_upstream_port(PCIDevice *dev)
-- 
2.4.3

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Igor Mammedov]

On Sun, 15 May 2016 22:23:32 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> Using the firmware assigned MMIO ranges for 64-bit PCI window
> leads to zero space for hot-plugging PCI devices over 4G.
> 
> PC machines can use the whole CPU addressable range after
> the space reserved for memory-hotplug.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/pci/pci.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bb605ef..44dd949 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -41,6 +41,7 @@
>  #include "hw/hotplug.h"
>  #include "hw/boards.h"
>  #include "qemu/cutils.h"
> +#include "hw/i386/pc.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>  
>  void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>  {
> -    range->begin = range->end = 0;
> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> +    Object *machine = qdev_get_machine();
> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> +        PCMachineState *pcms = PC_MACHINE(machine);
> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
that line should break linking on other targets which don't have
pc_machine_get_reserved_memory_end()
probably for got to add stub.

> +        if (!range->begin) {
> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> +                                    1ULL << 30);
> +        }
> +        range->end = 1ULL << 40; /* 40 bits physical */
x86 specific in generic code

ARM also has 64-bit PCI MMIO /git grep VIRT_PCIE_MMIO_HIGH/
perhaps range should be a property of PCI bus,
where a board sets its own values for start/size

> +    } else {
> +        range->begin = range->end = 0;
> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> +    }
>  }
>  
>  static bool pcie_has_upstream_port(PCIDevice *dev)

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/16/2016 11:24 AM, Igor Mammedov wrote:
> On Sun, 15 May 2016 22:23:32 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>> leads to zero space for hot-plugging PCI devices over 4G.
>>
>> PC machines can use the whole CPU addressable range after
>> the space reserved for memory-hotplug.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/pci/pci.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index bb605ef..44dd949 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -41,6 +41,7 @@
>>   #include "hw/hotplug.h"
>>   #include "hw/boards.h"
>>   #include "qemu/cutils.h"
>> +#include "hw/i386/pc.h"
>>
>>   //#define DEBUG_PCI
>>   #ifdef DEBUG_PCI
>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>


Hi Igor,
Thanks for reviewing this series.

>>   void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>   {
>> -    range->begin = range->end = 0;
>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>> +    Object *machine = qdev_get_machine();
>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>> +        PCMachineState *pcms = PC_MACHINE(machine);
>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
> that line should break linking on other targets which don't have
> pc_machine_get_reserved_memory_end()
> probably for got to add stub.
>

I cross-compiled all the targets with no problem.  It seems no stub is required.


>> +        if (!range->begin) {
>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>> +                                    1ULL << 30);
>> +        }
>> +        range->end = 1ULL << 40; /* 40 bits physical */
> x86 specific in generic code
>

I am aware I put x86 code in the generic pci code, but I limited it with
     if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
So we have a generic rule for getting the w64 range and a specific one for PC machines.

The alternative would be a w64 range per host-bridge, not bus.
Adding a pci_bus_get_w64_range function to PCIHostBridgeClass,
defaulting in current implementation for the base class and
overriding it for piix/q35 looks OK to you?

I thought about it, but it seemed over-engineered as opposed
to the 'simple' if statement, however I can try it if you think is better.

> ARM also has 64-bit PCI MMIO /git grep VIRT_PCIE_MMIO_HIGH/

I had a look and ARM does not use this infrastructure, it has its own abstractions,
a memmap list. This is the other reason I selected this implementation,
because is not really used by other targets (but it can be used in the future)


Thanks,
Marcel

> perhaps range should be a property of PCI bus,
> where a board sets its own values for start/size
>
>> +    } else {
>> +        range->begin = range->end = 0;
>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>> +    }
>>   }
>>
>>   static bool pcie_has_upstream_port(PCIDevice *dev)
>

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Igor Mammedov]

On Mon, 16 May 2016 13:14:51 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 05/16/2016 11:24 AM, Igor Mammedov wrote:
> > On Sun, 15 May 2016 22:23:32 +0300
> > Marcel Apfelbaum <marcel@redhat.com> wrote:
> >  
> >> Using the firmware assigned MMIO ranges for 64-bit PCI window
> >> leads to zero space for hot-plugging PCI devices over 4G.
> >>
> >> PC machines can use the whole CPU addressable range after
> >> the space reserved for memory-hotplug.
> >>
> >> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >> ---
> >>   hw/pci/pci.c | 16 ++++++++++++++--
> >>   1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index bb605ef..44dd949 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -41,6 +41,7 @@
> >>   #include "hw/hotplug.h"
> >>   #include "hw/boards.h"
> >>   #include "qemu/cutils.h"
> >> +#include "hw/i386/pc.h"
> >>
> >>   //#define DEBUG_PCI
> >>   #ifdef DEBUG_PCI
> >> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> >>  
> 
> 
> Hi Igor,
> Thanks for reviewing this series.
> 
> >>   void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >>   {
> >> -    range->begin = range->end = 0;
> >> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >> +    Object *machine = qdev_get_machine();
> >> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> >> +        PCMachineState *pcms = PC_MACHINE(machine);
> >> +        range->begin = pc_machine_get_reserved_memory_end(pcms);  
> > that line should break linking on other targets which don't have
> > pc_machine_get_reserved_memory_end()
> > probably for got to add stub.
> >  
> 
> I cross-compiled all the targets with no problem.  It seems no stub is required.
./configure && make

  LINK  aarch64-softmmu/qemu-system-aarch64
../hw/pci/pci.o: In function `pci_bus_get_w64_range':
/home/imammedo/builds/qemu/hw/pci/pci.c:2474: undefined reference to `pc_machine_get_reserved_memory_end'
collect2: error: ld returned 1 exit status

> 
> 
> >> +        if (!range->begin) {
> >> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> >> +                                    1ULL << 30);
> >> +        }
mayby move above hunk to pc_machine_get_reserved_memory_end()
so it would always return valid value.

> >> +        range->end = 1ULL << 40; /* 40 bits physical */  
> > x86 specific in generic code
> >  
> 
> I am aware I put x86 code in the generic pci code, but I limited it with
>      if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> So we have a generic rule for getting the w64 range and a specific one for PC machines.
> 
> The alternative would be a w64 range per host-bridge, not bus.
> Adding a pci_bus_get_w64_range function to PCIHostBridgeClass,
> defaulting in current implementation for the base class and
> overriding it for piix/q35 looks OK to you?
> 
> I thought about it, but it seemed over-engineered as opposed
> to the 'simple' if statement, however I can try it if you think is better.
> 
> > ARM also has 64-bit PCI MMIO /git grep VIRT_PCIE_MMIO_HIGH/  
> 
> I had a look and ARM does not use this infrastructure, it has its own abstractions,
> a memmap list. This is the other reason I selected this implementation,
> because is not really used by other targets (but it can be used in the future)

if it's only for x86 and whatever was programmed by BIOS is ignored,
I'd drop PCI_HOST_PROP_PCI_HOLE64_*/pci_bus_get_w64_range() indirection
and just directly use pc_machine_get_reserved_memory_end()
from acpi-build.c

in that case you won't need a stub for pc_machine_get_reserved_memory_end()
as its usage will be limited only to hw/i386 scope.

Given opportunity, I'd drop PCI_HOST_PROP_PCI_HOLE* properties,
but for it we need ACK from libvirt side in case they are using it
for some reason.

> 
> 
> Thanks,
> Marcel
> 
> > perhaps range should be a property of PCI bus,
> > where a board sets its own values for start/size
> >  
> >> +    } else {
> >> +        range->begin = range->end = 0;
> >> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >> +    }
> >>   }
> >>
> >>   static bool pcie_has_upstream_port(PCIDevice *dev)  
> >  
> 

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/16/2016 05:19 PM, Igor Mammedov wrote:
> On Mon, 16 May 2016 13:14:51 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 05/16/2016 11:24 AM, Igor Mammedov wrote:
>>> On Sun, 15 May 2016 22:23:32 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>>>> leads to zero space for hot-plugging PCI devices over 4G.
>>>>
>>>> PC machines can use the whole CPU addressable range after
>>>> the space reserved for memory-hotplug.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>>    hw/pci/pci.c | 16 ++++++++++++++--
>>>>    1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index bb605ef..44dd949 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -41,6 +41,7 @@
>>>>    #include "hw/hotplug.h"
>>>>    #include "hw/boards.h"
>>>>    #include "qemu/cutils.h"
>>>> +#include "hw/i386/pc.h"
>>>>
>>>>    //#define DEBUG_PCI
>>>>    #ifdef DEBUG_PCI
>>>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>>>
>>
>>
>> Hi Igor,
>> Thanks for reviewing this series.
>>
>>>>    void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>>    {
>>>> -    range->begin = range->end = 0;
>>>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>> +    Object *machine = qdev_get_machine();
>>>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>>>> +        PCMachineState *pcms = PC_MACHINE(machine);
>>>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>>> that line should break linking on other targets which don't have
>>> pc_machine_get_reserved_memory_end()
>>> probably for got to add stub.
>>>
>>
>> I cross-compiled all the targets with no problem.  It seems no stub is required.
> ./configure && make
>
>    LINK  aarch64-softmmu/qemu-system-aarch64
> ../hw/pci/pci.o: In function `pci_bus_get_w64_range':
> /home/imammedo/builds/qemu/hw/pci/pci.c:2474: undefined reference to `pc_machine_get_reserved_memory_end'
> collect2: error: ld returned 1 exit status

Ooops, I did configured it to cross-compile everything, but I somehow missed it.
I'll take care of it.


>
>>
>>
>>>> +        if (!range->begin) {
>>>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>>>> +                                    1ULL << 30);
>>>> +        }
> mayby move above hunk to pc_machine_get_reserved_memory_end()
> so it would always return valid value.
>
>>>> +        range->end = 1ULL << 40; /* 40 bits physical */
>>> x86 specific in generic code
>>>
>>
>> I am aware I put x86 code in the generic pci code, but I limited it with
>>       if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>> So we have a generic rule for getting the w64 range and a specific one for PC machines.
>>
>> The alternative would be a w64 range per host-bridge, not bus.
>> Adding a pci_bus_get_w64_range function to PCIHostBridgeClass,
>> defaulting in current implementation for the base class and
>> overriding it for piix/q35 looks OK to you?
>>
>> I thought about it, but it seemed over-engineered as opposed
>> to the 'simple' if statement, however I can try it if you think is better.
>>
>>> ARM also has 64-bit PCI MMIO /git grep VIRT_PCIE_MMIO_HIGH/
>>
>> I had a look and ARM does not use this infrastructure, it has its own abstractions,
>> a memmap list. This is the other reason I selected this implementation,
>> because is not really used by other targets (but it can be used in the future)
>
> if it's only for x86 and whatever was programmed by BIOS is ignored,
> I'd drop PCI_HOST_PROP_PCI_HOLE64_*/pci_bus_get_w64_range() indirection
> and just directly use pc_machine_get_reserved_memory_end()
> from acpi-build.c
>
> in that case you won't need a stub for pc_machine_get_reserved_memory_end()
> as its usage will be limited only to hw/i386 scope.
>

Good point, I'll try this.

> Given opportunity, I'd drop PCI_HOST_PROP_PCI_HOLE* properties,
> but for it we need ACK from libvirt side in case they are using it
> for some reason.

It seems out of the scope of this series, however I can do it on top.

Thanks,
Marcel

>
>>
>>
>> Thanks,
>> Marcel
>>
>>> perhaps range should be a property of PCI bus,
>>> where a board sets its own values for start/size
>>>
>>>> +    } else {
>>>> +        range->begin = range->end = 0;
>>>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>> +    }
>>>>    }
>>>>
>>>>    static bool pcie_has_upstream_port(PCIDevice *dev)
>>>
>>
>

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Igor Mammedov]

On Wed, 18 May 2016 17:07:05 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 05/16/2016 05:19 PM, Igor Mammedov wrote:
> > On Mon, 16 May 2016 13:14:51 +0300
> > Marcel Apfelbaum <marcel@redhat.com> wrote:
> >  
> >> On 05/16/2016 11:24 AM, Igor Mammedov wrote:  
> >>> On Sun, 15 May 2016 22:23:32 +0300
> >>> Marcel Apfelbaum <marcel@redhat.com> wrote:
> >>>  
> >>>> Using the firmware assigned MMIO ranges for 64-bit PCI window
> >>>> leads to zero space for hot-plugging PCI devices over 4G.
> >>>>
> >>>> PC machines can use the whole CPU addressable range after
> >>>> the space reserved for memory-hotplug.
> >>>>
> >>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>> ---
> >>>>    hw/pci/pci.c | 16 ++++++++++++++--
> >>>>    1 file changed, 14 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>> index bb605ef..44dd949 100644
> >>>> --- a/hw/pci/pci.c
> >>>> +++ b/hw/pci/pci.c
> >>>> @@ -41,6 +41,7 @@
> >>>>    #include "hw/hotplug.h"
> >>>>    #include "hw/boards.h"
> >>>>    #include "qemu/cutils.h"
> >>>> +#include "hw/i386/pc.h"
> >>>>
> >>>>    //#define DEBUG_PCI
> >>>>    #ifdef DEBUG_PCI
> >>>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> >>>>  
> >>
> >>
> >> Hi Igor,
> >> Thanks for reviewing this series.
> >>  
> >>>>    void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >>>>    {
> >>>> -    range->begin = range->end = 0;
> >>>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>>> +    Object *machine = qdev_get_machine();
> >>>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> >>>> +        PCMachineState *pcms = PC_MACHINE(machine);
> >>>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);  
> >>> that line should break linking on other targets which don't have
> >>> pc_machine_get_reserved_memory_end()
> >>> probably for got to add stub.
> >>>  
> >>
> >> I cross-compiled all the targets with no problem.  It seems no stub is required.  
> > ./configure && make
> >
> >    LINK  aarch64-softmmu/qemu-system-aarch64
> > ../hw/pci/pci.o: In function `pci_bus_get_w64_range':
> > /home/imammedo/builds/qemu/hw/pci/pci.c:2474: undefined reference to `pc_machine_get_reserved_memory_end'
> > collect2: error: ld returned 1 exit status  
> 
> Ooops, I did configured it to cross-compile everything, but I somehow missed it.
> I'll take care of it.
> 
> 
> >  
> >>
> >>  
> >>>> +        if (!range->begin) {
> >>>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> >>>> +                                    1ULL << 30);
> >>>> +        }  
> > mayby move above hunk to pc_machine_get_reserved_memory_end()
> > so it would always return valid value.
> >  
> >>>> +        range->end = 1ULL << 40; /* 40 bits physical */  
> >>> x86 specific in generic code
> >>>  
> >>
> >> I am aware I put x86 code in the generic pci code, but I limited it with
> >>       if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> >> So we have a generic rule for getting the w64 range and a specific one for PC machines.
> >>
> >> The alternative would be a w64 range per host-bridge, not bus.
> >> Adding a pci_bus_get_w64_range function to PCIHostBridgeClass,
> >> defaulting in current implementation for the base class and
> >> overriding it for piix/q35 looks OK to you?
> >>
> >> I thought about it, but it seemed over-engineered as opposed
> >> to the 'simple' if statement, however I can try it if you think is better.
> >>  
> >>> ARM also has 64-bit PCI MMIO /git grep VIRT_PCIE_MMIO_HIGH/  
> >>
> >> I had a look and ARM does not use this infrastructure, it has its own abstractions,
> >> a memmap list. This is the other reason I selected this implementation,
> >> because is not really used by other targets (but it can be used in the future)  
> >
> > if it's only for x86 and whatever was programmed by BIOS is ignored,
> > I'd drop PCI_HOST_PROP_PCI_HOLE64_*/pci_bus_get_w64_range() indirection
> > and just directly use pc_machine_get_reserved_memory_end()
> > from acpi-build.c
> >
> > in that case you won't need a stub for pc_machine_get_reserved_memory_end()
> > as its usage will be limited only to hw/i386 scope.
> >  
> 
> Good point, I'll try this.
> 
> > Given opportunity, I'd drop PCI_HOST_PROP_PCI_HOLE* properties,
> > but for it we need ACK from libvirt side in case they are using it
> > for some reason.  
> 
> It seems out of the scope of this series, however I can do it on top.
it's just nice to have cleanup, but I don't insist on it.

the thing here is that if pc_machine_get_reserved_memory_end() were used
directly for acpi-build.c then properties as is would give old/different values.

> 
> Thanks,
> Marcel
> 
> >  
> >>
> >>
> >> Thanks,
> >> Marcel
> >>  
> >>> perhaps range should be a property of PCI bus,
> >>> where a board sets its own values for start/size
> >>>  
> >>>> +    } else {
> >>>> +        range->begin = range->end = 0;
> >>>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>>> +    }
> >>>>    }
> >>>>
> >>>>    static bool pcie_has_upstream_port(PCIDevice *dev)  
> >>>  
> >>  
> >  
> 

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/18/2016 05:26 PM, Igor Mammedov wrote:
> On Wed, 18 May 2016 17:07:05 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 05/16/2016 05:19 PM, Igor Mammedov wrote:
>>> On Mon, 16 May 2016 13:14:51 +0300
>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>
>>>> On 05/16/2016 11:24 AM, Igor Mammedov wrote:
>>>>> On Sun, 15 May 2016 22:23:32 +0300
>>>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>>>
>>>>>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>>>>>> leads to zero space for hot-plugging PCI devices over 4G.
>>>>>>
>>>>>> PC machines can use the whole CPU addressable range after
>>>>>> the space reserved for memory-hotplug.
>>>>>>
>>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>>>> ---
>>>>>>     hw/pci/pci.c | 16 ++++++++++++++--
>>>>>>     1 file changed, 14 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>>> index bb605ef..44dd949 100644
>>>>>> --- a/hw/pci/pci.c
>>>>>> +++ b/hw/pci/pci.c
>>>>>> @@ -41,6 +41,7 @@
>>>>>>     #include "hw/hotplug.h"
>>>>>>     #include "hw/boards.h"
>>>>>>     #include "qemu/cutils.h"
>>>>>> +#include "hw/i386/pc.h"
>>>>>>
>>>>>>     //#define DEBUG_PCI
>>>>>>     #ifdef DEBUG_PCI
>>>>>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>>>>>
>>>>
>>>>
>>>> Hi Igor,
>>>> Thanks for reviewing this series.
>>>>
>>>>>>     void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>>>>     {
>>>>>> -    range->begin = range->end = 0;
>>>>>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>>>> +    Object *machine = qdev_get_machine();
>>>>>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>>>>>> +        PCMachineState *pcms = PC_MACHINE(machine);
>>>>>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>>>>> that line should break linking on other targets which don't have
>>>>> pc_machine_get_reserved_memory_end()
>>>>> probably for got to add stub.
>>>>>
>>>>
>>>> I cross-compiled all the targets with no problem.  It seems no stub is required.
>>> ./configure && make
>>>
>>>     LINK  aarch64-softmmu/qemu-system-aarch64
>>> ../hw/pci/pci.o: In function `pci_bus_get_w64_range':
>>> /home/imammedo/builds/qemu/hw/pci/pci.c:2474: undefined reference to `pc_machine_get_reserved_memory_end'
>>> collect2: error: ld returned 1 exit status
>>
>> Ooops, I did configured it to cross-compile everything, but I somehow missed it.
>> I'll take care of it.
>>
>>
>>>
>>>>
>>>>
>>>>>> +        if (!range->begin) {
>>>>>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>>>>>> +                                    1ULL << 30);
>>>>>> +        }
>>> mayby move above hunk to pc_machine_get_reserved_memory_end()
>>> so it would always return valid value.
>>>
>>>>>> +        range->end = 1ULL << 40; /* 40 bits physical */
>>>>> x86 specific in generic code
>>>>>
>>>>
>>>> I am aware I put x86 code in the generic pci code, but I limited it with
>>>>        if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>>>> So we have a generic rule for getting the w64 range and a specific one for PC machines.
>>>>
>>>> The alternative would be a w64 range per host-bridge, not bus.
>>>> Adding a pci_bus_get_w64_range function to PCIHostBridgeClass,
>>>> defaulting in current implementation for the base class and
>>>> overriding it for piix/q35 looks OK to you?
>>>>
>>>> I thought about it, but it seemed over-engineered as opposed
>>>> to the 'simple' if statement, however I can try it if you think is better.
>>>>
>>>>> ARM also has 64-bit PCI MMIO /git grep VIRT_PCIE_MMIO_HIGH/
>>>>
>>>> I had a look and ARM does not use this infrastructure, it has its own abstractions,
>>>> a memmap list. This is the other reason I selected this implementation,
>>>> because is not really used by other targets (but it can be used in the future)
>>>
>>> if it's only for x86 and whatever was programmed by BIOS is ignored,
>>> I'd drop PCI_HOST_PROP_PCI_HOLE64_*/pci_bus_get_w64_range() indirection
>>> and just directly use pc_machine_get_reserved_memory_end()
>>> from acpi-build.c
>>>
>>> in that case you won't need a stub for pc_machine_get_reserved_memory_end()
>>> as its usage will be limited only to hw/i386 scope.
>>>
>>
>> Good point, I'll try this.
>>
>>> Given opportunity, I'd drop PCI_HOST_PROP_PCI_HOLE* properties,
>>> but for it we need ACK from libvirt side in case they are using it
>>> for some reason.
>>
>> It seems out of the scope of this series, however I can do it on top.
> it's just nice to have cleanup, but I don't insist on it.
>
> the thing here is that if pc_machine_get_reserved_memory_end() were used
> directly for acpi-build.c then properties as is would give old/different values.

You are right, I'll remove at least the 64-bit properties.

Thanks,
Marcel

>
>>
>> Thanks,
>> Marcel
>>
>>>
>>>>
>>>>
>>>> Thanks,
>>>> Marcel
>>>>
>>>>> perhaps range should be a property of PCI bus,
>>>>> where a board sets its own values for start/size
>>>>>
>>>>>> +    } else {
>>>>>> +        range->begin = range->end = 0;
>>>>>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>>>> +    }
>>>>>>     }
>>>>>>
>>>>>>     static bool pcie_has_upstream_port(PCIDevice *dev)
>>>>>
>>>>
>>>
>>
>

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Igor Mammedov]

On Sun, 15 May 2016 22:23:32 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> Using the firmware assigned MMIO ranges for 64-bit PCI window
> leads to zero space for hot-plugging PCI devices over 4G.
> 
> PC machines can use the whole CPU addressable range after
> the space reserved for memory-hotplug.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
that patch also has side effect of unconditionally adding
QWordMemory() resource in PCI0._CRS
on all machine types with QEMU generated ACPI tables.

Have you tested that it won't break boot of legacy OSes
(XP, WS2003, old linux with 32bit kernel)?

> ---
>  hw/pci/pci.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bb605ef..44dd949 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -41,6 +41,7 @@
>  #include "hw/hotplug.h"
>  #include "hw/boards.h"
>  #include "qemu/cutils.h"
> +#include "hw/i386/pc.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI
> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>  
>  void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>  {
> -    range->begin = range->end = 0;
> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> +    Object *machine = qdev_get_machine();
> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> +        PCMachineState *pcms = PC_MACHINE(machine);
> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
> +        if (!range->begin) {
> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> +                                    1ULL << 30);
> +        }
> +        range->end = 1ULL << 40; /* 40 bits physical */
> +    } else {
> +        range->begin = range->end = 0;
> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> +    }
>  }
>  
>  static bool pcie_has_upstream_port(PCIDevice *dev)

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Laszlo Ersek]

On 05/18/16 15:59, Igor Mammedov wrote:
> On Sun, 15 May 2016 22:23:32 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
> 
>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>> leads to zero space for hot-plugging PCI devices over 4G.
>>
>> PC machines can use the whole CPU addressable range after
>> the space reserved for memory-hotplug.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

> that patch also has side effect of unconditionally adding
> QWordMemory() resource in PCI0._CRS
> on all machine types with QEMU generated ACPI tables.
> 
> Have you tested that it won't break boot of legacy OSes
> (XP, WS2003, old linux with 32bit kernel)?

Ah, very good point. I recall that in my initial patch (which I meant
mostly as a discussion-starter :)) I paid attention to call
aml_qword_memory() only when unavoidable:

http://thread.gmane.org/gmane.comp.emulators.qemu/400375

Thanks
Laszlo

>> ---
>>  hw/pci/pci.c | 16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index bb605ef..44dd949 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -41,6 +41,7 @@
>>  #include "hw/hotplug.h"
>>  #include "hw/boards.h"
>>  #include "qemu/cutils.h"
>> +#include "hw/i386/pc.h"
>>  
>>  //#define DEBUG_PCI
>>  #ifdef DEBUG_PCI
>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>  
>>  void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>  {
>> -    range->begin = range->end = 0;
>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>> +    Object *machine = qdev_get_machine();
>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>> +        PCMachineState *pcms = PC_MACHINE(machine);
>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>> +        if (!range->begin) {
>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>> +                                    1ULL << 30);
>> +        }
>> +        range->end = 1ULL << 40; /* 40 bits physical */
>> +    } else {
>> +        range->begin = range->end = 0;
>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>> +    }
>>  }
>>  
>>  static bool pcie_has_upstream_port(PCIDevice *dev)
> 

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/18/2016 04:59 PM, Igor Mammedov wrote:
> On Sun, 15 May 2016 22:23:32 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>> leads to zero space for hot-plugging PCI devices over 4G.
>>
>> PC machines can use the whole CPU addressable range after
>> the space reserved for memory-hotplug.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> that patch also has side effect of unconditionally adding
> QWordMemory() resource in PCI0._CRS
> on all machine types with QEMU generated ACPI tables.

Right.

>
> Have you tested that it won't break boot of legacy OSes
> (XP, WS2003, old linux with 32bit kernel)?

I tested it with an old 32-bit CPU with a Linux guest and all
I received was a warning message that is not CPU addressable and will be discarded.
I didn't try it with WinXp though, I'll give it a try and report the issues, if any.

Thanks,
Marcel

>
>> ---
>>   hw/pci/pci.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index bb605ef..44dd949 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -41,6 +41,7 @@
>>   #include "hw/hotplug.h"
>>   #include "hw/boards.h"
>>   #include "qemu/cutils.h"
>> +#include "hw/i386/pc.h"
>>
>>   //#define DEBUG_PCI
>>   #ifdef DEBUG_PCI
>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>
>>   void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>   {
>> -    range->begin = range->end = 0;
>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>> +    Object *machine = qdev_get_machine();
>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>> +        PCMachineState *pcms = PC_MACHINE(machine);
>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>> +        if (!range->begin) {
>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>> +                                    1ULL << 30);
>> +        }
>> +        range->end = 1ULL << 40; /* 40 bits physical */
>> +    } else {
>> +        range->begin = range->end = 0;
>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>> +    }
>>   }
>>
>>   static bool pcie_has_upstream_port(PCIDevice *dev)
>

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 03:59:29PM +0200, Igor Mammedov wrote:
> On Sun, 15 May 2016 22:23:32 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
> 
> > Using the firmware assigned MMIO ranges for 64-bit PCI window
> > leads to zero space for hot-plugging PCI devices over 4G.
> > 
> > PC machines can use the whole CPU addressable range after
> > the space reserved for memory-hotplug.
> > 
> > Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> that patch also has side effect of unconditionally adding
> QWordMemory() resource in PCI0._CRS
> on all machine types with QEMU generated ACPI tables.
> 
> Have you tested that it won't break boot of legacy OSes
> (XP, WS2003, old linux with 32bit kernel)?

It's almost sure it break it.
Maybe you can check _REV in _CRS to work around this for XP.

> > ---
> >  hw/pci/pci.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index bb605ef..44dd949 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -41,6 +41,7 @@
> >  #include "hw/hotplug.h"
> >  #include "hw/boards.h"
> >  #include "qemu/cutils.h"
> > +#include "hw/i386/pc.h"
> >  
> >  //#define DEBUG_PCI
> >  #ifdef DEBUG_PCI
> > @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> >  
> >  void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >  {
> > -    range->begin = range->end = 0;
> > -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> > +    Object *machine = qdev_get_machine();
> > +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> > +        PCMachineState *pcms = PC_MACHINE(machine);
> > +        range->begin = pc_machine_get_reserved_memory_end(pcms);
> > +        if (!range->begin) {
> > +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> > +                                    1ULL << 30);
> > +        }
> > +        range->end = 1ULL << 40; /* 40 bits physical */
> > +    } else {
> > +        range->begin = range->end = 0;
> > +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> > +    }
> >  }
> >  
> >  static bool pcie_has_upstream_port(PCIDevice *dev)

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/18/2016 05:11 PM, Michael S. Tsirkin wrote:
> On Wed, May 18, 2016 at 03:59:29PM +0200, Igor Mammedov wrote:
>> On Sun, 15 May 2016 22:23:32 +0300
>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>
>>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>>> leads to zero space for hot-plugging PCI devices over 4G.
>>>
>>> PC machines can use the whole CPU addressable range after
>>> the space reserved for memory-hotplug.
>>>
>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> that patch also has side effect of unconditionally adding
>> QWordMemory() resource in PCI0._CRS
>> on all machine types with QEMU generated ACPI tables.
>>
>> Have you tested that it won't break boot of legacy OSes
>> (XP, WS2003, old linux with 32bit kernel)?
>
> It's almost sure it break it.
> Maybe you can check _REV in _CRS to work around this for XP.

I'll try it.

Thanks,
Marcel

>
>>> ---
>>>   hw/pci/pci.c | 16 ++++++++++++++--
>>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>> index bb605ef..44dd949 100644
>>> --- a/hw/pci/pci.c
>>> +++ b/hw/pci/pci.c
>>> @@ -41,6 +41,7 @@
>>>   #include "hw/hotplug.h"
>>>   #include "hw/boards.h"
>>>   #include "qemu/cutils.h"
>>> +#include "hw/i386/pc.h"
>>>
>>>   //#define DEBUG_PCI
>>>   #ifdef DEBUG_PCI
>>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>>
>>>   void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>   {
>>> -    range->begin = range->end = 0;
>>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>> +    Object *machine = qdev_get_machine();
>>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>>> +        PCMachineState *pcms = PC_MACHINE(machine);
>>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>>> +        if (!range->begin) {
>>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>>> +                                    1ULL << 30);
>>> +        }
>>> +        range->end = 1ULL << 40; /* 40 bits physical */
>>> +    } else {
>>> +        range->begin = range->end = 0;
>>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>> +    }
>>>   }
>>>
>>>   static bool pcie_has_upstream_port(PCIDevice *dev)

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Igor Mammedov]

On Wed, 18 May 2016 17:12:09 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 05/18/2016 05:11 PM, Michael S. Tsirkin wrote:
> > On Wed, May 18, 2016 at 03:59:29PM +0200, Igor Mammedov wrote:  
> >> On Sun, 15 May 2016 22:23:32 +0300
> >> Marcel Apfelbaum <marcel@redhat.com> wrote:
> >>  
> >>> Using the firmware assigned MMIO ranges for 64-bit PCI window
> >>> leads to zero space for hot-plugging PCI devices over 4G.
> >>>
> >>> PC machines can use the whole CPU addressable range after
> >>> the space reserved for memory-hotplug.
> >>>
> >>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>  
> >> that patch also has side effect of unconditionally adding
> >> QWordMemory() resource in PCI0._CRS
> >> on all machine types with QEMU generated ACPI tables.
> >>
> >> Have you tested that it won't break boot of legacy OSes
> >> (XP, WS2003, old linux with 32bit kernel)?  
> >
> > It's almost sure it break it.
> > Maybe you can check _REV in _CRS to work around this for XP.  
> 
> I'll try it.
but only after you check if just presence of QWord would crash XP,
so in case it doesn't crash we would keep _CRS simple static
structure.

I very vaguely recall that XP ignored QWord in PCI0._CRS,
but it was long time ago so it won't hurt to recheck.

> 
> Thanks,
> Marcel
> 
> >  
> >>> ---
> >>>   hw/pci/pci.c | 16 ++++++++++++++--
> >>>   1 file changed, 14 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>> index bb605ef..44dd949 100644
> >>> --- a/hw/pci/pci.c
> >>> +++ b/hw/pci/pci.c
> >>> @@ -41,6 +41,7 @@
> >>>   #include "hw/hotplug.h"
> >>>   #include "hw/boards.h"
> >>>   #include "qemu/cutils.h"
> >>> +#include "hw/i386/pc.h"
> >>>
> >>>   //#define DEBUG_PCI
> >>>   #ifdef DEBUG_PCI
> >>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> >>>
> >>>   void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >>>   {
> >>> -    range->begin = range->end = 0;
> >>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>> +    Object *machine = qdev_get_machine();
> >>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> >>> +        PCMachineState *pcms = PC_MACHINE(machine);
> >>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
> >>> +        if (!range->begin) {
> >>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> >>> +                                    1ULL << 30);
> >>> +        }
> >>> +        range->end = 1ULL << 40; /* 40 bits physical */
> >>> +    } else {
> >>> +        range->begin = range->end = 0;
> >>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>> +    }
> >>>   }
> >>>
> >>>   static bool pcie_has_upstream_port(PCIDevice *dev)  
> 

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/18/2016 05:31 PM, Igor Mammedov wrote:
> On Wed, 18 May 2016 17:12:09 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 05/18/2016 05:11 PM, Michael S. Tsirkin wrote:
>>> On Wed, May 18, 2016 at 03:59:29PM +0200, Igor Mammedov wrote:
>>>> On Sun, 15 May 2016 22:23:32 +0300
>>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>>
>>>>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>>>>> leads to zero space for hot-plugging PCI devices over 4G.
>>>>>
>>>>> PC machines can use the whole CPU addressable range after
>>>>> the space reserved for memory-hotplug.
>>>>>
>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> that patch also has side effect of unconditionally adding
>>>> QWordMemory() resource in PCI0._CRS
>>>> on all machine types with QEMU generated ACPI tables.
>>>>
>>>> Have you tested that it won't break boot of legacy OSes
>>>> (XP, WS2003, old linux with 32bit kernel)?
>>>
>>> It's almost sure it break it.
>>> Maybe you can check _REV in _CRS to work around this for XP.
>>
>> I'll try it.
> but only after you check if just presence of QWord would crash XP,
> so in case it doesn't crash we would keep _CRS simple static
> structure.
>
> I very vaguely recall that XP ignored QWord in PCI0._CRS,
> but it was long time ago so it won't hurt to recheck.

Thanks for the pointer!
Marcel

>
>>
>> Thanks,
>> Marcel
>>
>>>
>>>>> ---
>>>>>    hw/pci/pci.c | 16 ++++++++++++++--
>>>>>    1 file changed, 14 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>>> index bb605ef..44dd949 100644
>>>>> --- a/hw/pci/pci.c
>>>>> +++ b/hw/pci/pci.c
>>>>> @@ -41,6 +41,7 @@
>>>>>    #include "hw/hotplug.h"
>>>>>    #include "hw/boards.h"
>>>>>    #include "qemu/cutils.h"
>>>>> +#include "hw/i386/pc.h"
>>>>>
>>>>>    //#define DEBUG_PCI
>>>>>    #ifdef DEBUG_PCI
>>>>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>>>>
>>>>>    void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>>>    {
>>>>> -    range->begin = range->end = 0;
>>>>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>>> +    Object *machine = qdev_get_machine();
>>>>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>>>>> +        PCMachineState *pcms = PC_MACHINE(machine);
>>>>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>>>>> +        if (!range->begin) {
>>>>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>>>>> +                                    1ULL << 30);
>>>>> +        }
>>>>> +        range->end = 1ULL << 40; /* 40 bits physical */
>>>>> +    } else {
>>>>> +        range->begin = range->end = 0;
>>>>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>>> +    }
>>>>>    }
>>>>>
>>>>>    static bool pcie_has_upstream_port(PCIDevice *dev)
>>
>

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 04:31:46PM +0200, Igor Mammedov wrote:
> On Wed, 18 May 2016 17:12:09 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
> 
> > On 05/18/2016 05:11 PM, Michael S. Tsirkin wrote:
> > > On Wed, May 18, 2016 at 03:59:29PM +0200, Igor Mammedov wrote:  
> > >> On Sun, 15 May 2016 22:23:32 +0300
> > >> Marcel Apfelbaum <marcel@redhat.com> wrote:
> > >>  
> > >>> Using the firmware assigned MMIO ranges for 64-bit PCI window
> > >>> leads to zero space for hot-plugging PCI devices over 4G.
> > >>>
> > >>> PC machines can use the whole CPU addressable range after
> > >>> the space reserved for memory-hotplug.
> > >>>
> > >>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>  
> > >> that patch also has side effect of unconditionally adding
> > >> QWordMemory() resource in PCI0._CRS
> > >> on all machine types with QEMU generated ACPI tables.
> > >>
> > >> Have you tested that it won't break boot of legacy OSes
> > >> (XP, WS2003, old linux with 32bit kernel)?  
> > >
> > > It's almost sure it break it.
> > > Maybe you can check _REV in _CRS to work around this for XP.  
> > 
> > I'll try it.
> but only after you check if just presence of QWord would crash XP,
> so in case it doesn't crash we would keep _CRS simple static
> structure.
> 
> I very vaguely recall that XP ignored QWord in PCI0._CRS,
> but it was long time ago so it won't hurt to recheck.

I played with different guests (32 and 64 bit)
at some point.

Generally, windows tends to crash when CRS resources exceed the
supported limits
of physical memory (sometimes with weird off by one errors,
e.g. win7 32 bit seems to survive with a 36 bit pci hole even though
this means the max address is 2^37-1 which it can't address).

This might depend on CPU as well.

Which makes me ask: why don't we fix this in BIOS?
If you want it to allocate a large window, do it.

> > 
> > Thanks,
> > Marcel
> > 
> > >  
> > >>> ---
> > >>>   hw/pci/pci.c | 16 ++++++++++++++--
> > >>>   1 file changed, 14 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > >>> index bb605ef..44dd949 100644
> > >>> --- a/hw/pci/pci.c
> > >>> +++ b/hw/pci/pci.c
> > >>> @@ -41,6 +41,7 @@
> > >>>   #include "hw/hotplug.h"
> > >>>   #include "hw/boards.h"
> > >>>   #include "qemu/cutils.h"
> > >>> +#include "hw/i386/pc.h"
> > >>>
> > >>>   //#define DEBUG_PCI
> > >>>   #ifdef DEBUG_PCI
> > >>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> > >>>
> > >>>   void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> > >>>   {
> > >>> -    range->begin = range->end = 0;
> > >>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> > >>> +    Object *machine = qdev_get_machine();
> > >>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> > >>> +        PCMachineState *pcms = PC_MACHINE(machine);
> > >>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
> > >>> +        if (!range->begin) {
> > >>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> > >>> +                                    1ULL << 30);
> > >>> +        }
> > >>> +        range->end = 1ULL << 40; /* 40 bits physical */
> > >>> +    } else {
> > >>> +        range->begin = range->end = 0;
> > >>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> > >>> +    }
> > >>>   }
> > >>>
> > >>>   static bool pcie_has_upstream_port(PCIDevice *dev)  
> > 

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/18/2016 05:42 PM, Michael S. Tsirkin wrote:
> On Wed, May 18, 2016 at 04:31:46PM +0200, Igor Mammedov wrote:
>> On Wed, 18 May 2016 17:12:09 +0300
>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>
>>> On 05/18/2016 05:11 PM, Michael S. Tsirkin wrote:
>>>> On Wed, May 18, 2016 at 03:59:29PM +0200, Igor Mammedov wrote:
>>>>> On Sun, 15 May 2016 22:23:32 +0300
>>>>> Marcel Apfelbaum <marcel@redhat.com> wrote:
>>>>>
>>>>>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>>>>>> leads to zero space for hot-plugging PCI devices over 4G.
>>>>>>
>>>>>> PC machines can use the whole CPU addressable range after
>>>>>> the space reserved for memory-hotplug.
>>>>>>
>>>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>>> that patch also has side effect of unconditionally adding
>>>>> QWordMemory() resource in PCI0._CRS
>>>>> on all machine types with QEMU generated ACPI tables.
>>>>>
>>>>> Have you tested that it won't break boot of legacy OSes
>>>>> (XP, WS2003, old linux with 32bit kernel)?
>>>>
>>>> It's almost sure it break it.
>>>> Maybe you can check _REV in _CRS to work around this for XP.
>>>
>>> I'll try it.
>> but only after you check if just presence of QWord would crash XP,
>> so in case it doesn't crash we would keep _CRS simple static
>> structure.
>>
>> I very vaguely recall that XP ignored QWord in PCI0._CRS,
>> but it was long time ago so it won't hurt to recheck.
>
> I played with different guests (32 and 64 bit)
> at some point.
>
> Generally, windows tends to crash when CRS resources exceed the
> supported limits
> of physical memory (sometimes with weird off by one errors,
> e.g. win7 32 bit seems to survive with a 36 bit pci hole even though
> this means the max address is 2^37-1 which it can't address).
>
> This might depend on CPU as well.

It seems QEMU returns 40-bit as CPU addressable bits, but I might got it wrong.

>
> Which makes me ask: why don't we fix this in BIOS?
> If you want it to allocate a large window, do it.

I don't follow, BIOS can assign resources to PCI devices, but how to
specify a MMIO range for hot-plug? Add it to the CRS of the host-bridge, right?

Thanks,
Marcel

[...]

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 05:52:29PM +0300, Marcel Apfelbaum wrote:
> On 05/18/2016 05:42 PM, Michael S. Tsirkin wrote:
> >On Wed, May 18, 2016 at 04:31:46PM +0200, Igor Mammedov wrote:
> >>On Wed, 18 May 2016 17:12:09 +0300
> >>Marcel Apfelbaum <marcel@redhat.com> wrote:
> >>
> >>>On 05/18/2016 05:11 PM, Michael S. Tsirkin wrote:
> >>>>On Wed, May 18, 2016 at 03:59:29PM +0200, Igor Mammedov wrote:
> >>>>>On Sun, 15 May 2016 22:23:32 +0300
> >>>>>Marcel Apfelbaum <marcel@redhat.com> wrote:
> >>>>>
> >>>>>>Using the firmware assigned MMIO ranges for 64-bit PCI window
> >>>>>>leads to zero space for hot-plugging PCI devices over 4G.
> >>>>>>
> >>>>>>PC machines can use the whole CPU addressable range after
> >>>>>>the space reserved for memory-hotplug.
> >>>>>>
> >>>>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>>>that patch also has side effect of unconditionally adding
> >>>>>QWordMemory() resource in PCI0._CRS
> >>>>>on all machine types with QEMU generated ACPI tables.
> >>>>>
> >>>>>Have you tested that it won't break boot of legacy OSes
> >>>>>(XP, WS2003, old linux with 32bit kernel)?
> >>>>
> >>>>It's almost sure it break it.
> >>>>Maybe you can check _REV in _CRS to work around this for XP.
> >>>
> >>>I'll try it.
> >>but only after you check if just presence of QWord would crash XP,
> >>so in case it doesn't crash we would keep _CRS simple static
> >>structure.
> >>
> >>I very vaguely recall that XP ignored QWord in PCI0._CRS,
> >>but it was long time ago so it won't hurt to recheck.
> >
> >I played with different guests (32 and 64 bit)
> >at some point.
> >
> >Generally, windows tends to crash when CRS resources exceed the
> >supported limits
> >of physical memory (sometimes with weird off by one errors,
> >e.g. win7 32 bit seems to survive with a 36 bit pci hole even though
> >this means the max address is 2^37-1 which it can't address).
> >
> >This might depend on CPU as well.
> 
> It seems QEMU returns 40-bit as CPU addressable bits, but I might got it wrong.
> 
> >
> >Which makes me ask: why don't we fix this in BIOS?
> >If you want it to allocate a large window, do it.
> 
> I don't follow, BIOS can assign resources to PCI devices, but how to
> specify a MMIO range for hot-plug? Add it to the CRS of the host-bridge, right?
> 
> Thanks,
> Marcel
> 
> [...]

Ah I forgot on PC there's no register for this. On Q35 there is.

-- 
MST

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Michael S. Tsirkin]

On Sun, May 15, 2016 at 10:23:32PM +0300, Marcel Apfelbaum wrote:
> Using the firmware assigned MMIO ranges for 64-bit PCI window
> leads to zero space for hot-plugging PCI devices over 4G.
> 
> PC machines can use the whole CPU addressable range after
> the space reserved for memory-hotplug.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/pci/pci.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index bb605ef..44dd949 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -41,6 +41,7 @@
>  #include "hw/hotplug.h"
>  #include "hw/boards.h"
>  #include "qemu/cutils.h"
> +#include "hw/i386/pc.h"
>  
>  //#define DEBUG_PCI
>  #ifdef DEBUG_PCI

I don't want pci to depend on PC.
Pls find another way to do this.


> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>  
>  void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>  {
> -    range->begin = range->end = 0;
> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> +    Object *machine = qdev_get_machine();
An empty line won't hurt here after the declaration.

> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> +        PCMachineState *pcms = PC_MACHINE(machine);

An empty line won't hurt here after the declaration.

> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
> +        if (!range->begin) {
> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> +                                    1ULL << 30);

Why 30? what is the logic here?

> +        }
> +        range->end = 1ULL << 40; /* 40 bits physical */

This comment does not help. Physical what? And why is 40 bit right?

> +    } else {
> +        range->begin = range->end = 0;
> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);

When does this trigger?
Pls add a comment.

> +    }
>  }
>  
>  static bool pcie_has_upstream_port(PCIDevice *dev)
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/18/2016 05:14 PM, Michael S. Tsirkin wrote:
> On Sun, May 15, 2016 at 10:23:32PM +0300, Marcel Apfelbaum wrote:
>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>> leads to zero space for hot-plugging PCI devices over 4G.
>>
>> PC machines can use the whole CPU addressable range after
>> the space reserved for memory-hotplug.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/pci/pci.c | 16 ++++++++++++++--
>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index bb605ef..44dd949 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -41,6 +41,7 @@
>>   #include "hw/hotplug.h"
>>   #include "hw/boards.h"
>>   #include "qemu/cutils.h"
>> +#include "hw/i386/pc.h"
>>
>>   //#define DEBUG_PCI
>>   #ifdef DEBUG_PCI
>
> I don't want pci to depend on PC.
> Pls find another way to do this.
>

Igor has an idea to not call pci_dev_get_w64 and make the computations
in the acpi code. I'll follow this idea.

>
>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>
>>   void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>   {
>> -    range->begin = range->end = 0;
>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>> +    Object *machine = qdev_get_machine();
> An empty line won't hurt here after the declaration.
>
>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>> +        PCMachineState *pcms = PC_MACHINE(machine);
>
> An empty line won't hurt here after the declaration.
>
>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>> +        if (!range->begin) {
>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>> +                                    1ULL << 30);
>
> Why 30? what is the logic here?
>

Will put it inside pci_dev_get_w64 and explain.

>> +        }
>> +        range->end = 1ULL << 40; /* 40 bits physical */
>
> This comment does not help. Physical what? And why is 40 bit right?

It refers to how many bits are CPU addressable. (I will add a better comment)
cpu_x86_cpuid from target-i386/cpu.c it always returns 40
so hard-coding it looked like a safe choice.

Thanks,
Marcel

>> +    } else {
>> +        range->begin = range->end = 0;
>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>
> When does this trigger?
> Pls add a comment.
>
>> +    }
>>   }
>>
>>   static bool pcie_has_upstream_port(PCIDevice *dev)
>> --
>> 2.4.3

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 05:43:37PM +0300, Marcel Apfelbaum wrote:
> On 05/18/2016 05:14 PM, Michael S. Tsirkin wrote:
> >On Sun, May 15, 2016 at 10:23:32PM +0300, Marcel Apfelbaum wrote:
> >>Using the firmware assigned MMIO ranges for 64-bit PCI window
> >>leads to zero space for hot-plugging PCI devices over 4G.
> >>
> >>PC machines can use the whole CPU addressable range after
> >>the space reserved for memory-hotplug.
> >>
> >>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>---
> >>  hw/pci/pci.c | 16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>index bb605ef..44dd949 100644
> >>--- a/hw/pci/pci.c
> >>+++ b/hw/pci/pci.c
> >>@@ -41,6 +41,7 @@
> >>  #include "hw/hotplug.h"
> >>  #include "hw/boards.h"
> >>  #include "qemu/cutils.h"
> >>+#include "hw/i386/pc.h"
> >>
> >>  //#define DEBUG_PCI
> >>  #ifdef DEBUG_PCI
> >
> >I don't want pci to depend on PC.
> >Pls find another way to do this.
> >
> 
> Igor has an idea to not call pci_dev_get_w64 and make the computations
> in the acpi code. I'll follow this idea.
> 
> >
> >>@@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> >>
> >>  void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >>  {
> >>-    range->begin = range->end = 0;
> >>-    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>+    Object *machine = qdev_get_machine();
> >An empty line won't hurt here after the declaration.
> >
> >>+    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> >>+        PCMachineState *pcms = PC_MACHINE(machine);
> >
> >An empty line won't hurt here after the declaration.
> >
> >>+        range->begin = pc_machine_get_reserved_memory_end(pcms);
> >>+        if (!range->begin) {
> >>+            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> >>+                                    1ULL << 30);
> >
> >Why 30? what is the logic here?
> >
> 
> Will put it inside pci_dev_get_w64 and explain.
> 
> >>+        }
> >>+        range->end = 1ULL << 40; /* 40 bits physical */
> >
> >This comment does not help. Physical what? And why is 40 bit right?
> 
> It refers to how many bits are CPU addressable. (I will add a better comment)
> cpu_x86_cpuid from target-i386/cpu.c it always returns 40
> so hard-coding it looked like a safe choice.
> 
> Thanks,
> Marcel

Besides being ugly (we should get this from CPU code, not hard-code it)
not all guests look at CPUID unfortunately.
E.g. try win7 32 bit.


> >>+    } else {
> >>+        range->begin = range->end = 0;
> >>+        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >
> >When does this trigger?
> >Pls add a comment.
> >
> >>+    }
> >>  }
> >>
> >>  static bool pcie_has_upstream_port(PCIDevice *dev)
> >>--
> >>2.4.3

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Marcel Apfelbaum]

On 05/18/2016 05:57 PM, Michael S. Tsirkin wrote:
> On Wed, May 18, 2016 at 05:43:37PM +0300, Marcel Apfelbaum wrote:
>> On 05/18/2016 05:14 PM, Michael S. Tsirkin wrote:
>>> On Sun, May 15, 2016 at 10:23:32PM +0300, Marcel Apfelbaum wrote:
>>>> Using the firmware assigned MMIO ranges for 64-bit PCI window
>>>> leads to zero space for hot-plugging PCI devices over 4G.
>>>>
>>>> PC machines can use the whole CPU addressable range after
>>>> the space reserved for memory-hotplug.
>>>>
>>>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>>>> ---
>>>>   hw/pci/pci.c | 16 ++++++++++++++--
>>>>   1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>>>> index bb605ef..44dd949 100644
>>>> --- a/hw/pci/pci.c
>>>> +++ b/hw/pci/pci.c
>>>> @@ -41,6 +41,7 @@
>>>>   #include "hw/hotplug.h"
>>>>   #include "hw/boards.h"
>>>>   #include "qemu/cutils.h"
>>>> +#include "hw/i386/pc.h"
>>>>
>>>>   //#define DEBUG_PCI
>>>>   #ifdef DEBUG_PCI
>>>
>>> I don't want pci to depend on PC.
>>> Pls find another way to do this.
>>>
>>
>> Igor has an idea to not call pci_dev_get_w64 and make the computations
>> in the acpi code. I'll follow this idea.
>>
>>>
>>>> @@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
>>>>
>>>>   void pci_bus_get_w64_range(PCIBus *bus, Range *range)
>>>>   {
>>>> -    range->begin = range->end = 0;
>>>> -    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>> +    Object *machine = qdev_get_machine();
>>> An empty line won't hurt here after the declaration.
>>>
>>>> +    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
>>>> +        PCMachineState *pcms = PC_MACHINE(machine);
>>>
>>> An empty line won't hurt here after the declaration.
>>>
>>>> +        range->begin = pc_machine_get_reserved_memory_end(pcms);
>>>> +        if (!range->begin) {
>>>> +            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
>>>> +                                    1ULL << 30);
>>>
>>> Why 30? what is the logic here?
>>>
>>
>> Will put it inside pci_dev_get_w64 and explain.
>>
>>>> +        }
>>>> +        range->end = 1ULL << 40; /* 40 bits physical */
>>>
>>> This comment does not help. Physical what? And why is 40 bit right?
>>
>> It refers to how many bits are CPU addressable. (I will add a better comment)
>> cpu_x86_cpuid from target-i386/cpu.c it always returns 40
>> so hard-coding it looked like a safe choice.
>>
>> Thanks,
>> Marcel
>
> Besides being ugly (we should get this from CPU code, not hard-code it)
> not all guests look at CPUID unfortunately.
> E.g. try win7 32 bit.
>

I am open to better ideas, can you please advice?

Thanks,
Marcel

>
>>>> +    } else {
>>>> +        range->begin = range->end = 0;
>>>> +        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
>>>
>>> When does this trigger?
>>> Pls add a comment.
>>>
>>>> +    }
>>>>   }
>>>>
>>>>   static bool pcie_has_upstream_port(PCIDevice *dev)
>>>> --
>>>> 2.4.3

Re: [Qemu-devel] [PATCH V2 2/4] pci: reserve 64 bit MMIO range for PCI hotplug

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 06:01:15PM +0300, Marcel Apfelbaum wrote:
> On 05/18/2016 05:57 PM, Michael S. Tsirkin wrote:
> >On Wed, May 18, 2016 at 05:43:37PM +0300, Marcel Apfelbaum wrote:
> >>On 05/18/2016 05:14 PM, Michael S. Tsirkin wrote:
> >>>On Sun, May 15, 2016 at 10:23:32PM +0300, Marcel Apfelbaum wrote:
> >>>>Using the firmware assigned MMIO ranges for 64-bit PCI window
> >>>>leads to zero space for hot-plugging PCI devices over 4G.
> >>>>
> >>>>PC machines can use the whole CPU addressable range after
> >>>>the space reserved for memory-hotplug.
> >>>>
> >>>>Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> >>>>---
> >>>>  hw/pci/pci.c | 16 ++++++++++++++--
> >>>>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>>>
> >>>>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >>>>index bb605ef..44dd949 100644
> >>>>--- a/hw/pci/pci.c
> >>>>+++ b/hw/pci/pci.c
> >>>>@@ -41,6 +41,7 @@
> >>>>  #include "hw/hotplug.h"
> >>>>  #include "hw/boards.h"
> >>>>  #include "qemu/cutils.h"
> >>>>+#include "hw/i386/pc.h"
> >>>>
> >>>>  //#define DEBUG_PCI
> >>>>  #ifdef DEBUG_PCI
> >>>
> >>>I don't want pci to depend on PC.
> >>>Pls find another way to do this.
> >>>
> >>
> >>Igor has an idea to not call pci_dev_get_w64 and make the computations
> >>in the acpi code. I'll follow this idea.
> >>
> >>>
> >>>>@@ -2467,8 +2468,19 @@ static void pci_dev_get_w64(PCIBus *b, PCIDevice *dev, void *opaque)
> >>>>
> >>>>  void pci_bus_get_w64_range(PCIBus *bus, Range *range)
> >>>>  {
> >>>>-    range->begin = range->end = 0;
> >>>>-    pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>>>+    Object *machine = qdev_get_machine();
> >>>An empty line won't hurt here after the declaration.
> >>>
> >>>>+    if (object_dynamic_cast(machine, TYPE_PC_MACHINE)) {
> >>>>+        PCMachineState *pcms = PC_MACHINE(machine);
> >>>
> >>>An empty line won't hurt here after the declaration.
> >>>
> >>>>+        range->begin = pc_machine_get_reserved_memory_end(pcms);
> >>>>+        if (!range->begin) {
> >>>>+            range->begin = ROUND_UP(0x100000000ULL + pcms->above_4g_mem_size,
> >>>>+                                    1ULL << 30);
> >>>
> >>>Why 30? what is the logic here?
> >>>
> >>
> >>Will put it inside pci_dev_get_w64 and explain.
> >>
> >>>>+        }
> >>>>+        range->end = 1ULL << 40; /* 40 bits physical */
> >>>
> >>>This comment does not help. Physical what? And why is 40 bit right?
> >>
> >>It refers to how many bits are CPU addressable. (I will add a better comment)
> >>cpu_x86_cpuid from target-i386/cpu.c it always returns 40
> >>so hard-coding it looked like a safe choice.
> >>
> >>Thanks,
> >>Marcel
> >
> >Besides being ugly (we should get this from CPU code, not hard-code it)
> >not all guests look at CPUID unfortunately.
> >E.g. try win7 32 bit.
> >
> 
> I am open to better ideas, can you please advice?
> 
> Thanks,
> Marcel

I'm afraid you will have to try a ton of guests and collect 
data on which window ranges make them crash :(

> >
> >>>>+    } else {
> >>>>+        range->begin = range->end = 0;
> >>>>+        pci_for_each_device_under_bus(bus, pci_dev_get_w64, range);
> >>>
> >>>When does this trigger?
> >>>Pls add a comment.
> >>>
> >>>>+    }
> >>>>  }
> >>>>
> >>>>  static bool pcie_has_upstream_port(PCIDevice *dev)
> >>>>--
> >>>>2.4.3

[Qemu-devel] [PATCH V2 3/4] acpi: refactor pxb crs computation

[Marcel Apfelbaum]

Instead of always passing both IO and MEM ranges when
computing CRS ranges, define a new CrsRangeSet structure
that include them both.

This is done before introducing a third type of range,
64-bit MEM, so it will be easier to pass them all around.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c | 82 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 51 insertions(+), 31 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 279f0d7..78f25ef 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -736,6 +736,23 @@ static void crs_range_free(gpointer data)
     g_free(entry);
 }
 
+typedef struct CrsRangeSet {
+    GPtrArray *io_ranges;
+    GPtrArray *mem_ranges;
+ } CrsRangeSet;
+
+static void crs_range_set_init(CrsRangeSet *range_set)
+{
+    range_set->io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+    range_set->mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+}
+
+static void crs_range_set_free(CrsRangeSet *range_set)
+{
+    g_ptr_array_free(range_set->io_ranges, true);
+    g_ptr_array_free(range_set->mem_ranges, true);
+}
+
 static gint crs_range_compare(gconstpointer a, gconstpointer b)
 {
      CrsRangeEntry *entry_a = *(CrsRangeEntry **)a;
@@ -820,18 +837,17 @@ static void crs_range_merge(GPtrArray *range)
     g_ptr_array_free(tmp, true);
 }
 
-static Aml *build_crs(PCIHostState *host,
-                      GPtrArray *io_ranges, GPtrArray *mem_ranges)
+static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
 {
     Aml *crs = aml_resource_template();
-    GPtrArray *host_io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
-    GPtrArray *host_mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+    CrsRangeSet temp_range_set;
     CrsRangeEntry *entry;
     uint8_t max_bus = pci_bus_num(host->bus);
     uint8_t type;
     int devfn;
     int i;
 
+    crs_range_set_init(&temp_range_set);
     for (devfn = 0; devfn < ARRAY_SIZE(host->bus->devices); devfn++) {
         uint64_t range_base, range_limit;
         PCIDevice *dev = host->bus->devices[devfn];
@@ -855,9 +871,11 @@ static Aml *build_crs(PCIHostState *host,
             }
 
             if (r->type & PCI_BASE_ADDRESS_SPACE_IO) {
-                crs_range_insert(host_io_ranges, range_base, range_limit);
+                crs_range_insert(temp_range_set.io_ranges,
+                                 range_base, range_limit);
             } else { /* "memory" */
-                crs_range_insert(host_mem_ranges, range_base, range_limit);
+                crs_range_insert(temp_range_set.mem_ranges,
+                                 range_base, range_limit);
             }
         }
 
@@ -876,7 +894,8 @@ static Aml *build_crs(PCIHostState *host,
              * that do not support multiple root buses
              */
             if (range_base && range_base <= range_limit) {
-                crs_range_insert(host_io_ranges, range_base, range_limit);
+                crs_range_insert(temp_range_set.io_ranges,
+                                 range_base, range_limit);
             }
 
             range_base =
@@ -889,7 +908,8 @@ static Aml *build_crs(PCIHostState *host,
              * that do not support multiple root buses
              */
             if (range_base && range_base <= range_limit) {
-                crs_range_insert(host_mem_ranges, range_base, range_limit);
+                crs_range_insert(temp_range_set.mem_ranges,
+                                 range_base, range_limit);
             }
 
             range_base =
@@ -902,35 +922,36 @@ static Aml *build_crs(PCIHostState *host,
              * that do not support multiple root buses
              */
             if (range_base && range_base <= range_limit) {
-                crs_range_insert(host_mem_ranges, range_base, range_limit);
+                crs_range_insert(temp_range_set.mem_ranges,
+                                 range_base, range_limit);
             }
         }
     }
 
-    crs_range_merge(host_io_ranges);
-    for (i = 0; i < host_io_ranges->len; i++) {
-        entry = g_ptr_array_index(host_io_ranges, i);
+    crs_range_merge(temp_range_set.io_ranges);
+    for (i = 0; i < temp_range_set.io_ranges->len; i++) {
+        entry = g_ptr_array_index(temp_range_set.io_ranges, i);
         aml_append(crs,
                    aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
                                AML_POS_DECODE, AML_ENTIRE_RANGE,
                                0, entry->base, entry->limit, 0,
                                entry->limit - entry->base + 1));
-        crs_range_insert(io_ranges, entry->base, entry->limit);
+        crs_range_insert(range_set->io_ranges, entry->base, entry->limit);
     }
-    g_ptr_array_free(host_io_ranges, true);
 
-    crs_range_merge(host_mem_ranges);
-    for (i = 0; i < host_mem_ranges->len; i++) {
-        entry = g_ptr_array_index(host_mem_ranges, i);
+    crs_range_merge(temp_range_set.mem_ranges);
+    for (i = 0; i < temp_range_set.mem_ranges->len; i++) {
+        entry = g_ptr_array_index(temp_range_set.mem_ranges, i);
         aml_append(crs,
                    aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED,
                                     AML_MAX_FIXED, AML_NON_CACHEABLE,
                                     AML_READ_WRITE,
                                     0, entry->base, entry->limit, 0,
                                     entry->limit - entry->base + 1));
-        crs_range_insert(mem_ranges, entry->base, entry->limit);
+        crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
     }
-    g_ptr_array_free(host_mem_ranges, true);
+
+    crs_range_set_free(&temp_range_set);
 
     aml_append(crs,
         aml_word_bus_number(AML_MIN_FIXED, AML_MAX_FIXED, AML_POS_DECODE,
@@ -1985,8 +2006,7 @@ build_dsdt(GArray *table_data, GArray *linker,
 {
     CrsRangeEntry *entry;
     Aml *dsdt, *sb_scope, *scope, *dev, *method, *field, *pkg, *crs;
-    GPtrArray *mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
-    GPtrArray *io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+    CrsRangeSet crs_range_set;
     PCMachineState *pcms = PC_MACHINE(machine);
     uint32_t nr_mem = machine->ram_slots;
     int root_bus_limit = 0xFF;
@@ -2087,6 +2107,7 @@ build_dsdt(GArray *table_data, GArray *linker,
     }
     aml_append(dsdt, scope);
 
+    crs_range_set_init(&crs_range_set);
     bus = PC_MACHINE(machine)->bus;
     if (bus) {
         QLIST_FOREACH(bus, &bus->child, sibling) {
@@ -2113,8 +2134,7 @@ build_dsdt(GArray *table_data, GArray *linker,
             }
 
             aml_append(dev, build_prt(false));
-            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent),
-                            io_ranges, mem_ranges);
+            crs = build_crs(PCI_HOST_BRIDGE(BUS(bus)->parent), &crs_range_set);
             aml_append(dev, aml_name_decl("_CRS", crs));
             aml_append(scope, dev);
             aml_append(dsdt, scope);
@@ -2135,9 +2155,9 @@ build_dsdt(GArray *table_data, GArray *linker,
                     AML_POS_DECODE, AML_ENTIRE_RANGE,
                     0x0000, 0x0000, 0x0CF7, 0x0000, 0x0CF8));
 
-    crs_replace_with_free_ranges(io_ranges, 0x0D00, 0xFFFF);
-    for (i = 0; i < io_ranges->len; i++) {
-        entry = g_ptr_array_index(io_ranges, i);
+    crs_replace_with_free_ranges(crs_range_set.io_ranges, 0x0D00, 0xFFFF);
+    for (i = 0; i < crs_range_set.io_ranges->len; i++) {
+        entry = g_ptr_array_index(crs_range_set.io_ranges, i);
         aml_append(crs,
             aml_word_io(AML_MIN_FIXED, AML_MAX_FIXED,
                         AML_POS_DECODE, AML_ENTIRE_RANGE,
@@ -2150,9 +2170,10 @@ build_dsdt(GArray *table_data, GArray *linker,
                          AML_CACHEABLE, AML_READ_WRITE,
                          0, 0x000A0000, 0x000BFFFF, 0, 0x00020000));
 
-    crs_replace_with_free_ranges(mem_ranges, pci->w32.begin, pci->w32.end - 1);
-    for (i = 0; i < mem_ranges->len; i++) {
-        entry = g_ptr_array_index(mem_ranges, i);
+    crs_replace_with_free_ranges(crs_range_set.mem_ranges,
+                                 pci->w32.begin, pci->w32.end - 1);
+    for (i = 0; i < crs_range_set. mem_ranges->len; i++) {
+        entry = g_ptr_array_index(crs_range_set.mem_ranges, i);
         aml_append(crs,
             aml_dword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
                              AML_NON_CACHEABLE, AML_READ_WRITE,
@@ -2187,8 +2208,7 @@ build_dsdt(GArray *table_data, GArray *linker,
     aml_append(dev, aml_name_decl("_CRS", crs));
     aml_append(scope, dev);
 
-    g_ptr_array_free(io_ranges, true);
-    g_ptr_array_free(mem_ranges, true);
+    crs_range_set_free(&crs_range_set);
 
     /* reserve PCIHP resources */
     if (pm->pcihp_io_len) {
-- 
2.4.3

[Qemu-devel] [PATCH V2 4/4] hw/apci: handle 64-bit MMIO regions correctly

[Marcel Apfelbaum]

In build_crs(), the calculation and merging of the ranges already happens
in 64-bit, but the entry boundaries are silently truncated to 32-bit in the
call to aml_dword_memory(). Fix it by handling the 64-bit MMIO ranges separately.
This fixes 64-bit BARs behind PXBs.

Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
---
 hw/i386/acpi-build.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 9 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 78f25ef..aaf4a34 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -739,18 +739,22 @@ static void crs_range_free(gpointer data)
 typedef struct CrsRangeSet {
     GPtrArray *io_ranges;
     GPtrArray *mem_ranges;
+    GPtrArray *mem_64bit_ranges;
  } CrsRangeSet;
 
 static void crs_range_set_init(CrsRangeSet *range_set)
 {
     range_set->io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
     range_set->mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
+    range_set->mem_64bit_ranges =
+            g_ptr_array_new_with_free_func(crs_range_free);
 }
 
 static void crs_range_set_free(CrsRangeSet *range_set)
 {
     g_ptr_array_free(range_set->io_ranges, true);
     g_ptr_array_free(range_set->mem_ranges, true);
+    g_ptr_array_free(range_set->mem_64bit_ranges, true);
 }
 
 static gint crs_range_compare(gconstpointer a, gconstpointer b)
@@ -908,8 +912,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
              * that do not support multiple root buses
              */
             if (range_base && range_base <= range_limit) {
-                crs_range_insert(temp_range_set.mem_ranges,
-                                 range_base, range_limit);
+                uint64_t length = range_limit - range_base + 1;
+                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
+                    crs_range_insert(temp_range_set.mem_ranges,
+                                     range_base, range_limit);
+                } else {
+                    crs_range_insert(temp_range_set.mem_64bit_ranges,
+                                     range_base, range_limit);
+                }
             }
 
             range_base =
@@ -922,8 +932,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
              * that do not support multiple root buses
              */
             if (range_base && range_base <= range_limit) {
-                crs_range_insert(temp_range_set.mem_ranges,
-                                 range_base, range_limit);
+                uint64_t length = range_limit - range_base + 1;
+                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
+                    crs_range_insert(temp_range_set.mem_ranges,
+                                     range_base, range_limit);
+                } else {
+                    crs_range_insert(temp_range_set.mem_64bit_ranges,
+                                     range_base, range_limit);
+                }
             }
         }
     }
@@ -951,6 +967,19 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
         crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
     }
 
+    crs_range_merge(temp_range_set.mem_64bit_ranges);
+    for (i = 0; i < temp_range_set.mem_64bit_ranges->len; i++) {
+        entry = g_ptr_array_index(temp_range_set.mem_64bit_ranges, i);
+        aml_append(crs,
+                   aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+                                    AML_MAX_FIXED, AML_NON_CACHEABLE,
+                                    AML_READ_WRITE,
+                                    0, entry->base, entry->limit, 0,
+                                    entry->limit - entry->base + 1));
+        crs_range_insert(range_set->mem_64bit_ranges,
+                         entry->base, entry->limit);
+    }
+
     crs_range_set_free(&temp_range_set);
 
     aml_append(crs,
@@ -2182,11 +2211,17 @@ build_dsdt(GArray *table_data, GArray *linker,
     }
 
     if (pci->w64.begin) {
-        aml_append(crs,
-            aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
-                             AML_CACHEABLE, AML_READ_WRITE,
-                             0, pci->w64.begin, pci->w64.end - 1, 0,
-                             pci->w64.end - pci->w64.begin));
+        crs_replace_with_free_ranges(crs_range_set.mem_64bit_ranges,
+                                     pci->w64.begin, pci->w64.end - 1);
+        for (i = 0; i < crs_range_set.mem_64bit_ranges->len; i++) {
+            entry = g_ptr_array_index(crs_range_set.mem_64bit_ranges, i);
+            aml_append(crs,
+                       aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
+                                        AML_MAX_FIXED,
+                                        AML_CACHEABLE, AML_READ_WRITE,
+                                        0, entry->base, entry->limit,
+                                        0, entry->limit - entry->base + 1));
+        }
     }
 
     if (misc->tpm_version != TPM_VERSION_UNSPEC) {
-- 
2.4.3

Re: [Qemu-devel] [PATCH V2 4/4] hw/apci: handle 64-bit MMIO regions correctly

[Igor Mammedov]

On Sun, 15 May 2016 22:23:34 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> In build_crs(), the calculation and merging of the ranges already happens
> in 64-bit, but the entry boundaries are silently truncated to 32-bit in the
> call to aml_dword_memory(). Fix it by handling the 64-bit MMIO ranges separately.
> This fixes 64-bit BARs behind PXBs.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
>  hw/i386/acpi-build.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 78f25ef..aaf4a34 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -739,18 +739,22 @@ static void crs_range_free(gpointer data)
>  typedef struct CrsRangeSet {
>      GPtrArray *io_ranges;
>      GPtrArray *mem_ranges;
> +    GPtrArray *mem_64bit_ranges;
>   } CrsRangeSet;
>  
>  static void crs_range_set_init(CrsRangeSet *range_set)
>  {
>      range_set->io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
>      range_set->mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
> +    range_set->mem_64bit_ranges =
> +            g_ptr_array_new_with_free_func(crs_range_free);
>  }
>  
>  static void crs_range_set_free(CrsRangeSet *range_set)
>  {
>      g_ptr_array_free(range_set->io_ranges, true);
>      g_ptr_array_free(range_set->mem_ranges, true);
> +    g_ptr_array_free(range_set->mem_64bit_ranges, true);
>  }
>  
>  static gint crs_range_compare(gconstpointer a, gconstpointer b)
> @@ -908,8 +912,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>               * that do not support multiple root buses
>               */
>              if (range_base && range_base <= range_limit) {
> -                crs_range_insert(temp_range_set.mem_ranges,
> -                                 range_base, range_limit);
> +                uint64_t length = range_limit - range_base + 1;
> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
> +                    crs_range_insert(temp_range_set.mem_ranges,
> +                                     range_base, range_limit);
> +                } else {
> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
> +                                     range_base, range_limit);
> +                }
>              }
>  
>              range_base =
> @@ -922,8 +932,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>               * that do not support multiple root buses
>               */
>              if (range_base && range_base <= range_limit) {
> -                crs_range_insert(temp_range_set.mem_ranges,
> -                                 range_base, range_limit);
> +                uint64_t length = range_limit - range_base + 1;
> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
> +                    crs_range_insert(temp_range_set.mem_ranges,
> +                                     range_base, range_limit);
> +                } else {
> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
> +                                     range_base, range_limit);
> +                }
>              }
>          }
>      }
> @@ -951,6 +967,19 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>          crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
>      }
>  
> +    crs_range_merge(temp_range_set.mem_64bit_ranges);
> +    for (i = 0; i < temp_range_set.mem_64bit_ranges->len; i++) {
> +        entry = g_ptr_array_index(temp_range_set.mem_64bit_ranges, i);
> +        aml_append(crs,
> +                   aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> +                                    AML_MAX_FIXED, AML_NON_CACHEABLE,
> +                                    AML_READ_WRITE,
> +                                    0, entry->base, entry->limit, 0,
> +                                    entry->limit - entry->base + 1));
> +        crs_range_insert(range_set->mem_64bit_ranges,
> +                         entry->base, entry->limit);
> +    }
instead of adding another field, wouldn't following be a simpler approach:

for (i = 0; i < temp_range_set.mem_ranges->len; i++) {
    entry = g_ptr_array_index(temp_range_set.mem_ranges, i);
    if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
       aml_dword_memory(...);
    } else {
       aml_qword_memory(...);
    }


> +
>      crs_range_set_free(&temp_range_set);
>  
>      aml_append(crs,
> @@ -2182,11 +2211,17 @@ build_dsdt(GArray *table_data, GArray *linker,
>      }
>  
>      if (pci->w64.begin) {
> -        aml_append(crs,
> -            aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
> -                             AML_CACHEABLE, AML_READ_WRITE,
> -                             0, pci->w64.begin, pci->w64.end - 1, 0,
> -                             pci->w64.end - pci->w64.begin));
> +        crs_replace_with_free_ranges(crs_range_set.mem_64bit_ranges,
> +                                     pci->w64.begin, pci->w64.end - 1);
> +        for (i = 0; i < crs_range_set.mem_64bit_ranges->len; i++) {
> +            entry = g_ptr_array_index(crs_range_set.mem_64bit_ranges, i);
> +            aml_append(crs,
> +                       aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> +                                        AML_MAX_FIXED,
> +                                        AML_CACHEABLE, AML_READ_WRITE,
> +                                        0, entry->base, entry->limit,
> +                                        0, entry->limit - entry->base + 1));
> +        }
>      }
>  
>      if (misc->tpm_version != TPM_VERSION_UNSPEC) {

Re: [Qemu-devel] [PATCH V2 4/4] hw/apci: handle 64-bit MMIO regions correctly

[Marcel Apfelbaum]

On 05/16/2016 02:19 PM, Igor Mammedov wrote:
> On Sun, 15 May 2016 22:23:34 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> In build_crs(), the calculation and merging of the ranges already happens
>> in 64-bit, but the entry boundaries are silently truncated to 32-bit in the
>> call to aml_dword_memory(). Fix it by handling the 64-bit MMIO ranges separately.
>> This fixes 64-bit BARs behind PXBs.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>> ---
>>   hw/i386/acpi-build.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 44 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 78f25ef..aaf4a34 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -739,18 +739,22 @@ static void crs_range_free(gpointer data)
>>   typedef struct CrsRangeSet {
>>       GPtrArray *io_ranges;
>>       GPtrArray *mem_ranges;
>> +    GPtrArray *mem_64bit_ranges;
>>    } CrsRangeSet;
>>
>>   static void crs_range_set_init(CrsRangeSet *range_set)
>>   {
>>       range_set->io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
>>       range_set->mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
>> +    range_set->mem_64bit_ranges =
>> +            g_ptr_array_new_with_free_func(crs_range_free);
>>   }
>>
>>   static void crs_range_set_free(CrsRangeSet *range_set)
>>   {
>>       g_ptr_array_free(range_set->io_ranges, true);
>>       g_ptr_array_free(range_set->mem_ranges, true);
>> +    g_ptr_array_free(range_set->mem_64bit_ranges, true);
>>   }
>>
>>   static gint crs_range_compare(gconstpointer a, gconstpointer b)
>> @@ -908,8 +912,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>                * that do not support multiple root buses
>>                */
>>               if (range_base && range_base <= range_limit) {
>> -                crs_range_insert(temp_range_set.mem_ranges,
>> -                                 range_base, range_limit);
>> +                uint64_t length = range_limit - range_base + 1;
>> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
>> +                    crs_range_insert(temp_range_set.mem_ranges,
>> +                                     range_base, range_limit);
>> +                } else {
>> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
>> +                                     range_base, range_limit);
>> +                }
>>               }
>>
>>               range_base =
>> @@ -922,8 +932,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>                * that do not support multiple root buses
>>                */
>>               if (range_base && range_base <= range_limit) {
>> -                crs_range_insert(temp_range_set.mem_ranges,
>> -                                 range_base, range_limit);
>> +                uint64_t length = range_limit - range_base + 1;
>> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
>> +                    crs_range_insert(temp_range_set.mem_ranges,
>> +                                     range_base, range_limit);
>> +                } else {
>> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
>> +                                     range_base, range_limit);
>> +                }
>>               }
>>           }
>>       }
>> @@ -951,6 +967,19 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>           crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
>>       }
>>
>> +    crs_range_merge(temp_range_set.mem_64bit_ranges);
>> +    for (i = 0; i < temp_range_set.mem_64bit_ranges->len; i++) {
>> +        entry = g_ptr_array_index(temp_range_set.mem_64bit_ranges, i);
>> +        aml_append(crs,
>> +                   aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>> +                                    AML_MAX_FIXED, AML_NON_CACHEABLE,
>> +                                    AML_READ_WRITE,
>> +                                    0, entry->base, entry->limit, 0,
>> +                                    entry->limit - entry->base + 1));
>> +        crs_range_insert(range_set->mem_64bit_ranges,
>> +                         entry->base, entry->limit);
>> +    }
> instead of adding another field, wouldn't following be a simpler approach:
>

This would be enough if we only need to create the ACPI table entries.
However we need the mem_64bit_ranges field (I assume you are talking about it)
for "accounting":
We need to extract 'free' ranges given the used one for MMIO and 64-BIT MMIO
separately, for each of them  we will supply a different PCI Window.

The approach I chose is easier to understand IMO:
We have IO, MMIO and 64-MMIO ranges. For each of them we have a different window, but
we apply the same algorithm. Unifying MMIO and 64-MMIO will not give us a real
advantage, as the code we suppress for building the ACPI tables will become
twice as long when we do the 'accounting'. (see: crs_replace_with_free_ranges)

Thanks,
Marcel


> for (i = 0; i < temp_range_set.mem_ranges->len; i++) {
>      entry = g_ptr_array_index(temp_range_set.mem_ranges, i);
>      if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
>         aml_dword_memory(...);
>      } else {
>         aml_qword_memory(...);
>      }
>
>
>> +
>>       crs_range_set_free(&temp_range_set);
>>
>>       aml_append(crs,
>> @@ -2182,11 +2211,17 @@ build_dsdt(GArray *table_data, GArray *linker,
>>       }
>>
>>       if (pci->w64.begin) {
>> -        aml_append(crs,
>> -            aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
>> -                             AML_CACHEABLE, AML_READ_WRITE,
>> -                             0, pci->w64.begin, pci->w64.end - 1, 0,
>> -                             pci->w64.end - pci->w64.begin));
>> +        crs_replace_with_free_ranges(crs_range_set.mem_64bit_ranges,
>> +                                     pci->w64.begin, pci->w64.end - 1);
>> +        for (i = 0; i < crs_range_set.mem_64bit_ranges->len; i++) {
>> +            entry = g_ptr_array_index(crs_range_set.mem_64bit_ranges, i);
>> +            aml_append(crs,
>> +                       aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>> +                                        AML_MAX_FIXED,
>> +                                        AML_CACHEABLE, AML_READ_WRITE,
>> +                                        0, entry->base, entry->limit,
>> +                                        0, entry->limit - entry->base + 1));
>> +        }
>>       }
>>
>>       if (misc->tpm_version != TPM_VERSION_UNSPEC) {
>

Re: [Qemu-devel] [PATCH V2 4/4] hw/apci: handle 64-bit MMIO regions correctly

[Michael S. Tsirkin]

On Sun, May 15, 2016 at 10:23:34PM +0300, Marcel Apfelbaum wrote:
> In build_crs(), the calculation and merging of the ranges already happens
> in 64-bit, but the entry boundaries are silently truncated to 32-bit in the
> call to aml_dword_memory(). Fix it by handling the 64-bit MMIO ranges separately.
> This fixes 64-bit BARs behind PXBs.
> 
> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>

Can this be based on master? Looks like  bugfix useful in
stable.

> ---
>  hw/i386/acpi-build.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 78f25ef..aaf4a34 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -739,18 +739,22 @@ static void crs_range_free(gpointer data)
>  typedef struct CrsRangeSet {
>      GPtrArray *io_ranges;
>      GPtrArray *mem_ranges;
> +    GPtrArray *mem_64bit_ranges;
>   } CrsRangeSet;
>  
>  static void crs_range_set_init(CrsRangeSet *range_set)
>  {
>      range_set->io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
>      range_set->mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
> +    range_set->mem_64bit_ranges =
> +            g_ptr_array_new_with_free_func(crs_range_free);
>  }
>  
>  static void crs_range_set_free(CrsRangeSet *range_set)
>  {
>      g_ptr_array_free(range_set->io_ranges, true);
>      g_ptr_array_free(range_set->mem_ranges, true);
> +    g_ptr_array_free(range_set->mem_64bit_ranges, true);
>  }
>  
>  static gint crs_range_compare(gconstpointer a, gconstpointer b)
> @@ -908,8 +912,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>               * that do not support multiple root buses
>               */
>              if (range_base && range_base <= range_limit) {
> -                crs_range_insert(temp_range_set.mem_ranges,
> -                                 range_base, range_limit);
> +                uint64_t length = range_limit - range_base + 1;
> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
> +                    crs_range_insert(temp_range_set.mem_ranges,
> +                                     range_base, range_limit);
> +                } else {
> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
> +                                     range_base, range_limit);
> +                }
>              }
>  
>              range_base =
> @@ -922,8 +932,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>               * that do not support multiple root buses
>               */
>              if (range_base && range_base <= range_limit) {
> -                crs_range_insert(temp_range_set.mem_ranges,
> -                                 range_base, range_limit);
> +                uint64_t length = range_limit - range_base + 1;
> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
> +                    crs_range_insert(temp_range_set.mem_ranges,
> +                                     range_base, range_limit);
> +                } else {
> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
> +                                     range_base, range_limit);
> +                }
>              }
>          }
>      }
> @@ -951,6 +967,19 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>          crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
>      }
>  
> +    crs_range_merge(temp_range_set.mem_64bit_ranges);
> +    for (i = 0; i < temp_range_set.mem_64bit_ranges->len; i++) {
> +        entry = g_ptr_array_index(temp_range_set.mem_64bit_ranges, i);
> +        aml_append(crs,
> +                   aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> +                                    AML_MAX_FIXED, AML_NON_CACHEABLE,
> +                                    AML_READ_WRITE,
> +                                    0, entry->base, entry->limit, 0,
> +                                    entry->limit - entry->base + 1));
> +        crs_range_insert(range_set->mem_64bit_ranges,
> +                         entry->base, entry->limit);
> +    }
> +
>      crs_range_set_free(&temp_range_set);
>  
>      aml_append(crs,
> @@ -2182,11 +2211,17 @@ build_dsdt(GArray *table_data, GArray *linker,
>      }
>  
>      if (pci->w64.begin) {
> -        aml_append(crs,
> -            aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
> -                             AML_CACHEABLE, AML_READ_WRITE,
> -                             0, pci->w64.begin, pci->w64.end - 1, 0,
> -                             pci->w64.end - pci->w64.begin));
> +        crs_replace_with_free_ranges(crs_range_set.mem_64bit_ranges,
> +                                     pci->w64.begin, pci->w64.end - 1);
> +        for (i = 0; i < crs_range_set.mem_64bit_ranges->len; i++) {
> +            entry = g_ptr_array_index(crs_range_set.mem_64bit_ranges, i);
> +            aml_append(crs,
> +                       aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
> +                                        AML_MAX_FIXED,
> +                                        AML_CACHEABLE, AML_READ_WRITE,
> +                                        0, entry->base, entry->limit,
> +                                        0, entry->limit - entry->base + 1));
> +        }
>      }
>  
>      if (misc->tpm_version != TPM_VERSION_UNSPEC) {
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH V2 4/4] hw/apci: handle 64-bit MMIO regions correctly

[Marcel Apfelbaum]

On 05/18/2016 05:16 PM, Michael S. Tsirkin wrote:
> On Sun, May 15, 2016 at 10:23:34PM +0300, Marcel Apfelbaum wrote:
>> In build_crs(), the calculation and merging of the ranges already happens
>> in 64-bit, but the entry boundaries are silently truncated to 32-bit in the
>> call to aml_dword_memory(). Fix it by handling the 64-bit MMIO ranges separately.
>> This fixes 64-bit BARs behind PXBs.
>>
>> Signed-off-by: Marcel Apfelbaum <marcel@redhat.com>
>
> Can this be based on master? Looks like  bugfix useful in
> stable.

You need patch 3/4 and you should be able to apply this on master.

Also I did not test it without a valid 64bit MMIO range. Let me please
test it and switch the patches in the series so it will start with 3/4 and 4/4.

Thanks,
Marel

>
>> ---
>>   hw/i386/acpi-build.c | 53 +++++++++++++++++++++++++++++++++++++++++++---------
>>   1 file changed, 44 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 78f25ef..aaf4a34 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -739,18 +739,22 @@ static void crs_range_free(gpointer data)
>>   typedef struct CrsRangeSet {
>>       GPtrArray *io_ranges;
>>       GPtrArray *mem_ranges;
>> +    GPtrArray *mem_64bit_ranges;
>>    } CrsRangeSet;
>>
>>   static void crs_range_set_init(CrsRangeSet *range_set)
>>   {
>>       range_set->io_ranges = g_ptr_array_new_with_free_func(crs_range_free);
>>       range_set->mem_ranges = g_ptr_array_new_with_free_func(crs_range_free);
>> +    range_set->mem_64bit_ranges =
>> +            g_ptr_array_new_with_free_func(crs_range_free);
>>   }
>>
>>   static void crs_range_set_free(CrsRangeSet *range_set)
>>   {
>>       g_ptr_array_free(range_set->io_ranges, true);
>>       g_ptr_array_free(range_set->mem_ranges, true);
>> +    g_ptr_array_free(range_set->mem_64bit_ranges, true);
>>   }
>>
>>   static gint crs_range_compare(gconstpointer a, gconstpointer b)
>> @@ -908,8 +912,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>                * that do not support multiple root buses
>>                */
>>               if (range_base && range_base <= range_limit) {
>> -                crs_range_insert(temp_range_set.mem_ranges,
>> -                                 range_base, range_limit);
>> +                uint64_t length = range_limit - range_base + 1;
>> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
>> +                    crs_range_insert(temp_range_set.mem_ranges,
>> +                                     range_base, range_limit);
>> +                } else {
>> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
>> +                                     range_base, range_limit);
>> +                }
>>               }
>>
>>               range_base =
>> @@ -922,8 +932,14 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>                * that do not support multiple root buses
>>                */
>>               if (range_base && range_base <= range_limit) {
>> -                crs_range_insert(temp_range_set.mem_ranges,
>> -                                 range_base, range_limit);
>> +                uint64_t length = range_limit - range_base + 1;
>> +                if (range_limit <= UINT32_MAX && length <= UINT32_MAX) {
>> +                    crs_range_insert(temp_range_set.mem_ranges,
>> +                                     range_base, range_limit);
>> +                } else {
>> +                    crs_range_insert(temp_range_set.mem_64bit_ranges,
>> +                                     range_base, range_limit);
>> +                }
>>               }
>>           }
>>       }
>> @@ -951,6 +967,19 @@ static Aml *build_crs(PCIHostState *host, CrsRangeSet *range_set)
>>           crs_range_insert(range_set->mem_ranges, entry->base, entry->limit);
>>       }
>>
>> +    crs_range_merge(temp_range_set.mem_64bit_ranges);
>> +    for (i = 0; i < temp_range_set.mem_64bit_ranges->len; i++) {
>> +        entry = g_ptr_array_index(temp_range_set.mem_64bit_ranges, i);
>> +        aml_append(crs,
>> +                   aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>> +                                    AML_MAX_FIXED, AML_NON_CACHEABLE,
>> +                                    AML_READ_WRITE,
>> +                                    0, entry->base, entry->limit, 0,
>> +                                    entry->limit - entry->base + 1));
>> +        crs_range_insert(range_set->mem_64bit_ranges,
>> +                         entry->base, entry->limit);
>> +    }
>> +
>>       crs_range_set_free(&temp_range_set);
>>
>>       aml_append(crs,
>> @@ -2182,11 +2211,17 @@ build_dsdt(GArray *table_data, GArray *linker,
>>       }
>>
>>       if (pci->w64.begin) {
>> -        aml_append(crs,
>> -            aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED, AML_MAX_FIXED,
>> -                             AML_CACHEABLE, AML_READ_WRITE,
>> -                             0, pci->w64.begin, pci->w64.end - 1, 0,
>> -                             pci->w64.end - pci->w64.begin));
>> +        crs_replace_with_free_ranges(crs_range_set.mem_64bit_ranges,
>> +                                     pci->w64.begin, pci->w64.end - 1);
>> +        for (i = 0; i < crs_range_set.mem_64bit_ranges->len; i++) {
>> +            entry = g_ptr_array_index(crs_range_set.mem_64bit_ranges, i);
>> +            aml_append(crs,
>> +                       aml_qword_memory(AML_POS_DECODE, AML_MIN_FIXED,
>> +                                        AML_MAX_FIXED,
>> +                                        AML_CACHEABLE, AML_READ_WRITE,
>> +                                        0, entry->base, entry->limit,
>> +                                        0, entry->limit - entry->base + 1));
>> +        }
>>       }
>>
>>       if (misc->tpm_version != TPM_VERSION_UNSPEC) {
>> --
>> 2.4.3

Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Igor Mammedov]

On Sun, 15 May 2016 22:23:30 +0300
Marcel Apfelbaum <marcel@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

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(-)
> 

Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Michael S. Tsirkin]

On Wed, May 18, 2016 at 03:53:08PM +0200, Igor Mammedov wrote:
> On Sun, 15 May 2016 22:23:30 +0300
> Marcel Apfelbaum <marcel@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.

> 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(-)
> > 

Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Igor Mammedov]

On Wed, 18 May 2016 17:09:20 +0300
"Michael S. Tsirkin" <mst@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 <marcel@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"/

> 
> > 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(-)
> > >   
> 

Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Michael S. Tsirkin]

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" <mst@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 <marcel@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(-)
> > > >   
> > 

Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Igor Mammedov]

On Wed, 18 May 2016 17:44:57 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> 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" <mst@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 <marcel@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.
it should work in this case as it would fall back to non 'variant'
expected table.
Fail only happens if there isn't ANY expected table with
requested name.

So I use it to check the changes, I've made myself.

> 
> > >   
> > > > 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(-)
> > > > >     
> > >   

Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Marcel Apfelbaum]

On 05/18/2016 04:53 PM, Igor Mammedov wrote:
> On Sun, 15 May 2016 22:23:30 +0300
> Marcel Apfelbaum <marcel@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
>
> 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.
>
I'll add the test, thanks, and also I'll look for the PXB warning.

>
> 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()
>

Their PRT are not exactly the same, please see build_prt in hw/i386/acpi-build.c .

Thanks,
Marcel

>> 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(-)
>>
>

Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Igor Mammedov]

On Wed, 18 May 2016 17:22:43 +0300
Marcel Apfelbaum <marcel@redhat.com> wrote:

> On 05/18/2016 04:53 PM, Igor Mammedov wrote:
> > On Sun, 15 May 2016 22:23:30 +0300
> > Marcel Apfelbaum <marcel@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
> >
> > 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.
> >  
> I'll add the test, thanks, and also I'll look for the PXB warning.
> 
> >
> > 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()
> >  
> 
> Their PRT are not exactly the same, please see build_prt in hw/i386/acpi-build.c .
build_prt() can generate shared AML if is_pci0_prt
would be checked in AML, something like this:

@@@ -667,13 -684,13 +667,13 @@@ static Aml *build_prt(bool is_pci0_prt
  
          /* route[2] = "LNK[D|A|B|C]", selection based on pin % 3  */
          aml_append(while_ctx, initialize_route(route, "LNKD", lnk_idx, 0));
--        if (is_pci0_prt) {
++        {
              Aml *if_device_1, *if_pin_4, *else_pin_4;
  
              /* device 1 is the power-management device, needs SCI */
              if_device_1 = aml_if(aml_equal(lnk_idx, aml_int(1)));
              {
--                if_pin_4 = aml_if(aml_equal(pin, aml_int(4)));
++                if_pin_4 = aml_if(aml_and(aml_equal(pin, aml_int(4)), is_pci0_prt, NULL));
                  {
                      aml_append(if_pin_4,
                          aml_store(build_prt_entry("LNKS"), route));
@@@ -687,8 -704,8 +687,6 @@@
                  aml_append(if_device_1, else_pin_4);
              }
              aml_append(while_ctx, if_device_1);
--        } else {
--            aml_append(while_ctx, initialize_route(route, "LNKA", lnk_idx, 1));
          }
          aml_append(while_ctx, initialize_route(route, "LNKB", lnk_idx, 2));
          aml_append(while_ctx, initialize_route(route, "LNKC", lnk_idx, 3));


as is_pci0_prt key, it's probably possible to use _UID == 0 condition
or make a wrapper method per bus that explicitly will tell
common_prt() if it's root bus or not.

> 
> Thanks,
> Marcel
> 
> >> 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(-)
> >>  
> >  
> 
> 

Re: [Qemu-devel] [PATCH V2 0/4] pci: better support for 64-bit MMIO allocation

[Marcel Apfelbaum]

On 05/19/2016 12:04 PM, Igor Mammedov wrote:
> On Wed, 18 May 2016 17:22:43 +0300
> Marcel Apfelbaum <marcel@redhat.com> wrote:
>
>> On 05/18/2016 04:53 PM, Igor Mammedov wrote:
>>> On Sun, 15 May 2016 22:23:30 +0300
>>> Marcel Apfelbaum <marcel@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
>>>
>>> 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.
>>>
>> I'll add the test, thanks, and also I'll look for the PXB warning.
>>
>>>
>>> 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()
>>>
>>
>> Their PRT are not exactly the same, please see build_prt in hw/i386/acpi-build.c .
> build_prt() can generate shared AML if is_pci0_prt
> would be checked in AML, something like this:
>
> @@@ -667,13 -684,13 +667,13 @@@ static Aml *build_prt(bool is_pci0_prt
>
>            /* route[2] = "LNK[D|A|B|C]", selection based on pin % 3  */
>            aml_append(while_ctx, initialize_route(route, "LNKD", lnk_idx, 0));
> --        if (is_pci0_prt) {
> ++        {
>                Aml *if_device_1, *if_pin_4, *else_pin_4;
>
>                /* device 1 is the power-management device, needs SCI */
>                if_device_1 = aml_if(aml_equal(lnk_idx, aml_int(1)));
>                {
> --                if_pin_4 = aml_if(aml_equal(pin, aml_int(4)));
> ++                if_pin_4 = aml_if(aml_and(aml_equal(pin, aml_int(4)), is_pci0_prt, NULL));
>                    {
>                        aml_append(if_pin_4,
>                            aml_store(build_prt_entry("LNKS"), route));
> @@@ -687,8 -704,8 +687,6 @@@
>                    aml_append(if_device_1, else_pin_4);
>                }
>                aml_append(while_ctx, if_device_1);
> --        } else {
> --            aml_append(while_ctx, initialize_route(route, "LNKA", lnk_idx, 1));
>            }
>            aml_append(while_ctx, initialize_route(route, "LNKB", lnk_idx, 2));
>            aml_append(while_ctx, initialize_route(route, "LNKC", lnk_idx, 3));
>
>
> as is_pci0_prt key, it's probably possible to use _UID == 0 condition
> or make a wrapper method per bus that explicitly will tell
> common_prt() if it's root bus or not.
>

Thanks, I'll give a try (separate patch, of course)
Marcel

>>
>> Thanks,
>> Marcel
>>
>>>> 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(-)
>>>>
>>>
>>
>>
>