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

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

On 2014/10/28 17:56, Jan Beulich wrote:
>>>> On 28.10.14 at 08:11, <tiejun.chen@intel.com> wrote:
>> On 2014/10/27 17:56, Jan Beulich wrote:
>>>>>> On 27.10.14 at 08:12, <tiejun.chen@intel.com> wrote:
>>>> 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;
>>>
>>> Please stop echoing back what was requested.
>>
>> I try to fix inline to make sure I'm addressing all comments inline
>> properly. If you think this is correct please just ignore that.
>
> But it still takes people reading you reply time to read that. Please
> ask back only when you think an earlier reply was ambiguous.
>
>>>>>> +    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?
>>>
>>> Shifting a 32-bit quantity left still yields a 32-bit quantity, no matter
>>> whether the result is then stored in a 64-bit variable. You need to
>>> up-cast the left side of the shift first.
>>
>> Do you mean this?
>>
>> rdm_start = (uint64_t)rdm_map[j].start_pfn << PAGE_SHIFT;
>> rdm_end = rdm_start + ((uint64_t)rdm_map[j].nr_pages << PAGE_SHIFT);
>
> Yes. Finally.
>
>>>>>> @@ -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
>>>
>>> If only you checked for these negative values...
>>
>> May I can simplify these failed cases handle with within
>> hvm_get_reserved_device_memory_map() like,
>> 	if ( rc )
>> 		return 0;
>>
>> Because actually we don't need any negative return value again. So '0'
>> is always fine. So here,
>>
>> unsigned long nr_rdm_entries = hvm_get_reserved_device_memory_map();
>
> Except that "unsigned int" would seem more consistent.

Fixed.

>
>>>> 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.
>>>
>>> Resulting in the BAR not getting a value assigned afaict. Certainly
>>> not what we want as a side effect of your changes.
>>
>> I don't understand what a side effect is. I just to try to make sure BAR
>> space skip any conflict range but they are still in these resource ranges.
>
> A side effect is an effect you don't primarily intend with your change
> (or more generally, with any particular operation). In the case here,
> a BAR that previously got a value assigned may not anymore with
> your change in place. An acceptable effect of your change would be
> if the value it gets assigned is now different, but not assigning a value
> at all is not acceptable.

As I understand that value just need to align with BAR and size. Then 
any range should be fine. Here I think its not necessary to consider any 
space restriction, i.e, some device may just access under 4G.

>
>>>>> 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.
>>>
>>> Below whose MMIO? The host's or the guest's? In the latter case,
>>> just (in order to test your code) increase the range reserved for
>>> MMIO enough to cover the RMRR range.
>>
>> In my platform,
>>
>> RMRR region: base_addr ab80a000 end_address ab81dfff
>> RMRR region: base_addr ad000000 end_address af7fffff
>>
>> So I guess you hope I change this
>>
>> #define PCI_MEM_START       0xf0000000
>>
>> to
>>
>> #define PCI_MEM_START       0xa0000000
>>
>> right?
>
> Almost. You'd have to use 0x80000000, or other logic would break.
>
>> But what test you want to see? Just boot?

I don't see any strong error from boot.
>
> Yes, guest boot, but including proper inspection that what your
> code does is really how it should be.
>

Okay, I will go into details.

Thanks
Tiejun

> Jan
>