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


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