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

[Alexey G <x1917x@gmail.com>]

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

>> @@ -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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel