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

[ChenLiang]

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

> On Tue, Oct 14, 2014 at 08:19:56PM +0800, ChenLiang wrote:
>> On 2014/10/14 19:58, 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.
>>>>
>>>> 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.
>>>
>>>
>>> In fact, ever since
>>>     83d08f2673504a299194dcac1657a13754b5932a
>>>     pc: map PCI address space as catchall region for not mapped addresses
>>>
>>> all pci memory has lower priority than ioapic at 0xfe0000000.
>>>
>>> so ioapic will win, there should be no issue.
>>>
>>> IOW this is not the root cause.
>>
>>
>> TSS_PRIVATE_MEMSLOT and IDENTITY_PAGETABLE_PRIVATE_MEMSLOT also will overlap.
> 
> I'm sorry, I can't find these symbols in qemu source.


These symbols is in kmod source.

Best regards
chenliang

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