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

["Michael S. Tsirkin" <mst@redhat.com>]

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.



> On 2014/10/14 19:20, Michael S. Tsirkin wrote:
> 
> > On Tue, Oct 14, 2014 at 07:04:14PM +0800, arei.gonglei@huawei.com wrote:
> >> From: ChenLiang <chenliang88@huawei.com>
> >>
> >> 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 <chenliang88@huawei.com>
> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> > 
> > 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
> >>
> > 
> > .
> > 
> 
>