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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.09.17 at 12:42, <roger.pau@xxxxxxxxxx> wrote:
> 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.

But please double check first that I'm not mis-remembering.

Jan


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