Re: [kvm-unit-tests PATCH 04/14] pci: refactor init process to pci_dev_init()

[Peter Xu <peterx@redhat.com>]

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