Re: [PATCH kvm-unit-tests 15/17] pci: add msi support for 32/64bit address

[Peter Xu <peterx@redhat.com>]

On Fri, Nov 04, 2016 at 06:33:29PM +0100, Andrew Jones wrote:

[...]

> > +static pci_cap_handler cap_handlers[0xff] = {
> 
> Why 0xff? Shouldn't it be (PCI_CAP_ID_MAX + 1)?

Yes. Will fix.

[...]

> > +int pci_setup_msi(struct pci_dev *dev, uint64_t msi_addr, uint32_t msi_data)
> > +{
> > +	uint16_t msi_control;
> > +	uint16_t offset;
> > +	pcidevaddr_t addr = dev->pci_bdf;
> > +
> > +	assert(dev);
> > +
> > +	if (!dev->msi_offset) {
> > +		printf("MSI: dev 0x%x does not support MSI.\n", addr);
> > +		return -1;
> > +	}
> > +
> > +	offset = dev->msi_offset;
> > +	msi_control = pci_config_readw(addr, offset + PCI_MSI_FLAGS);
> > +	pci_config_writel(addr, offset + PCI_MSI_ADDRESS_LO,
> > +			  msi_addr & 0xffffffff);
> > +
> > +	if (msi_control & PCI_MSI_FLAGS_64BIT) {
> > +		pci_config_writel(addr, offset + PCI_MSI_ADDRESS_HI,
> > +				  (uint32_t)(msi_addr >> 32));
> > +		pci_config_writel(addr, offset + PCI_MSI_DATA_64, msi_data);
> > +		printf("MSI: dev 0x%x init 64bit address: ", addr);
> > +	} else {
> > +		pci_config_writel(addr, offset + PCI_MSI_DATA_32, msi_data);
> > +		printf("MSI: dev 0x%x init 32bit address: ", addr);
> > +	}
> > +	printf("addr=0x%lx, data=0x%x\n", msi_addr, msi_data);
> > +
> > +	msi_control |= PCI_MSI_FLAGS_ENABLE;
> > +	pci_config_writew(addr, offset + PCI_MSI_FLAGS, msi_control);
> > +
> > +	return 0;
> > +}
> 
> Again we only have 0/-1 for good/bad. I'd prefer true/false.

Will use bool as return code.

> Otherwise
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com> 

Thanks!

-- peterx