From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50522) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xe2vm-0000Mp-4f for qemu-devel@nongnu.org; Tue, 14 Oct 2014 10:18:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xe2vh-0000GQ-3m for qemu-devel@nongnu.org; Tue, 14 Oct 2014 10:18:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:63100) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xe2vg-0000GJ-Rx for qemu-devel@nongnu.org; Tue, 14 Oct 2014 10:18:25 -0400 Date: Tue, 14 Oct 2014 17:21:50 +0300 From: "Michael S. Tsirkin" Message-ID: <20141014142150.GD12439@redhat.com> References: <1413284654-13420-1-git-send-email-arei.gonglei@huawei.com> <20141014112034.GA9470@redhat.com> <543D0BDA.7070801@huawei.com> <20141014115825.GA10920@redhat.com> <543D14EC.4030602@huawei.com> <20141014122822.GB12439@redhat.com> <543D16AD.4070703@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <543D16AD.4070703@huawei.com> Subject: Re: [Qemu-devel] [PATCH] pci: do not pci_update_mappings when guest gets bar size List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: ChenLiang Cc: arei.gonglei@huawei.com, weidong.huang@huawei.com, qemu-devel@nongnu.org On Tue, Oct 14, 2014 at 08:27:25PM +0800, ChenLiang wrote: > 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 OK, I see. Thus the fix is to make memory core aware of the holes at identity base and TSS base. Maybe create an MR to cover this range. > > > >>> > >>> > >>> > >>>> 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 > >>>>>> > >>>>>> 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 > >>>>>> Signed-off-by: Gonglei > >>>>> > >>>>> 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 > >>>>>> > >>>>> > >>>>> . > >>>>> > >>>> > >>>> > >>> > >>> . > >>> > >> > >> > > > > . > > > >