Re: [v7][RFC][PATCH 05/13] hvmloader/mmio: reconcile guest mmio with reserved device memory

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2014/10/24 22:42, Jan Beulich wrote:
>>>> On 24.10.14 at 09:34, <tiejun.chen@intel.com> wrote:
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -37,6 +37,44 @@ uint64_t pci_hi_mem_start = 0, pci_hi_mem_end = 0;
>>   enum virtual_vga virtual_vga = VGA_none;
>>   unsigned long igd_opregion_pgbase = 0;
>>
>> +unsigned int need_skip_rmrr = 0;
>
> Static (and without initializer)?

static unsigned int need_skip_rmrr;

>
>> +
>> +/*
>> + * Check whether there exists mmio hole in the specified memory range.
>> + * Returns 1 if exists, else returns 0.
>> + */
>> +static int check_mmio_hole_confliction(uint64_t start, uint64_t memsize,
>
> I don't think the word "confliction" exists. "conflict" please.

s/check_mmio_hole_confliction/check_mmio_hole_conflict/g

>
>> +                           uint64_t mmio_start, uint64_t mmio_size)
>> +{
>> +    if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size )
>> +        return 0;
>> +    else
>> +        return 1;
>
> Make this a simple single return statement?

static int check_mmio_hole_conflict(uint64_t start, uint64_t memsize,
                                     uint64_t mmio_start, uint64_t 
mmio_size)
{
     if ( start + memsize <= mmio_start || start >= mmio_start + mmio_size )
         return 0;

     return 1;
}

>
>> +}
>> +
>> +static int check_reserved_device_memory_map(uint64_t mmio_base,
>> +                                            uint64_t mmio_max)
>> +{
>> +    uint32_t i = 0;
>
> Pointless initializer.
>
>> +    uint64_t rdm_start, rdm_end;
>> +    int nr_entries = -1;
>
> And again.
>
>> +
>> +    nr_entries = hvm_get_reserved_device_memory_map();
>
> It's completely unclear why this can't be the variable's initializer.

     uint32_t i;
     uint64_t rdm_start, rdm_end;
     int nr_rdm_entries = hvm_get_reserved_device_memory_map();

>
>> +
>> +    for ( i = 0; i < nr_entries; i++ )
>> +    {
>> +        rdm_start = rdm_map[i].start_pfn << PAGE_SHIFT;
>> +        rdm_end = rdm_start + (rdm_map[i].nr_pages << PAGE_SHIFT);
>
> I'm pretty certain I pointed out before that you can't simply shift
> these fields - you risk losing significant bits.

I tried to go back looking into something but just found you were saying 
I shouldn't use PAGE_SHIFT and PAGE_SIZE at the same time. If I'm still 
missing could you show me what you expect?

>
>> +        if ( check_mmio_hole_confliction(rdm_start, (rdm_end - rdm_start),
>
> Pointless parentheses.

         if ( check_mmio_hole_conflict(rdm_start, rdm_end - rdm_start,

>
>> +                                         mmio_base, mmio_max - mmio_base) )
>> +        {
>> +            need_skip_rmrr++;
>> +        }
>> +    }
>> +
>> +    return nr_entries;
>> +}
>> +
>>   void pci_setup(void)
>>   {
>>       uint8_t is_64bar, using_64bar, bar64_relocate = 0;
>> @@ -58,7 +96,9 @@ void pci_setup(void)
>>           uint32_t bar_reg;
>>           uint64_t bar_sz;
>>       } *bars = (struct bars *)scratch_start;
>> -    unsigned int i, nr_bars = 0;
>> +    unsigned int i, j, nr_bars = 0;
>> +    int nr_entries = 0;
>
> And another pointless initializer. Plus as a count of something this

int nr_rdm_entries;

> surely wants to be "unsigned int". Also I guess the variable name

nr_rdm_entries should be literally unsigned int but this value always be 
set from  hvm_get_reserved_device_memory_map(),

nr_rdm_entries = hvm_get_reserved_device_memory_map()

I hope that return value can be negative value in some failed case

> is too generic - nr_rdm_entries perhaps?

Okay, s/nr_entries/nr_rdm_entries/g

>
>> @@ -363,11 +411,29 @@ void pci_setup(void)
>>               bar_data &= ~PCI_BASE_ADDRESS_IO_MASK;
>>           }
>>
>> + reallocate_mmio:
>>           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 ( need_skip_rmrr )
>> +        {
>> +            for ( j = 0; j < nr_entries; j++ )
>> +            {
>> +                rdm_start = rdm_map[j].start_pfn << PAGE_SHIFT;
>> +                rdm_end = rdm_start + (rdm_map[j].nr_pages << PAGE_SHIFT);
>> +                if ( check_mmio_hole_confliction(rdm_start,
>> +                                                 (rdm_end - rdm_start),
>> +                                                 base, bar_sz) )
>
> "base" was already updated by this point.

Looks I should insert these code fragments between

bar_data_upper = (uint32_t)(base >> 32);

and

base += bar_sz;

So (See below)

>
>> +                {
>> +                    resource->base = rdm_end;
>> +                    need_skip_rmrr--;
>> +                    goto reallocate_mmio;
>
> If you ever get here, the earlier determination of whether the MMIO
> hole is large enough may get invalidated. I.e. I'm afraid this approach
> is still to simplistic. Also to way you do the retry, the resulting bar_data

Here move 'reallocate_mmio' downward one line, and

s/resource->base = rdm_end;/base = (rdm_end  + bar_sz - 1) & 
~(uint64_t)(bar_sz - 1);

Then,
	...
         base = (resource->base  + bar_sz - 1) & ~(uint64_t)(bar_sz - 1);
  reallocate_mmio:
         bar_data |= (uint32_t)base;
         bar_data_upper = (uint32_t)(base >> 32);

         if ( need_skip_rmrr )
         {
             for ( j = 0; j < nr_rdm_entries; j++ )
             {
                 rdm_start = rdm_map[j].start_pfn << PAGE_SHIFT;
                 rdm_end = rdm_start + (rdm_map[j].nr_pages << PAGE_SHIFT);
                 if ( check_mmio_hole_conflict(rdm_start, rdm_end - 
rdm_start,
                                               base, bar_sz) )
                 {
                     base = (rdm_end  + bar_sz - 1) & ~(uint64_t)(bar_sz 
- 1);
                     need_skip_rmrr--;
                     goto reallocate_mmio;
                 }
             }
         }

         base += bar_sz;

Additionally, actually there are some original codes just following my 
codes:

         if ( need_skip_rmrr )
         {
		...
         }

	base += bar_sz;

         if ( (base < resource->base) || (base > resource->max) )
         {
             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;
         }

This can guarantee we don't overwhelm the previous mmio range.

> and bar_data_upper will likely end up being garbage.
>
> Did you actually _test_ this code?

Actually in my real case those RMRR ranges are always below MMIO.

Thanks
Tiejun