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 <m...@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 <m...@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
_______________________________________________
ipxe-devel mailing list
ipxe-devel@lists.ipxe.org
https://lists.ipxe.org/mailman/listinfo.cgi/ipxe-devel
