Re: [RFC PATCH 07/12] hvmloader: allocate MMCONFIG area in the MMIO hole + minor code refactoring

[Alexey G <x1917x@gmail.com>]

On Thu, 31 May 2018 23:30:35 -0600
"Jan Beulich" <jbeulich@suse.com> wrote:

>>>> Alexey G <x1917x@gmail.com> 05/31/18 7:15 AM >>>  
>>On Wed, 30 May 2018 02:12:37 -0600 "Jan Beulich" <JBeulich@suse.com> wrote:  
>>>>>> On 29.05.18 at 20:47, <x1917x@gmail.com> wrote:    
>>>> On Wed, 30 May 2018 03:56:07 +1000
>>>> Alexey G <x1917x@gmail.com> wrote:    
>>>>>On Tue, 29 May 2018 08:23:51 -0600
>>>>>"Jan Beulich" <JBeulich@suse.com> wrote:    
>>>>>>>>> On 12.03.18 at 19:33, <x1917x@gmail.com> wrote:        
>>>>>>> @@ -172,10 +173,14 @@ void pci_setup(void)
>>>>>>>  
>>>>>>>      /* Create a list of device BARs in descending order of size. */
>>>>>>>      struct bars {
>>>>>>> -        uint32_t is_64bar;
>>>>>>>          uint32_t devfn;
>>>>>>>          uint32_t bar_reg;
>>>>>>>          uint64_t bar_sz;
>>>>>>> +        uint64_t addr_mask; /* which bits of the base address can be written */
>>>>>>> +        uint32_t bar_data;  /* initial value - BAR flags here */        
>>>>>>
>>>>>>Why 32 bits? You only use the low few ones afaics. Also please avoid fixed width
>>>>>>types unless you really need them.      
>>>>>
>>>>>bar_data is supposed to hold only BAR's kludge bits like 'enabled' bit
>>>>>values or MMCONFIG width bits. All of them occupy the low dword only
>>>>>while BAR's high dword is just a part of the address which will be
>>>>>replaced by allocated one (for mem64 BARs), thus no need to keep the
>>>>>high half.
>>>>>
>>>>>So this is a sort of minor optimization -- avoiding using 64-bit operand
>>>>>size when 32 bit is enough.    
>>>> 
>>>> Sorry, looks like I've misread the question. You were actually 
>>>> suggesting to make bar_data shorter. 8 bits is enough at the moment, so
>>>> bar_data can be changed to uint8_t, yes.    
>>>
>>>Right.  
>>
>>Ok, I'll switch to smaller types though not sure if it will make any
>>significant impact I'm afraid. 
>>
>>In particular, bar_data will be typically used in 32/64-bit 
>>arithmetics, using a 32-bit datatype means we avoiding explicit zero
>>extension for both 32 and 64-bit operations while for an uint8_t field
>>the compiler will have to provide extra MOVZX instructions to embed a
>>8-bit operand into 32/64-bit expressions. 32-bit bar_reg can be made
>>16-bit in the same way but any memory usage improvements will be
>>similarly counteracted by a requirement to use 66h-prefixed
>>instructions for it.  
>
>Hmm, yes, the space saving from using less wide types are probably indeed
>not worth it. But then please switch to "unsigned int" instead of uint<N>_t
>whenever the exact size doesn't matter.

Ok, will do in v2.

>Jan
>
>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel