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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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

Jan



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