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 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:    
>>> --- a/tools/firmware/hvmloader/config.h
>>> +++ b/tools/firmware/hvmloader/config.h
>>> @@ -53,10 +53,14 @@ extern uint8_t ioapic_version;
>>>  #define PCI_ISA_DEVFN       0x08    /* dev 1, fn 0 */
>>>  #define PCI_ISA_IRQ_MASK    0x0c20U /* ISA IRQs 5,10,11 are PCI connected 
>>> */
>>>  #define PCI_ICH9_LPC_DEVFN  0xf8    /* dev 31, fn 0 */
>>> +#define PCI_MCH_DEVFN       0       /* bus 0, dev 0, func 0 */    
>>
>>Just MCH is liable to become ambiguous in the future. Perhaps 
>>PCI_Q35_MCH_DEVFN?  
>
>Agree, PCI_Q35_MCH_DEVFN is more explicit.
>
>>> @@ -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.

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.

>>> @@ -259,13 +264,21 @@ void pci_setup(void)
>>>                  bar_reg = PCI_ROM_ADDRESS;
>>>  
>>>              bar_data = pci_readl(devfn, bar_reg);
>>> +
>>> +            is_mem = !!(((bar_data & PCI_BASE_ADDRESS_SPACE) ==
>>> +                       PCI_BASE_ADDRESS_SPACE_MEMORY) ||
>>> +                       (bar_reg == PCI_ROM_ADDRESS));    
>>
>>Once you make is_mem properly bool, !! won't be needed anymore.  
>
>OK, will switch to bool.
>
>>Jan
>>
>>  
>


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