From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCHv2] device-assignment: don't touch real command register Date: Tue, 17 Apr 2012 13:25:59 +0200 Message-ID: <4F8D5347.70300@siemens.com> References: <20120416195327.GA30402@redhat.com> <4F8D52E8.8080403@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: "kvm@vger.kernel.org" , Marcelo Tosatti , Alex Williamson , Avi Kivity To: "Michael S. Tsirkin" Return-path: Received: from goliath.siemens.de ([192.35.17.28]:22444 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755153Ab2DQL0E (ORCPT ); Tue, 17 Apr 2012 07:26:04 -0400 In-Reply-To: <4F8D52E8.8080403@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 >> >> --- >> 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