Re: [PATCHv4] device-assignment: don't touch pci command register

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

On 2012-04-17 14:10, Michael S. Tsirkin wrote:
> Real command register is under kernel control:
> it includes bits for triggering SERR, marking
> BARs as invalid and such which are all under host
> kernel control.
> 
> While there's no known bug this triggers - since qemu does its
> best to make guest state match device state -
> it seems safer to avoid touching this register as much as
> possible.
> 
> With this patch, we don't touch any bits
> except bus master which is ok to put under guest control
> and intx mask which kvm interrupt sharing machinery
> explicitly allows.
> 
> Note: PCI_STATUS bears looking into as well.
> 
> Tested-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> Changes from v3:
> - use pci_word_test_and_clear_mask to clear bits in a clean way
> Changes from v2:
> - whitespace fix
> Changes from v1:
> - fix intx mask
> 
>  hw/device-assignment.c |   20 ++++++--------------
>  1 files changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 89823f1..17a3a93 100644
> --- a/hw/device-assignment.c
> +++ b/hw/device-assignment.c
> @@ -501,7 +501,6 @@ static int get_real_device(AssignedDevice *pci_dev, uint16_t r_seg,
>      FILE *f;
>      unsigned long long start, end, size, flags;
>      uint16_t id;
> -    struct stat statbuf;
>      PCIRegion *rp;
>      PCIDevRegions *dev = &pci_dev->real_device;
>  
> @@ -610,12 +609,8 @@ again:
>      pci_dev->dev.config[2] = id & 0xff;
>      pci_dev->dev.config[3] = (id & 0xff00) >> 8;
>  
> -    /* dealing with virtual function device */
> -    snprintf(name, sizeof(name), "%sphysfn/", dir);
> -    if (!stat(name, &statbuf)) {
> -        /* always provide the written value on readout */
> -        assigned_dev_emulate_config_read(pci_dev, PCI_COMMAND, 2);
> -    }
> +    pci_word_test_and_clear_mask(pci_dev->emulate_config_write + PCI_COMMAND,
> +                                 PCI_COMMAND_MASTER | PCI_COMMAND_INTX_DISABLE);
>  
>      dev->region_number = r;
>      return 0;
> @@ -782,13 +778,9 @@ static int assign_device(AssignedDevice *dev)
>                  "cause host memory corruption if the device issues DMA write "
>                  "requests!\n");
>      }
>      if (dev->features & ASSIGNED_DEVICE_SHARE_INTX_MASK &&
>          kvm_has_intx_set_mask()) {
>          assigned_dev_data.flags |= KVM_DEV_ASSIGN_PCI_2_3;
> -
> -        /* hide host-side INTx masking from the guest */
> -        dev->emulate_config_read[PCI_COMMAND + 1] |=
> -            PCI_COMMAND_INTX_DISABLE >> 8;
>      }
>  
>      r = kvm_assign_pci_device(kvm_state, &assigned_dev_data);
> @@ -1631,10 +1624,10 @@ static void reset_assigned_device(DeviceState *dev)
>      }
>  
>      /*
> -     * When a 0 is written to the command register, the device is logically
> +     * When a 0 is written to the bus master register, the device is logically
>       * disconnected from the PCI bus. This avoids further DMA transfers.
>       */
> -    assigned_dev_pci_write_config(pci_dev, PCI_COMMAND, 0, 2);
> +    assigned_dev_pci_write_config(pci_dev, PCI_COMMAND, 0, 1);
>  }
>  
>  static int assigned_initfn(struct PCIDevice *pci_dev)
> @@ -1658,7 +1651,6 @@ static int assigned_initfn(struct PCIDevice *pci_dev)
>       * device initialization.
>       */
>      assigned_dev_emulate_config_read(dev, 0, PCI_CONFIG_SPACE_SIZE);
> -    assigned_dev_direct_config_read(dev, PCI_COMMAND, 2);
>      assigned_dev_direct_config_read(dev, PCI_STATUS, 2);
>      assigned_dev_direct_config_read(dev, PCI_REVISION_ID, 1);
>      assigned_dev_direct_config_read(dev, PCI_CLASS_PROG, 3);

Acked-by: Jan Kiszka <jan.kiszka@siemens.com>

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