Re: [PATCH v2 1/5] hvmloader: Correct bug in low mmio region accounting

[George Dunlap <george.dunlap@eu.citrix.com>]

On 20/06/13 11:40, Stefano Stabellini wrote:
> On Thu, 20 Jun 2013, George Dunlap wrote:
>> On 19/06/13 18:18, Stefano Stabellini wrote:
>>> On Tue, 18 Jun 2013, George Dunlap wrote:
>>>> 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.
>>>>
>>>> This patch first changes the name of this variable to "low_mmio_left"
>>>> to distinguish it from generic MMIO, and corrects the logic to only
>>>> subtract the size of the BAR for devices maped in the low MMIO region.
>>>>
>>>> Also make low_mmio_left unsigned, and don't allow it to go negative.
>>>> Since its main use is to be compared to a 64-bit unsigned int, this
>>>> may have undefined (and in practice almost certainly incorrect)
>>>> results.  Not subtracting is OK because if there's not enough room, it
>>>> won't actually be mapped.
>>>>
>>>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>>>> CC: Ian Jackson <ian.jackson@citrix.com>
>>>> CC: Ian Campbell <ian.campbell@citrix.com>
>>>> CC: Stefano Stabellini <stefano.stabellini@citrix.com>
>>>> CC: Hanweidong <hanweidong@huawei.com>
>>>> ---
>>>>    tools/firmware/hvmloader/pci.c |   10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tools/firmware/hvmloader/pci.c
>>>> b/tools/firmware/hvmloader/pci.c
>>>> index c78d4d3..8691a19 100644
>>>> --- a/tools/firmware/hvmloader/pci.c
>>>> +++ b/tools/firmware/hvmloader/pci.c
>>>> @@ -38,11 +38,10 @@ void pci_setup(void)
>>>>    {
>>>>        uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>>>>        uint32_t devfn, bar_reg, cmd, bar_data, bar_data_upper;
>>>> -    uint64_t base, bar_sz, bar_sz_upper, mmio_total = 0;
>>>> +    uint64_t base, bar_sz, bar_sz_upper, low_mmio_left, mmio_total = 0;
>>>>        uint32_t vga_devfn = 256;
>>>>        uint16_t class, vendor_id, device_id;
>>>>        unsigned int bar, pin, link, isa_irq;
>>>> -    int64_t mmio_left;
>>>>          /* Resources assignable to PCI devices via BARs. */
>>>>        struct resource {
>>>> @@ -244,7 +243,7 @@ void pci_setup(void)
>>>>        io_resource.base = 0xc000;
>>>>        io_resource.max = 0x10000;
>>>>    -    mmio_left = pci_mem_end - pci_mem_start;
>>>> +    low_mmio_left = pci_mem_end - pci_mem_start;
>>>>          /* Assign iomem and ioport resources in descending order of size.
>>>> */
>>>>        for ( i = 0; i < nr_bars; i++ )
>>>> @@ -253,7 +252,7 @@ void pci_setup(void)
>>>>            bar_reg = bars[i].bar_reg;
>>>>            bar_sz  = bars[i].bar_sz;
>>>>    -        using_64bar = bars[i].is_64bar && bar64_relocate && (mmio_left
>>>> < bar_sz);
>>>> +        using_64bar = bars[i].is_64bar && bar64_relocate &&
>>>> (low_mmio_left < bar_sz);
>>>>            bar_data = pci_readl(devfn, bar_reg);
>>>>              if ( (bar_data & PCI_BASE_ADDRESS_SPACE) ==
>>>> @@ -273,9 +272,10 @@ void pci_setup(void)
>>>>                }
>>>>                else {
>>>>                    resource = &mem_resource;
>>>> +                if ( bar_sz <= low_mmio_left )
>>>> +                    low_mmio_left -= bar_sz;
>>> Why do you need this check? Isn't the above if(using_64bar && (bar_sz >
>>> PCI_MIN_BIG_BAR_SIZE)) enough?
>> This is in the lowmem region.  There may be regions which can't be relocated
>> to the high PCI region that nevertheless don't fit in the low PCI region.  If
>> it doesn't fit, it will hit the "no space for resource" conditional below and
>> not be mapped; we need to make sure not to subtract it off.
>>
>> I suppose a more robust method might be to use resource->max - resource->base
>> instead of keeping a separate accounting... I had originally thought that
>> would be too invasive a change, but I'm not so sure now... any thoughts?
> You could just add:
>
> if (resource == &mem_resource)
>      low_mmio_left -= bar_sz;
>
> right below the resource size check. This way we would have only one
> check to see if the bar fits.

Actually I just changed v3 to rid of low_mmio_left altogether, and just 
use "mem_resource.max - mem_resource.base" for the one and only time the 
value is needed.

  -George