From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCH 4/5] pci-assign: Convert need_emulate_cmd into a bitmask Date: Wed, 15 Dec 2010 12:36:37 +0200 Message-ID: <20101215103636.GF28825@redhat.com> References: <89c6067f3e4ab9eca18f433972595039b266d62e.1292282738.git.jan.kiszka@web.de> <1292284589.2857.139.camel@x201> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kiszka , Avi Kivity , Marcelo Tosatti , kvm@vger.kernel.org, Jan Kiszka To: Alex Williamson Return-path: Received: from mx1.redhat.com ([209.132.183.28]:43683 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752647Ab0LOKhH (ORCPT ); Wed, 15 Dec 2010 05:37:07 -0500 Content-Disposition: inline In-Reply-To: <1292284589.2857.139.camel@x201> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Dec 13, 2010 at 04:56:29PM -0700, Alex Williamson wrote: > On Tue, 2010-12-14 at 00:25 +0100, Jan Kiszka wrote: > > From: Jan Kiszka > > > > Define a mask of PCI command register bits that need to be emulated, > > i.e. read back from their shadow state. We will need this for > > selectively emulating the INTx mask bit. > > > > Note: No initialization of emulate_cmd_mask to zero needed, the device > > state is already zero-initialized. > > > > Signed-off-by: Jan Kiszka > > --- > > hw/device-assignment.c | 18 ++++++++++-------- > > hw/device-assignment.h | 2 +- > > 2 files changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c > > index ef045f4..26d3bd7 100644 > > --- a/hw/device-assignment.c > > +++ b/hw/device-assignment.c > > @@ -525,14 +525,17 @@ again: > > DEBUG("(%x.%x): address=%04x val=0x%08x len=%d\n", > > (d->devfn >> 3) & 0x1F, (d->devfn & 0x7), address, val, len); > > > > - if (pci_dev->need_emulate_cmd && > > + if (pci_dev->emulate_cmd_mask && > > ranges_overlap(address, len, PCI_COMMAND, 2)) { > > if (address == PCI_COMMAND) { > > - val &= 0xffff0000; > > - val |= pci_default_read_config(d, PCI_COMMAND, 2); > > + val &= ~pci_dev->emulate_cmd_mask; > > + val |= pci_default_read_config(d, PCI_COMMAND, 2) & > > + pci_dev->emulate_cmd_mask; > > } else { > > /* high-byte access */ > > - val = pci_default_read_config(d, PCI_COMMAND+1, 1); > > + val &= ~(pci_dev->emulate_cmd_mask >> 8); > > + val |= pci_default_read_config(d, PCI_COMMAND+1, 1) & > > + (pci_dev->emulate_cmd_mask >> 8); > > } > > } > > We should definitely be using merge_bits here, this is the sort of thing > I had in mind for it: > > val = merge_bits(val, pci_default_read_config(d, address, len), PCI_COMMAND, pci_dev->emulate_cmd_mask); Not a comment on this patch at all, rather on assigned-devices support code generally: I think code will become cleaner if we add a mask array mapping the config space, along the lines of wmask and friends in pci.c I think this will make it possible to mostly get rid of merge_bits and tricky range check hacks. > > @@ -800,10 +803,9 @@ again: > > > > /* dealing with virtual function device */ > > snprintf(name, sizeof(name), "%sphysfn/", dir); > > - if (!stat(name, &statbuf)) > > - pci_dev->need_emulate_cmd = 1; > > - else > > - pci_dev->need_emulate_cmd = 0; > > + if (!stat(name, &statbuf)) { > > + pci_dev->emulate_cmd_mask = 0xffff; > > + } > > > > dev->region_number = r; > > return 0; > > diff --git a/hw/device-assignment.h b/hw/device-assignment.h > > index c94a730..9ead022 100644 > > --- a/hw/device-assignment.h > > +++ b/hw/device-assignment.h > > @@ -109,7 +109,7 @@ typedef struct AssignedDevice { > > void *msix_table_page; > > target_phys_addr_t msix_table_addr; > > int mmio_index; > > - int need_emulate_cmd; > > + uint32_t emulate_cmd_mask; > > char *configfd_name; > > QLIST_ENTRY(AssignedDevice) next; > > } AssignedDevice; > >