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

[Alexey G <x1917x@xxxxxxxxx>]

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.

Anyway, as the BAR allocation code is not memory- or
time-consuming/critical, I guess any option will be good.

>> Regarding avoiding using bool here -- the only reason was adapting to
>> the existing code style. For some reason the existing hvmloader code
>> prefers to use uint-types for bool values.  
>
>And wrongly so. We're slowly moving over, and we'd prefer the issue to
>not be widened by new code.

BTW, there are other changes pending for hvmloader/pci.c which will
(hopefully :) ) replace its BAR allocation and RMRR handling code, so
this patch can be considered as sort of intermediate one -- I'm using a
heavily reworked version of hvmloader/pci.c which I'd like to upstream.

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