Hi Sergey,
On 12/16/20 12:24 PM, Alexandru Elisei wrote:
> Hi Sergey,
>
> On 12/15/20 2:35 PM, Sergey Temerkhanov wrote:
>> Deactivate BARs before reactivating them to avoid breakage.
>> Some firmware components do not check whether the COMMAND
>> register contains any values before writing BARs which leads
>> to kvmtool errors during subsequent BAR deactivation
>>
>> Signed-off-by: Sergey Temerkhanov <s.temerkhanov@xxxxxxxxx>
>> ---
>> pci.c | 4 ----
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/pci.c b/pci.c
>> index 2e2c027..515d9dc 100644
>> --- a/pci.c
>> +++ b/pci.c
>> @@ -320,10 +320,6 @@ static void pci_config_bar_wr(struct kvm *kvm,
>> */
>> if (value == 0xffffffff) {
>> value = ~(pci__bar_size(pci_hdr, bar_num) - 1);
>> - /* Preserve the special bits. */
>> - value = (value & mask) | (pci_hdr->bar[bar_num] & ~mask);
>> - pci_hdr->bar[bar_num] = value;
>> - return;
> I think the PCI spec is clear what should happen when software writes all 1's to
> the BAR (PCI LOCAL BUS SPECIFICATION, REV. 3.0, page 226):
>
> "Power-up software can determine how much address space the device requires by
> writing a value of all 1's to the register and then reading the value back. The
> device will return 0's in all don't-care address bits, effectively specifying the
> address space required."
>
> Your patch breaks this mechanism for sizing a BAR.
Sorry, it doesn't break it, you can still read the size from the bar afterwards.
What your patch does is enable emulation for the memory region starting at
~(bar_size - 1), which is definitely not expected. What if we were on real
hardware and ~(bar_size - 1) falls outside the controller's address range? What if
for kvmtool it overlaps with an existing MMIO region?
Here's what should happen according the the PCI spec:
"Software saves the original value of the Base Address register, writes 0 FFFF
FFFFh to the register, then reads it back. Size calculation can be done from the
32-bit value read by first clearing encoding information bits (bit 0 for I/O, bits
0-3 for memory), inverting all 32 bits (logical NOT), then incrementing by 1. The
resultant 32-bit value is the memory/I/O range size decoded by the register. Note
that the upper 16 bits of the result is ignored if the Base Address register is
for I/O and bits 16-31 returned zero upon read. The original value in the Base
Address register is restored before re-enabling decode in the command register of
the device."
If you're getting errors when access is disabled, I suspect it's because the
firmware doesn't restore the previous BAR value.
Thanks,
Alex
>
> It also looks to me like the firmware is doing something very strange: it writes
> all 1's to a BAR, which with your patch enables emulation for the memory region
> [~(bar_size-1), ~(bar_size-1)+bar_size]. How does the firmware know the value of
> bar_size? It reads it back and confuses it with the address instead of the region
> size?
>
> Would you mind posting more details about the error you are seeing? Maybe we can
> find a different solution.
>
> Thanks,
>
> Alex
>
