Re: [Qemu-stable] [PATCH] pci: do not pci_update_mappings when guest gets bar size

[ChenLiang]

We find overlap when the size of pci bar is bigger then 16MB, it overlaps with 
private
memslot in the kmod. By the way, the new kmod skip private memslot. But I think 
if the size
of pci bar is enough big, it also  overlaps with other memslots.

the root cause is:

pci_default_write_config will do that:
    for (i = 0; i < l; val >>= 8, ++i) {
        uint8_t wmask = d->wmask[addr + i];
        uint8_t w1cmask = d->w1cmask[addr + i];
        assert(!(wmask & w1cmask));
        d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & wmask);
        d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
    }

*(int*)(d->config[addr]) will be 0xfe00000c, if val is 0xffffffff and the size 
of bar is 32MB.
This range overlap with private memslot in the old kmod.

then pci_update_mappings will update memslot.

On 2014/10/14 19:20, Michael S. Tsirkin wrote:

> On Tue, Oct 14, 2014 at 07:04:14PM +0800, address@hidden wrote:
>> From: ChenLiang <address@hidden>
>>
>> Power-up software can determine how much address space the device
>> requires by writing a value of all 1's to the register and then
>> reading the value back(PCI specification). Qemu should not do
>> pci_update_mappings. Qemu may exit, because the wrong address of
>> this bar is overlap with other memslots.
>>
>> Signed-off-by: ChenLiang <address@hidden>
>> Signed-off-by: Gonglei <address@hidden>
> 
> This is at best a work-around.
> Overlapping is observed in practice, qemu really shouldn't exit when
> this happens.
> So we should find the root cause and fix it there instead of
> adding work-arounds in PCI core.
> 
> With which device do you observe this?
> 
> 
>> ---
>>  hw/pci/pci.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index 6ce75aa..4d44b44 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -1158,12 +1158,12 @@ void pci_default_write_config(PCIDevice *d, uint32_t 
>> addr, uint32_t val_in, int
>>          d->config[addr + i] = (d->config[addr + i] & ~wmask) | (val & 
>> wmask);
>>          d->config[addr + i] &= ~(val & w1cmask); /* W1C: Write 1 to Clear */
>>      }
>> -    if (ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
>> +    if (((ranges_overlap(addr, l, PCI_BASE_ADDRESS_0, 24) ||
>>          ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
>> -        ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4) ||
>> -        range_covers_byte(addr, l, PCI_COMMAND))
>> +        ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4)) &&
>> +        val_in != 0xffffffff) || range_covers_byte(addr, l, PCI_COMMAND)) {
>>          pci_update_mappings(d);
>> -
>> +    }
>>      if (range_covers_byte(addr, l, PCI_COMMAND)) {
>>          pci_update_irq_disabled(d, was_irq_disabled);
>>          memory_region_set_enabled(&d->bus_master_enable_region,
>> -- 
>> 1.7.12.4
>>
> 
> .
>