>>> On 14.09.17 at 12:42, <roger....@citrix.com> wrote:
> On Thu, Sep 14, 2017 at 04:19:44AM -0600, Jan Beulich wrote:
>> >>> On 14.09.17 at 12:08, <roger....@citrix.com> wrote:
>> > On Thu, Sep 07, 2017 at 09:29:41AM -0600, Jan Beulich wrote:
>> >> >>> On 14.08.17 at 16:28, <roger....@citrix.com> wrote:
>> >> > +int vpci_msi_arch_enable(struct vpci_arch_msi *arch, struct pci_dev
> *pdev,
>> >> > + uint64_t address, uint32_t data, unsigned int
> vectors)
>> >> > +{
>> >> > + struct msi_info msi_info = {
>> >> > + .seg = pdev->seg,
>> >> > + .bus = pdev->bus,
>> >> > + .devfn = pdev->devfn,
>> >> > + .entry_nr = vectors,
>> >> > + };
>> >> > + unsigned int i;
>> >> > + int rc;
>> >> > +
>> >> > + ASSERT(arch->pirq == INVALID_PIRQ);
>> >> > +
>> >> > + /* Get a PIRQ. */
>> >> > + rc = allocate_and_map_msi_pirq(pdev->domain, -1, &arch->pirq,
>> >> > + MAP_PIRQ_TYPE_MULTI_MSI, &msi_info);
>> >> > + if ( rc )
>> >> > + {
>> >> > + gdprintk(XENLOG_ERR, "%04x:%02x:%02x.%u: failed to map PIRQ:
> %d\n",
>> >> > + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> >> > + PCI_FUNC(pdev->devfn), rc);
>> >> > + return rc;
>> >> > + }
>> >> > +
>> >> > + for ( i = 0; i < vectors; i++ )
>> >> > + {
>> >> > + xen_domctl_bind_pt_irq_t bind = {
>> >> > + .machine_irq = arch->pirq + i,
>> >> > + .irq_type = PT_IRQ_TYPE_MSI,
>> >> > + .u.msi.gvec = msi_vector(data) + i,
>> >>
>> >> Isn't that rather msi_vector(data + i), i.e. wouldn't you better
>> >> increment data together with i?
>> >
>> > That's true, because the vector is fetched from the last 8bits of the
>> > data, but I find it more confusing (and it requires that the reader
>> > knows this detail). IMHO I would prefer to leave it as-is.
>>
>> No, the problem is the wrap-around case, which your code
>> doesn't handle. Iirc hardware behaves along the lines of what
>> I've suggested to change to, with potentially the vector
>> increment carrying into other parts of the value. Hence you
>> either need an early check for there not being any wrapping,
>> or other places may need similar adjustment (in which case it
>> might be better to really just increment "data" once in the
>> loop.
>
> Oh, so the vector increment carries over to the delivery mode, then I
> will switch it.
But please double check first that I'm not mis-remembering.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
