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

[ChenLiang]

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

> On Tue, Oct 14, 2014 at 08:15:10PM +0800, ChenLiang wrote:
>> On 2014/10/14 19:48, Michael S. Tsirkin wrote:
>>
>>> On Tue, Oct 14, 2014 at 07:41:14PM +0800, ChenLiang wrote:
>>>> 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.
>>>
>>> Of course but it should not cause a crash.
>>> If you need the overlapping memslot available during the programming
>>> process, increase it's priority.
>>>
>>
>> Yeah, I know the priority of memory region.
>> The problem is overlaping should not happen when one pci bar is not
>> overlap with any other memslots. But Qemu always do pci_update_mappings
>> when guest os writes pci bar. Actually, should not do pci_update_mappings
>> if var is 0xffffffff.
> 
> Unfortunately your hack is not robust, so we can not include it.
> PCI devices should support arbitrary addresses.
> For example, if a device is programmed with an address
> 0xfe000000 then this is exactly the address it should claim.
> So I am sorry, you will have to either debug the problem to understand what
> is causing a crash, or tell us on the list how to reproduce it
> so others on the list can debug it.
> 
> 

For example:
guest os: suse10sp1
device: ivshmem 32MB
kmod: not include patch "KVM: Fix user memslot overlap check"

qemu will exit when vm start.

But guest os suse 11 sp2 will be ok.

Because the old linux kernel don't disable response in Memory space when it 
gets the size of pci bar.

ps: I am not sure whether "KVM: Fix user memslot overlap check" skip check 
overlaping with private memslot is safe.

The root cause is when guest write 0xffffffff to get size of pci bar. We should 
not do pci_update_mappings in pci_default_write_config.

>>>> 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
>>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>> .
>>>
>>
>>
> 
> .
>