[PATCHv2] device-assignment: don't touch real command register

[PATCHv2] device-assignment: don't touch real 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 under host
kernel control. Don't touch any except bus master
which is ok to put under guest control and intx
mask which kvm interrupt sharing machinery
explicitly allows.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---
Compiled only, don't have a setup for assignment
testing ATM. Is anyone interested enough to test
and report?

Changes from v1:
- fix intx mask handling

 hw/device-assignment.c |   24 ++++++++----------------
 1 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/hw/device-assignment.c b/hw/device-assignment.c
index 89823f1..6cdcc17 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,9 @@ 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);
-    }
+    /* Pass bus master writes to device. */
+    pci_dev->emulate_config_write[PCI_COMMAND] &= ~PCI_COMMAND_MASTER;
+    pci_dev->emulate_config_write[PCI_COMMAND + 1] &= ~(PCI_COMMAND_INTX_DISABLE >> 8);
 
     dev->region_number = r;
     return 0;
@@ -782,13 +778,10 @@ 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;
+    if (dev->features & ASSIGNED_DEVICE_SHARE_INTX_MASK &&
+	kvm_has_intx_set_mask()) {
+	    assigned_dev_data.flags |= KVM_DEV_ASSIGN_PCI_2_3;
     }
 
     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: [PATCHv2] device-assignment: don't touch real command register

[Alex Williamson]

On Mon, 2012-04-16 at 22:53 +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 under host
> kernel control. Don't touch any except bus master
> which is ok to put under guest control and intx
> mask which kvm interrupt sharing machinery
> explicitly allows.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> Compiled only, don't have a setup for assignment
> testing ATM. Is anyone interested enough to test
> and report?

Tested with an igb PF, seems to work correctly.  Guest can only toggle
INTx disable and BM.

> Changes from v1:
> - fix intx mask handling
> 
>  hw/device-assignment.c |   24 ++++++++----------------
>  1 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 89823f1..6cdcc17 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,9 @@ 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);
> -    }
> +    /* Pass bus master writes to device. */
> +    pci_dev->emulate_config_write[PCI_COMMAND] &= ~PCI_COMMAND_MASTER;
> +    pci_dev->emulate_config_write[PCI_COMMAND + 1] &= ~(PCI_COMMAND_INTX_DISABLE >> 8);
>  
>      dev->region_number = r;
>      return 0;
> @@ -782,13 +778,10 @@ 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;
> +    if (dev->features & ASSIGNED_DEVICE_SHARE_INTX_MASK &&
> +	kvm_has_intx_set_mask()) {
> +	    assigned_dev_data.flags |= KVM_DEV_ASSIGN_PCI_2_3;

nit, broken whitespace.

>      }
>  
>      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: [PATCHv2] device-assignment: don't touch real command register

[Jan Kiszka]

On 2012-04-16 21:53, 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 under host
> kernel control. Don't touch any except bus master
> which is ok to put under guest control and intx
> mask which kvm interrupt sharing machinery
> explicitly allows.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> Compiled only, don't have a setup for assignment
> testing ATM. Is anyone interested enough to test
> and report?
> 
> Changes from v1:
> - fix intx mask handling
> 
>  hw/device-assignment.c |   24 ++++++++----------------
>  1 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> index 89823f1..6cdcc17 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,9 @@ 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);
> -    }
> +    /* Pass bus master writes to device. */
> +    pci_dev->emulate_config_write[PCI_COMMAND] &= ~PCI_COMMAND_MASTER;
> +    pci_dev->emulate_config_write[PCI_COMMAND + 1] &= ~(PCI_COMMAND_INTX_DISABLE >> 8);

Comment doesn't fully match the code. And I bet you aren't using
checkpatch... ;)

>  
>      dev->region_number = r;
>      return 0;
> @@ -782,13 +778,10 @@ 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;
> +    if (dev->features & ASSIGNED_DEVICE_SHARE_INTX_MASK &&
> +	kvm_has_intx_set_mask()) {
> +	    assigned_dev_data.flags |= KVM_DEV_ASSIGN_PCI_2_3;
>      }
>  
>      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);

Patch looks otherwise fine to me.

Jan

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

Re: [PATCHv2] device-assignment: don't touch real command register

[Jan Kiszka]

On 2012-04-17 13:24, Jan Kiszka wrote:
> On 2012-04-16 21:53, 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 under host
>> kernel control. Don't touch any except bus master
>> which is ok to put under guest control and intx
>> mask which kvm interrupt sharing machinery
>> explicitly allows.
>>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> ---
>> Compiled only, don't have a setup for assignment
>> testing ATM. Is anyone interested enough to test
>> and report?
>>
>> Changes from v1:
>> - fix intx mask handling
>>
>>  hw/device-assignment.c |   24 ++++++++----------------
>>  1 files changed, 8 insertions(+), 16 deletions(-)
>>
>> diff --git a/hw/device-assignment.c b/hw/device-assignment.c
>> index 89823f1..6cdcc17 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,9 @@ 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);
>> -    }
>> +    /* Pass bus master writes to device. */
>> +    pci_dev->emulate_config_write[PCI_COMMAND] &= ~PCI_COMMAND_MASTER;
>> +    pci_dev->emulate_config_write[PCI_COMMAND + 1] &= ~(PCI_COMMAND_INTX_DISABLE >> 8);
> 
> Comment doesn't fully match the code. And I bet you aren't using
> checkpatch... ;)
> 
>>  
>>      dev->region_number = r;
>>      return 0;
>> @@ -782,13 +778,10 @@ 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;
>> +    if (dev->features & ASSIGNED_DEVICE_SHARE_INTX_MASK &&
>> +	kvm_has_intx_set_mask()) {
>> +	    assigned_dev_data.flags |= KVM_DEV_ASSIGN_PCI_2_3;
>>      }
>>  
>>      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);
> 
> Patch looks otherwise fine to me.

Err, I mean v3 on which I actually wanted to comment.

Jan

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

Re: [PATCHv2] device-assignment: don't touch real command register

[Michael S. Tsirkin]

On Tue, Apr 17, 2012 at 01:24:24PM +0200, Jan Kiszka wrote:
> > +    /* Pass bus master writes to device. */
> > +    pci_dev->emulate_config_write[PCI_COMMAND] &= ~PCI_COMMAND_MASTER;
> > +    pci_dev->emulate_config_write[PCI_COMMAND + 1] &= ~(PCI_COMMAND_INTX_DISABLE >> 8);
> 
> Comment doesn't fully match the code.

Will fix.

> And I bet you aren't using
> checkpatch... ;)

I didn't run it but it doesn't complain.