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

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Alexey G <x1917x@xxxxxxxxx> 05/31/18 7:15 AM >>>
>On Wed, 30 May 2018 02:12:37 -0600 "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
>>>>> On 29.05.18 at 20:47, <x1917x@xxxxxxxxx> wrote:  
>>> On Wed, 30 May 2018 03:56:07 +1000
>>> Alexey G <x1917x@xxxxxxxxx> wrote:  
>>>>On Tue, 29 May 2018 08:23:51 -0600
>>>>"Jan Beulich" <JBeulich@xxxxxxxx> wrote:  
>>>>>>>> On 12.03.18 at 19:33, <x1917x@xxxxxxxxx> 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.

Jan



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