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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
