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

[Ladi Prosek]

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.

> 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