From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [kvm-unit-tests PATCH 04/14] pci: refactor init process to pci_dev_init() Date: Mon, 24 Oct 2016 15:00:02 +0800 Message-ID: <20161024070002.GS15168@pxdev.xzpeter.org> References: <1476448852-30062-1-git-send-email-peterx@redhat.com> <1476448852-30062-5-git-send-email-peterx@redhat.com> <20161020100258.e4geme4ntgkl7md7@kamzik.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: kvm@vger.kernel.org, jan.kiszka@web.de, agordeev@redhat.com, rkrcmar@redhat.com, pbonzini@redhat.com To: Andrew Jones Return-path: Received: from mx1.redhat.com ([209.132.183.28]:35252 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757197AbcJXHAE (ORCPT ); Mon, 24 Oct 2016 03:00:04 -0400 Content-Disposition: inline In-Reply-To: <20161020100258.e4geme4ntgkl7md7@kamzik.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Thu, Oct 20, 2016 at 12:02:58PM +0200, Andrew Jones wrote: > On Fri, Oct 14, 2016 at 08:40:42PM +0800, Peter Xu wrote: > > pci_find_dev() is not sufficient for further tests for Intel IOMMU. This > > patch introduced pci_dev struct, as a abstract of a specific PCI device. > > > > All the rest of current PCI APIs are changed to leverage the pci_dev > > struct. > > > > x86/vmexit.c is the only user of the old PCI APIs. Changing it to use > > the new ones. > > > > (Indentation is fixed from using 4 spaces into tab in pci.c) > > We need to get Alex's series in and then revisit this. He's maintained > the API's use of a devid handle. It's possible a struct would be better, > but I'm not so sure. In any case we need to break this patch up in order > to see the goals step-by-step. > > 1) cleanup style - Alex's series does that already > 2) replace the devid handle with struct pci_dev - should be done > everywhere or nowhere > 3) simplify x86/vmexit's by making better use of the API > 4) extend the API for IOMMU Yes. I'll follow above steps to split the patch. I think I'll just rebase the branch to Alex's for next version. [...] > > +int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id) > > +{ > > + unsigned i; > > + uint32_t id; > > + > > + memset(dev, 0, sizeof(*dev)); > > + > > + for (i = 0; i < PCI_DEVFN_MAX; ++i) { > > + id = pci_config_read(i, 0); > > Hmm, pci_config_read still uses devid, but everything else now uses > pci_dev. I don't really like that inconsistency. Yes, I think a struct for PCI device might be still essential (otherwise I don't know where to store per-device PCI information, like MSI offsets, etc.). In that sense, I would prefer using pci_dev in pci_config_*() ops. Will fix based on Alex's tree. > > > + if ((id & 0xFFFF) == vendor_id && (id >> 16) == device_id) > > + break; > > + } > > + > > + if (i == PCI_DEVFN_MAX) { > > + printf("PCI: failed init dev (vendor: 0x%04x, " > > + "device: 0x%04x)\n", vendor_id, device_id); > > + return -1; > > + } > > + > > + dev->pci_addr = i; > > but i is the devid, not the addr. Unless I misunderstand what 'addr' means > here. Right. Maybe I should rename pci_addr to pci_bdf. [...] > > @@ -27,8 +38,6 @@ bool pci_bar_is_valid(pcidevaddr_t dev, int bar_num); > > #define PCI_VENDOR_ID_REDHAT 0x1b36 > > #define PCI_DEVICE_ID_REDHAT_TEST 0x0005 > > > > -#define PCI_TESTDEV_NUM_BARS 2 > > Why remove this? I could be used to test that we only find this many > BARs on the testdev, no? Because there's no code referencing this macro, so cleaned it up. I don't remember whether Alex is using it, I think I can avoid touching it in next version. > > > - > > struct pci_test_dev_hdr { > > uint8_t test; > > uint8_t width; > > @@ -39,4 +48,12 @@ struct pci_test_dev_hdr { > > uint8_t name[]; > > }; > > > > +enum pci_dma_dir { > > + PCI_DMA_FROM_DEVICE = 0, > > + PCI_DMA_TO_DEVICE, > > +}; > > +typedef enum pci_dma_dir pci_dma_dir_t; > > + > > +typedef uint64_t iova_t; > > stray changes Yeah... I'll move them outside of this patch, to somewhere more suitable. [...] > > + if (!pci_dev_init(&dev, PCI_VENDOR_ID_REDHAT, > > + PCI_DEVICE_ID_REDHAT_TEST)) { > > + pci_test.memaddr = ioremap(dev.pci_bar[0], PAGE_SIZE); > > + pci_test.iobar = dev.pci_bar[1]; > > Before we didn't require BAR0 to be MEM and BAR1 to be IO, now we do. It's > probably safe, but less flexible. I think we should at least provide > asserts that our assumptions are correct. Sure. It'll be better to have asserts here. Thanks! -- peterx