Re: [Qemu-devel] [PATCH v3] pci : Add pba_offset PCI quirk for Chelsio T5 devices

[Alex Williamson <alex.williamson@redhat.com>]

On Wed, 2015-07-01 at 18:10 +0000, Gabriel Laupre wrote:
> > What you are suggesting is:
> > If table_offset is not as expected, then check if it's a chelsio device.
> > If it's not, then print a message. On the other hand, if it's a chelsio device, then let msix_init() catch the error. Why ? And if we are sure that msix_init will error out, what's the purpose of the table_offset check ?
> 
> The test needs only to check the pba_offset and reduced as following is enough
> 
> ...
>     /*   
>      * Test the size of the pba_offset variable and catch if it extends outside
>      * of the specified BAR. If it is the case, we need to apply a hardware
>      * specific quirk if the device is known or we have a broken configuration.
>      */
>     if (vdev->msix->pba_offset >=
>         vdev->bars[vdev->msix->pba_bar].region.size) {
> 
>         PCIDevice *pdev = &vdev->pdev;
>         uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
>         uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
> 
>         /*
>          * Chelsio T5 Virtual Function devices are encoded as 0x58xx for T5
>          * adapters. The T5 hardware returns an incorrect value of 0x8000 for
>          * the VF PBA offset while the BAR itself is only 8k. The correct value
>          * is 0x1000, so we hard code that here.
>          */
>         if (vendor == PCI_VENDOR_ID_CHELSIO && (device & 0xff00) == 0x5800) {
>             vdev->msix->pba_offset = 0x1000;
>         } else {
>             error_report("vfio: Hardware reports invalid configuration, "
>                          "MSIX data outside of specified BAR");
>             return -EINVAL;
>         }
>     }  
> ...
> 
> As the hardware problem is only related with the pba_offset and the purpose of the quirk is to correct the known hardware error. The table_offset has never been seen as wrong. Therefore the msix_init() sanity check should take care of a "rare" potential error as you mentioned. 
> 
> This time I'll wait for ACKs from your side before submitting a new version :)

I would s/data/PBA/ in the error_report text for this version.  Thanks,

Alex

> -----Original Message-----
> From: Bandan Das [mailto:bsd@redhat.com] 
> Sent: Tuesday, June 30, 2015 6:48 PM
> To: Gabriel Laupre
> Cc: jb-gnumlists@wisemo.com; Casey Leedom; mst@redhat.com; qemu-devel@nongnu.org; Anish Bhatt; Michael Boksanyi; alex.williamson@redhat.com; bsd@makefile.in
> Subject: Re: [Qemu-devel] [PATCH v3] pci : Add pba_offset PCI quirk for Chelsio T5 devices
> 
> Gabriel Laupre <glaupre@chelsio.com> writes:
> 
> > @ Bandan ...
> >> > + + /* Chelsio T5 Virtual Function devices are encoded as 0x58xx
> >> > for T5 + * adapters. The T5 hardware returns an incorrect value of
> >> > 0x8000 for + * the VF PBA offset. The correct value is 0x1000, so 
> >> > we hard code that + * here. */ + if (vendor == 
> >> > PCI_VENDOR_ID_CHELSIO && (device & 0xff00) == 0x5800) { +
> >> > vdev->msix->pba_offset = 0x1000;
> >
> >> For the rare case where table_offset is wrong for the device being
> > checked for above and pba_offset is actually correct, shouldn't we 
> > fail ?
> >
> > I don't know if it is relevant to do all the tests here because in the 
> > function msix_init() all size are checked. I would prefer keeping this 
> > test as this to simplify the quirk, i.e. just testing the device 
> > first, and if another size than the pba_offset is wrong, then the 
> > sanity check in the function msix_init() will catch the error.
> 
> Ok, here's the excerpt:
> 
> +    /* Test the size of the pba variables and catch if they extend outside of
> +     * the specified BAR. If it is the case, we have a broken configuration or
> +     * we need to apply a hardware specific quirk. */
> +    if (vdev->msix->table_offset >=
> +        vdev->bars[vdev->msix->table_bar].region.size ||
> +        vdev->msix->pba_offset >=
> +        vdev->bars[vdev->msix->pba_bar].region.size) {
> +
> +        PCIDevice *pdev = &vdev->pdev;
> +        uint16_t vendor = pci_get_word(pdev->config + PCI_VENDOR_ID);
> +        uint16_t device = pci_get_word(pdev->config + PCI_DEVICE_ID);
> +
> +        /* Chelsio T5 Virtual Function devices are encoded as 0x58xx for T5
> +         * adapters. The T5 hardware returns an incorrect value of 0x8000 for
> +         * the VF PBA offset. The correct value is 0x1000, so we hard code that
> +         * here. */
> +        if (vendor == PCI_VENDOR_ID_CHELSIO && (device & 0xff00) == 0x5800) {
> +            vdev->msix->pba_offset = 0x1000;
> +        } else {
> +            error_report("vfio: Hardware reports invalid configuration, "
> +            "MSIX data outside of specified BAR");
> +            return -EINVAL;
> +        }
> +    }
> 
> 
> What you are suggesting is:
> If table_offset is not as expected, then check if it's a chelsio device.
> If it's not, then print a message. On the other hand, if it's a chelsio device, then let msix_init() catch the error. Why ? And if we are sure that msix_init will error out, what's the purpose of the table_offset check ?
> 
> > @ Alex I corrected what you pointed out. I will send the patch v4 in a 
> > minute.
> >
> > Thanks you
> >
> > Gabriel