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

[Alex Williamson <alex.williamson@redhat.com>]

On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
> 
> 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 |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index bc3a57b..6ff1456 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -494,14 +494,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)) {
> @@ -533,6 +530,17 @@ 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 &&
> +        ranges_overlap(address, len, PCI_COMMAND, 2)) {
> +        if (address == PCI_COMMAND) {
> +            val &= 0xffff0000;
> +            val |= pci_default_read_config(d, PCI_COMMAND, 2);
> +        } else {
> +            /* high-byte access */
> +            val = pci_default_read_config(d, PCI_COMMAND+1, 1);
> +        }
> +    }
> +
>      if (!pci_dev->cap.available) {
>          /* kill the special capabilities */
>          if (address == PCI_COMMAND && len == 4) {

We might be able to use the merge_bits function that I just added for
capability support, perhaps something like:

if (pci_dev->need_emulate_cmd) {
    val = merge_bits(val, pci_default_read_config(d, address, len), PCI_COMMAND, 0xffff)
}