Re: [RFC PATCH 8/8] device-assignment: pass through and stub more PCI caps

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Nov 12, 2010 at 08:42:38AM -0700, Alex Williamson wrote:
> On Fri, 2010-11-12 at 11:11 +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 11, 2010 at 11:30:07PM -0700, Alex Williamson wrote:
> > > On Fri, 2010-11-12 at 07:36 +0200, Michael S. Tsirkin wrote:
> > > > On Thu, Nov 11, 2010 at 07:56:46PM -0700, Alex Williamson wrote:
> > > > > Some drivers depend on finding capabilities like power management,
> > > > > PCI express/X, vital product data, or vendor specific fields.  Now
> > > > > that we have better capability support, we can pass more of these
> > > > > tables through to the guest.  Note that VPD and VNDR are direct pass
> > > > > through capabilies, the rest are mostly empty shells with a few
> > > > > writable bits where necessary.
> > > > > 
> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > ---
> > > > > 
> > > > >  hw/device-assignment.c |  160 +++++++++++++++++++++++++++++++++++++++++++++---
> > > > >  1 files changed, 149 insertions(+), 11 deletions(-)
> > > > > 
> > > > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > > > > index 179c7dc..1b228ad 100644
> > > > > --- a/hw/device-assignment.c
> > > > > +++ b/hw/device-assignment.c
> > > > > @@ -366,6 +366,27 @@ static uint8_t assigned_dev_pci_read_byte(PCIDevice *d, int pos)
> > > > >      return (uint8_t)assigned_dev_pci_read(d, pos, 1);
> > > > >  }
> > > > >  
> > > > > +static void assigned_dev_pci_write(PCIDevice *d, int pos, uint32_t val, int len)
> > > > > +{
> > > > > +    AssignedDevice *pci_dev = container_of(d, AssignedDevice, dev);
> > > > > +    ssize_t ret;
> > > > > +    int fd = pci_dev->real_device.config_fd;
> > > > > +
> > > > > +again:
> > > > > +    ret = pwrite(fd, &val, len, pos);
> > > > > +    if (ret != len) {
> > > > > +	if ((ret < 0) && (errno == EINTR || errno == EAGAIN))
> > > > > +	    goto again;
> > > > 
> > > > 
> > > > do {} while() ?
> > > 
> > > Sure, this is just a copy of another place that does something similar.
> > > They should either be merged or both converted in a separate patch.
> > > 
> > > > > +
> > > > > +	fprintf(stderr, "%s: pwrite failed, ret = %zd errno = %d\n",
> > > > > +		__func__, ret, errno);
> > > > > +
> > > > > +	exit(1);
> > > > > +    }
> > > > > +
> > > > > +    return;
> > > > > +}
> > > > > +
> > > > >  static uint8_t pci_find_cap_offset(PCIDevice *d, uint8_t cap)
> > > > >  {
> > > > >      int id;
> > > > > @@ -1244,37 +1265,75 @@ static void assigned_dev_update_msix(PCIDevice *pci_dev, unsigned int ctrl_pos)
> > > > >  #endif
> > > > >  #endif
> > > > >  
> > > > > +static uint32_t assigned_device_pci_cap_read_config(PCIDevice *pci_dev,
> > > > > +                                                    uint8_t cap_id,
> > > > > +                                                    uint32_t address, int len)
> > > > > +{
> > > > > +    uint8_t cap;
> > > > > +
> > > > > +    switch (cap_id) {
> > > > > +
> > > > > +    case PCI_CAP_ID_VPD:
> > > > > +        cap = pci_find_capability(pci_dev, cap_id);
> > > > > +        if (address - cap >= PCI_CAP_FLAGS) {
> > > > > +            return assigned_dev_pci_read(pci_dev, address, len);
> > > > > +        }
> > > > > +        break;
> > > > > +
> > > > > +    case PCI_CAP_ID_VNDR:
> > > > > +        cap = pci_find_capability(pci_dev, cap_id);
> > > > > +        if (address - cap > PCI_CAP_FLAGS) {
> > > > > +            return assigned_dev_pci_read(pci_dev, address, len);
> > > > > +        }
> > > > > +        break;
> > > > > +    }
> > > > > +
> > > > > +    return pci_default_cap_read_config(pci_dev, cap_id, address, len);
> > > > > +}
> > > > > +
> > > > >  static void assigned_device_pci_cap_write_config(PCIDevice *pci_dev,
> > > > >                                                   uint8_t cap_id,
> > > > >                                                   uint32_t address,
> > > > >                                                   uint32_t val, int len)
> > > > >  {
> > > > > +    uint8_t cap;
> > > > > +
> > > > >      pci_default_cap_write_config(pci_dev, cap_id, address, val, len);
> > > > >  
> > > > >      switch (cap_id) {
> > > > >  #ifdef KVM_CAP_IRQ_ROUTING
> > > > >      case PCI_CAP_ID_MSI:
> > > > >  #ifdef KVM_CAP_DEVICE_MSI
> > > > > -        {
> > > > > -            uint8_t cap = pci_find_capability(pci_dev, cap_id);
> > > > > -            if (ranges_overlap(address - cap, len, PCI_MSI_FLAGS, 1)) {
> > > > > -                assigned_dev_update_msi(pci_dev, cap + PCI_MSI_FLAGS);
> > > > > -            }
> > > > > +        cap = pci_find_capability(pci_dev, cap_id);
> > > > > +        if (ranges_overlap(address - cap, len, PCI_MSI_FLAGS, 1)) {
> > > > > +            assigned_dev_update_msi(pci_dev, cap + PCI_MSI_FLAGS);
> > > > >          }
> > > > >  #endif
> > > > >          break;
> > > > >  
> > > > >      case PCI_CAP_ID_MSIX:
> > > > >  #ifdef KVM_CAP_DEVICE_MSIX
> > > > > -        {
> > > > > -            uint8_t cap = pci_find_capability(pci_dev, cap_id);
> > > > > -            if (ranges_overlap(address - cap, len, PCI_MSIX_FLAGS + 1, 1)) {
> > > > > -                assigned_dev_update_msix(pci_dev, cap + PCI_MSIX_FLAGS);
> > > > > -            }
> > > > > +        cap = pci_find_capability(pci_dev, cap_id);
> > > > > +        if (ranges_overlap(address - cap, len, PCI_MSIX_FLAGS + 1, 1)) {
> > > > > +            assigned_dev_update_msix(pci_dev, cap + PCI_MSIX_FLAGS);
> > > > >          }
> > > > >  #endif
> > > > >          break;
> > > > >  #endif
> > > > > +
> > > > > +    case PCI_CAP_ID_VPD:
> > > > > +        cap = pci_find_capability(pci_dev, cap_id);
> > > > > +        if (address - cap >= PCI_CAP_FLAGS) {
> > > > > +            assigned_dev_pci_write(pci_dev, address, val, len);
> > > > > +        }
> > > > > +        break;
> > > > > +
> > > > > +    case PCI_CAP_ID_VNDR:
> > > > > +        cap = pci_find_capability(pci_dev, cap_id);
> > > > > +        if (address - cap > PCI_CAP_FLAGS) {
> > > > > +            assigned_dev_pci_write(pci_dev, address, val, len);
> > > > > +        }
> > > > > +        break;
> > > > 
> > > > I have a feeling we should use overlap functions instead of
> > > > address math. What do you think?
> > > 
> > > if (!ranges_overlap(address - cap, len, 0, PCI_CAP_FLAGS)) ?
> > 
> > 	ranges_overlap(address, len, cap, PCI_CAP_FLAGS)
> > 
> > > Sure, that'd be a nice cleanup.
> > > 
> > > > Also - put cap offsets in assigned device structure to avoid
> > > > find calls?
> > > 
> > > I suppose there aren't enough capability IDs that it'd take much space
> > > to do so, but it doesn't sound like a unique to device assignment issue.
> > > Maybe that should live on PCIDevice with an access function.
> > 
> > Sure, I put all caps that we actually emulate in PCIDevice.
> > So that would apply to express, pcix, etc.
> > Sticking offsets to caps that core doesn't emulate in PCIDevice
> > seems a bit strange. That's why each device has its own device state.
> 
> The counter argument is that instead of sprinkling cap_msi, cap_msix,
> cap_pcie, cap_foo into PCIDevice as support gets added, it would add a
> lot of consistency to have a uint8_t caps[PCI_CAP_ID_MAX], then
> pci_find_capability simply becomes return pdev->caps[cap_id], and we can
> make more use of it.

Consider that express has 16 bit IDs too. That might make it a problem
if we try to use them as indexes.


-- 
MST