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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 14, 2017 at 04:19:44AM -0600, Jan Beulich wrote:
> >>> 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.

Oh, so the vector increment carries over to the delivery mode, then I
will switch it.

Thanks, Roger.

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