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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.10.14 at 08:11, <tiejun.chen@xxxxxxxxx> wrote:
> On 2014/10/27 17:56, Jan Beulich wrote:
>>>>> On 27.10.14 at 08:12, <tiejun.chen@xxxxxxxxx> wrote:
>>> On 2014/10/24 22:42, Jan Beulich wrote:
>>>>>>> On 24.10.14 at 09:34, <tiejun.chen@xxxxxxxxx> 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.

>>> 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.

>>>> 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?

Yes, guest boot, but including proper inspection that what your
code does is really how it should be.

Jan

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