Re: [Xen-devel] [PATCH v5 09/11] vpci/msi: add MSI handlers

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.17 at 12:08, <roger.pau@xxxxxxxxxx> wrote:
> On Thu, Sep 07, 2017 at 09:29:41AM -0600, Jan Beulich wrote:
>> >>> On 14.08.17 at 16:28, <roger.pau@xxxxxxxxxx> 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.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel