Re: [Xen-devel] [PATCH 1/2] vpci/msi: split code to bind pirq

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 14.05.18 at 16:15, <roger.pau@xxxxxxxxxx> wrote:
> On Mon, May 14, 2018 at 06:24:37AM -0600, Jan Beulich wrote:
>> >>> On 08.05.18 at 11:25, <roger.pau@xxxxxxxxxx> wrote:
>> > --- a/xen/arch/x86/hvm/vmsi.c
>> > +++ b/xen/arch/x86/hvm/vmsi.c
>> > @@ -663,6 +663,42 @@ void vpci_msi_arch_mask(struct vpci_msi *msi, const 
> struct pci_dev *pdev,
>> >      vpci_mask_pirq(pdev->domain, msi->arch.pirq + entry, mask);
>> >  }
>> >  
>> > +static int vpci_msi_update(const struct pci_dev *pdev, uint32_t data,
>> > +                           uint64_t address, unsigned int vectors,
>> > +                           unsigned int pirq, uint32_t mask)
>> > +{
>> > +    unsigned int i;
>> > +
>> > +    ASSERT(pcidevs_locked());
>> > +
>> > +    for ( i = 0; i < vectors; i++ )
>> > +    {
>> > +        uint8_t vector = MASK_EXTR(data, MSI_DATA_VECTOR_MASK);
>> > +        uint8_t vector_mask = 0xff >> (8 - fls(vectors) + 1);
>> > +        struct xen_domctl_bind_pt_irq bind = {
>> > +            .machine_irq = pirq + i,
>> > +            .irq_type = PT_IRQ_TYPE_MSI,
>> > +            .u.msi.gvec = (vector & ~vector_mask) |
>> > +                          ((vector + i) & vector_mask),
>> > +            .u.msi.gflags = msi_gflags(data, address, (mask >> i) & 1),
>> > +        };
>> > +        int rc = pt_irq_create_bind(pdev->domain, &bind);
>> > +
>> > +        if ( rc )
>> > +        {
>> > +            gdprintk(XENLOG_ERR,
>> > +                     "%04x:%02x:%02x.%u: failed to bind PIRQ %u: %d\n",
>> > +                     pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> > +                     PCI_FUNC(pdev->devfn), pirq + i, rc);
>> > +            while ( bind.machine_irq-- )
>> > +                pt_irq_destroy_bind(pdev->domain, &bind);
>> 
>> I realize this is just code movement, but is this while() correct? I think 
> it
>> can only be correct if pirq (which bind.machine_irq gets initialized from)
>> was always zero, yet that doesn't look to be the case.
>> 
>> If you agree, I'd prefer fixed code to be moved (read: wants a prereq
>> patch), or for the fix to be applied while moving the code (suitably
>> reasoned about in the description).
> 
> Right, this should be:
> 
> while ( bind.machine_irq-- >= pirq )
>     pt_irq_destroy_bind(pdev->domain, &bind);

">" you presumably mean, due to the post-decrement?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel