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

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

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