Re: [PATCH 3/5] pci-assign: Fix dword read at PCI_COMMAND

[Jan Kiszka <jan.kiszka@siemens.com>]

On 2011-04-28 16:51, Alex Williamson wrote:
> On Thu, 2011-04-28 at 10:59 +0200, Jan Kiszka wrote:
>> If we emulate the command register, we must only read its content from
>> the shadow config space. For dword read of both PCI_COMMAND and
>> PCI_STATUS, at least the latter must be read from the device.
>>
>> For simplicity reasons and as the code path is not considered
>> performance critical for the affected SRIOV devices, the fix performes
>> device access to the command word unconditionally, even if emulation is
>> enabled and only that word is read.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/device-assignment.c |    8 +++++---
>>  1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index f37f108..ee81434 100644
>> --- a/hw/device-assignment.c
>> +++ b/hw/device-assignment.c
>> @@ -482,14 +482,11 @@ static uint32_t assigned_dev_pci_read_config(PCIDevice *d, uint32_t address,
>>      /*
>>       * Catch access to
>>       *  - vendor & device ID
>> -     *  - command register (if emulation needed)
>>       *  - base address registers
>>       *  - ROM base address & capability pointer
>>       *  - interrupt line & pin
>>       */
>>      if (ranges_overlap(address, len, PCI_VENDOR_ID, 4) ||
>> -        (pci_dev->need_emulate_cmd &&
>> -         ranges_overlap(address, len, PCI_COMMAND, 2)) ||
>>          ranges_overlap(address, len, PCI_BASE_ADDRESS_0, 24) ||
>>          ranges_overlap(address, len, PCI_ROM_ADDRESS, 8) ||
>>          ranges_overlap(address, len, PCI_INTERRUPT_LINE, 2)) {
>> @@ -521,6 +518,11 @@ do_log:
>>      DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n",
>>            (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len);
>>  
>> +    if (pci_dev->need_emulate_cmd) {
>> +        val = merge_bits(val, pci_default_read_config(d, PCI_COMMAND, 2),
>> +                         address, len, PCI_COMMAND, 0xffff);
> 
> Shouldn't this be pci_default_read_config(d, address, len)?  I think
> what you have works since PCI_COMMAND is at the start of a dword, but it
> violates the merge_bits() assumption that val and mval are from the same
> address with the same length.  Thanks,

This is actually a real issue, reading a byte from PCI_COMMAND+1 is broken.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux