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

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

[Michael S. Tsirkin]

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);
-- 
1.7.9.111.gf3fb0

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

[Alex Williamson]

On Tue, 2012-04-17 at 15:10 +0300, 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>

Acked-by: Alex Williamson <alex.williamson@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);

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

[Jan Kiszka]

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

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

[Marcelo Tosatti]

On Tue, Apr 17, 2012 at 03:10:42PM +0300, 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>

Applied, thanks.