[ipxe-devel] [PATCH v3 4/4] [virtio] Add virtio-net 1.0 support

[Michael S. Tsirkin]

On Tue, Apr 05, 2016 at 10:14:41PM +0200, Ladi Prosek wrote:
> On Tue, Apr 5, 2016 at 7:52 PM, Michael S. Tsirkin <mst at redhat.com> wrote:
> > On Tue, Apr 05, 2016 at 05:38:24PM +0200, Ladi Prosek wrote:
> >> +     /* Enable the PCI device only if we need to */
> >> +     if ( ( virtnet->vdev.common.flags & VIRTIO_PCI_REGION_TYPE_MASK ) !=
> >> +             VIRTIO_PCI_REGION_PCI_CONFIG ||
> >> +          ( virtnet->vdev.isr.flags & VIRTIO_PCI_REGION_TYPE_MASK ) !=
> >> +             VIRTIO_PCI_REGION_PCI_CONFIG ||
> >> +          ( device && ( virtnet->vdev.device.flags & VIRTIO_PCI_REGION_TYPE_MASK ) !=
> >> +             VIRTIO_PCI_REGION_PCI_CONFIG ) ) {
> >> +             /* At least one BAR has been mapped */
> >> +             adjust_pci_device ( pci );
> >> +             virtnet->vdev.pci_device_initialized = 1;
> >> +     }
> >
> >
> > This is slightly wrong for two reasons:
> >         - it does not enable bus mastering
> >         - unused BAR types are also enabled.
> >
> > How about using the following to enable exactly what's
> > needed:
> 
> I see, ok, I think I understand it now. I'm assuming that you also
> wanted to implement the existing adjust_pci_device in terms of the new
> function.
> 
> Would you mind if we did this separately as a follow up and kept the
> simple logic in v2 of this series which just called adjust_pci_device
> unconditionally? Your proposal will touch both modern and legacy code
> paths so it's not really virtio 1.0 specific and, more importantly,
> it's not making it worse than it currently is.

Yes, I agree.

> > Signed-off-by: Michael S. Tsirkin <mst at redhat.com>
> >
> > diff --git a/src/include/ipxe/pci.h b/src/include/ipxe/pci.h
> > index 89d9d80..e22ff54 100644
> > --- a/src/include/ipxe/pci.h
> > +++ b/src/include/ipxe/pci.h
> > @@ -278,6 +278,8 @@ struct pci_driver {
> >         PCI_FUNC ( (pci)->busdevfn )
> >
> >  extern void adjust_pci_device ( struct pci_device *pci );
> > +extern unsigned enable_pci_device ( struct pci_device *pci, unsigned flags );
> > +extern void disable_pci_device ( struct pci_device *pci, unsigned flags );
> >  extern unsigned long pci_bar_start ( struct pci_device *pci,
> >                                      unsigned int reg );
> >  extern int pci_read_config ( struct pci_device *pci );
> > diff --git a/src/drivers/bus/pci.c b/src/drivers/bus/pci.c
> > index 6fbedd9..6befb67 100644
> > --- a/src/drivers/bus/pci.c
> > +++ b/src/drivers/bus/pci.c
> > @@ -169,6 +169,49 @@ void adjust_pci_device ( struct pci_device *pci ) {
> >  }
> >
> >  /**
> > + * Enable PCI device
> > + *
> > + * @v pci              PCI device
> > + * @v flags            Flags to enable
> > + * @ret rc             Original flags
> > + *
> > + * Enable device in case BIOS neglected to do so.  Also
> > + * adjust PCI latency timer to a reasonable value, 32.
> > + */
> > +unsigned enable_pci_device ( struct pci_device *pci, unsigned flags ) {
> > +       unsigned short new_command, pci_command;
> > +       unsigned char pci_latency;
> > +
> > +       pci_read_config_word ( pci, PCI_COMMAND, &pci_command );
> > +       new_command = ( pci_command | flags );
> > +       if ( pci_command != new_command ) {
> > +               DBGC ( pci, PCI_FMT " device not enabled by BIOS! Updating "
> > +                      "PCI command %04x->%04x\n",
> > +                      PCI_ARGS ( pci ), pci_command, new_command );
> > +               pci_write_config_word ( pci, PCI_COMMAND, new_command );
> > +       }
> > +
> > +       pci_read_config_byte ( pci, PCI_LATENCY_TIMER, &pci_latency);
> > +       if ( pci_latency < 32 ) {
> > +               DBGC ( pci, PCI_FMT " latency timer is unreasonably low at "
> > +                      "%d. Setting to 32.\n", PCI_ARGS ( pci ), pci_latency );
> > +               pci_write_config_byte ( pci, PCI_LATENCY_TIMER, 32);
> > +       }
> 
> Missing return pci_command :-p
> 
> > +}
> > +
> > +/**
> > + * Disable PCI device
> > + *
> > + * @v pci              PCI device
> > + * @v flags            Flags to restore
> > + *
> > + * Restore device to state it had before we enabled it.
> > + */
> > +void disable_pci_device ( struct pci_device *pci, unsigned flags ) {
> > +       pci_write_config_word ( pci, PCI_COMMAND, flags );
> > +}
> > +
> > +/**
> >   * Read PCI device configuration
> >   *
> >   * @v pci              PCI device