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

[Michael S. Tsirkin]

On Tue, Oct 14, 2014 at 08:23:08PM +0800, Gonglei wrote:
> On 2014/10/14 20:15, chenliang (T) 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.
> > 
> 
> The PCI spec says [Page 222]:
> Decode (I/O or memory) of a register is disabled via the command register 
> before sizing a
> Base Address register. Software saves the original value of the Base Address 
> register,
> writes 0FFFFFFFFh to the register, then reads it back.

Yes. Unfortunately many guests ignore the spec, they
size an enabled BAR and hope for the best.
This typically works on real hardware and should work
within a VM as well.



> >>> 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?
> 
> ivshmem device with 32M' bar2 size.
> 
> Best regards,
> -Gonglei
> 
> >>>>
> >>>>
> >>>>> ---
> >>>>>  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
> >>>>>
> >>>>
> >>>> .
> >>>>
> >>>
> >>>
> >>
> >> .
> >>
> > 
> > 
> > 
> 
>