Re: [Xen-devel] [PATCH v4 5/8] hvmloader: Correct bug in low mmio region accounting

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

George Dunlap writes ("[PATCH v4 5/8] hvmloader: Correct bug in low mmio region 
accounting"):
> When deciding whether to map a device in low MMIO space (<4GiB),
> hvmloader compares it with "mmio_left", which is set to the size of
> the low MMIO range (pci_mem_end - pci_mem_start).  However, even if it
> does map a device in high MMIO space, it still removes the size of its
> BAR from mmio_left.
> 
> In reality we don't need to do a separate accounting of the low memory
> available -- this can be calculated from mem_resource.  Just get rid
> of the variable and the duplicate accounting entirely.  This will make
> the code more robust.
...
> -        using_64bar = bars[i].is_64bar && bar64_relocate && (mmio_left < 
> bar_sz);
> +        using_64bar = bars[i].is_64bar && bar64_relocate
> +            && (bar_sz > (mem_resource.max - mem_resource.base));

This is not entirely straightforward I think.

The actual calculation about whether it will actually fit, rather than
a precalculation of whether it is going to fit, is done here:

        base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
        bar_data |= (uint32_t)base;
        bar_data_upper = (uint32_t)(base >> 32);
        base += bar_sz;

        if ( (base < resource->base) || (base > resource->max) )
            [ ... doesn't fit ... ]

The first test rounds the base up to a multiple of bar_sz.  I assume
that this is a requirement of the PCI spec.

(While I'm here I'll note that the (uint64_t) cast in that line is
unneccessary and confusing.  If bar_sz weren't 64-bit this code would
be quite wrong, and putting that cast there suggests that it might not
be.)

In infer (from "bar_sz &= ~(bar_sz - 1)") that bar_sz is supposed to
be always a power of two.  And we have devices in descending order of
size.  So at least after the first device, this rounding does nothing.

But for the first device I think it may be possible for resource->base
not to be a multiple of the bar_sz, and in that case it might be that
the precalculation thinks it will fit when the actual placement
calculation doesn't.

Do you think this is possible ?

This is certainly excessively confusing.  From a lack-of-regressions
point of view we are going to have to analyse it properly regardless
of whether we restructure it or not.

I would be tempted to suggest lifting the "base" etc. calculation into
a macro or function so that we can directly say

  +        using_64bar = bars[i].is_64bar && bar64_relocate
  +            && !try_allocate_resource(&mem_resource, &allocd, &new_base)

and later

  -       base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
  -       bar_data |= (uint32_t)base;
  -       bar_data_upper = (uint32_t)(base >> 32);
  -       base += bar_sz;
  -
  +       if ( !try_allocate_resource(resource, &allocd, &new_base) )
          {
              printf("pci dev %02x:%x bar %02x size "PRIllx": no space for "
                     "resource!\n", devfn>>3, devfn&7, bar_reg,
                     PRIllx_arg(bar_sz));
              continue;
          }

  -       resource->base = base;
  +       resource->base = new_base;
  +       bar_data |= (uint32_t)allocd;
  +       bar_data_upper = (uint32_t)(allocd >> 32);

or something.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel