Re: [Qemu-devel] [PATCH V5 17/18] pci: remove hard-coded bar size in msix_init_exclusive_bar()

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

On Wed, Apr 01, 2015 at 06:12:18PM +0800, Jason Wang wrote:
> >> diff --git a/hw/pci/msix.c b/hw/pci/msix.c
> >> index 24de260..8c6d8f3 100644
> >> --- a/hw/pci/msix.c
> >> +++ b/hw/pci/msix.c
> >> @@ -291,33 +291,44 @@ int msix_init(struct PCIDevice *dev, unsigned
> >>short nentries,
> >>  }
> >>     int msix_init_exclusive_bar(PCIDevice *dev, unsigned short
> >>nentries,
> >> -                            uint8_t bar_nr)
> >> +                            uint8_t bar_nr, bool legacy_layout)
> >>  {
> >>      int ret;
> >>      char *name;
> >> -
> >> -    /*
> >> -     * Migration compatibility dictates that this remains a 4k
> >> -     * BAR with the vector table in the lower half and PBA in
> >> -     * the upper half.  Do not use these elsewhere!
> >> -     */
> >> -#define MSIX_EXCLUSIVE_BAR_SIZE 4096
> >> -#define MSIX_EXCLUSIVE_BAR_TABLE_OFFSET 0
> >> -#define MSIX_EXCLUSIVE_BAR_PBA_OFFSET (MSIX_EXCLUSIVE_BAR_SIZE / 2)
> >> -#define MSIX_EXCLUSIVE_CAP_OFFSET 0
> >> -
> >> -    if (nentries * PCI_MSIX_ENTRY_SIZE >
> >>MSIX_EXCLUSIVE_BAR_PBA_OFFSET) {
> >> -        return -EINVAL;
> >> +    uint32_t bar_size;
> >> +    uint32_t bar_pba_offset;
> >> +
> >> +    if (legacy_layout) {
> >> +        /*
> >> +         * Migration compatibility dictates that this remains a 4k
> >> +         * BAR with the vector table in the lower half and PBA in
> >> +         * the upper half.  Do not use these elsewhere!
> >> +         */
> >> +        bar_size = 4096;
> >> +        bar_pba_offset = bar_size / 2;
> >> +
> >> +        if (nentries * PCI_MSIX_ENTRY_SIZE > bar_pba_offset) {
> >> +            return -EINVAL;
> >> +        }
> >
> >So this takes pains to behave in a compatible
> >way when more than 128 vectors are requested.
> >But since we only had up to 64 VQs, why does
> >some a configuration make sense?
> 
> This question could also be asked for the code even without this patch. Spec
> does not clarify this and if we think vectors>=128 is not a valid
> configuration with only 64 VQs, qemu should fail and quit instead of a
> warning. Unfortunately we don't do this and leave a chance for user to use
> it.

I agree with this as a general design principle.
And it's typically even better to just support what
the user requested, even if it doesn't make sense.

-- 
MST