Re: [Qemu-devel] [PATCH] RFC: delay pci_update_mappings for 64-bit BARs

[Isaku Yamahata <yamahata@valinux.co.jp>]

On Mon, Dec 13, 2010 at 03:43:44PM -0700, Cam Macdonell wrote:
> Do not call pci_update_mappings on the lower 32-bits of a 64-bit bar.  Wait for the upper 32 or else Qemu will try to map on just the lower 32 which is probably going to corrupt memory.
> 
> I was encountering crashes when mapping certain PCI region sizes.  The problem turns out that pci_update_mappings is being called without all 64-bits in the BAR.  For example when mapping to 0x180000000, once the lower 32-bits were written the remapping happened (mapping to 0x8000000) which would overwrite something.
> 
> I'm not certain if this is completely correct, I'm simply testing the lower 4-bits to only be MEM_TYPE_64 flag.  Upper 32-bit address parts can be values like 0xff which is tricky to test against.

You're assuming that guest OS always write lower 32bit and them upper 32bit.
Is the assumption correct?
I found Linux does, but I don't know about other OSes.
And I couldn't find any sentence about how to update (64bit) BAR in the specs.
(Please correct me if I missed it)

Some work around would be necessary regardless of 32bit-or-64bit.
because qemu doesn't emulate bus accurately at the moment.
How about the followings?
If BAR overlaps with RAM, don't map BAR.
If BAR overlaps with other BARs, record the overlapping and
when updating one of the BARs, update all the overlapping BARs.
Which BAR wins depends on the order of updating, it doesn't matter because
it's anomaly case.

This way, 32bit BAR case is also covered.

thanks,

> 
> Cam
> ---
>  hw/pci.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index 438c0d1..3b81792 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1000,6 +1000,9 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>  {
>      int i, was_irq_disabled = pci_irq_disabled(d);
>      uint32_t config_size = pci_config_size(d);
> +    int is_64 = 0;
> +
> +    is_64 = ((val & 0xf) == PCI_BASE_ADDRESS_MEM_TYPE_64);
>  
>      for (i = 0; i < l && addr + i < config_size; val >>= 8, ++i) {
>          uint8_t wmask = d->wmask[addr + i];
> @@ -1008,7 +1011,7 @@ void pci_default_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int l)
>          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) && (!is_64)) ||
>          ranges_overlap(addr, l, PCI_ROM_ADDRESS, 4) ||
>          ranges_overlap(addr, l, PCI_ROM_ADDRESS1, 4) ||
>          range_covers_byte(addr, l, PCI_COMMAND))
> -- 
> 1.7.0.4
> 
> 

-- 
yamahata