From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter Xu <peterx@redhat.com>
Subject: Re: [kvm-unit-tests PATCH 13/14] pci: add msi support for 32/64bit
 address
Date: Tue, 25 Oct 2016 14:21:34 +0800
Message-ID: <20161025062134.GB15168@pxdev.xzpeter.org>
References: <1476448852-30062-1-git-send-email-peterx@redhat.com>
 <1476448852-30062-14-git-send-email-peterx@redhat.com>
 <20161020133032.6e7nub4ycl4uywkk@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 <drjones@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx1.redhat.com ([209.132.183.28]:54706 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1756370AbcJYGWM (ORCPT <rfc822;kvm@vger.kernel.org>);
        Tue, 25 Oct 2016 02:22:12 -0400
Content-Disposition: inline
In-Reply-To: <20161020133032.6e7nub4ycl4uywkk@kamzik.brq.redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On Thu, Oct 20, 2016 at 03:30:32PM +0200, Andrew Jones wrote:

[...]

> > +static void pci_cap_walk(pci_dev *dev)
> > +{
> > +	uint8_t cap_offset;
> > +	uint8_t cap_id;
> > +
> > +	cap_offset = pci_config_readb(dev->pci_addr, PCI_CAPABILITY_LIST);
> > +	while (cap_offset) {
> > +		cap_id = pci_config_readb(dev->pci_addr, cap_offset);
> > +		printf("PCI detected cap 0x%x\n", cap_id);
> > +		if (cap_handlers[cap_id]) {
> > +			cap_handlers[cap_id](dev, cap_offset);
> > +		}
> 
> nit: no need for {}

Will fix.

[...]

> > @@ -49,6 +107,7 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id)
> >  	dev->inited = true;
> >  
> >  	pci_set_master(dev, 1);
> > +	pci_cap_walk(dev);
> 
> again, not sure we want this in the one and only init function

I'll put it into the new pci_enable_defaults().

> 
> >  
> >  	return 0;
> >  }
> > diff --git a/lib/pci.h b/lib/pci.h
> > index 6a1c3c9..5581446 100644
> > --- a/lib/pci.h
> > +++ b/lib/pci.h
> > @@ -27,6 +27,7 @@ enum {
> >  struct pci_dev {
> >  	bool inited;
> >  	uint16_t pci_addr;
> > +	uint16_t msi_offset;
> >  	phys_addr_t pci_bar[PCI_BAR_NUM];
> >  };
> >  typedef struct pci_dev pci_dev;
> > @@ -35,6 +36,7 @@ int pci_dev_init(pci_dev *dev, uint16_t vendor_id, uint16_t device_id);
> >  phys_addr_t pci_bar_addr(pci_dev *dev, int bar_num);
> >  bool pci_bar_is_memory(pci_dev *dev, int bar_num);
> >  bool pci_bar_is_valid(pci_dev *dev, int bar_num);
> > +void pci_setup_msi(pci_dev *dev, uint64_t msi_addr, uint32_t msi_data);
> >  
> >  /*
> >   * pci-testdev is a driver for the pci-testdev qemu pci device. The
> > -- 
> > 2.7.4
> >
> 
> Looks good from my not knowing much about PCI perspective.

Thanks for review!

-- peterx