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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 14.05.18 at 17:00, <roger.pau@citrix.com> wrote:
> On Mon, May 14, 2018 at 08:56:16AM -0600, Jan Beulich wrote:
>> >>> On 14.05.18 at 16:15, <roger.pau@citrix.com> wrote:
>> > On Mon, May 14, 2018 at 06:24:37AM -0600, Jan Beulich wrote:
>> >> >>> On 08.05.18 at 11:25, <roger.pau@citrix.com> 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?
> 
> Ended up doing --bind.machine_irq >= pirq, because it seemed clearer
> IMO.

Please don't: Even if in practice pirq can't be zero (I think), your variant
would degenerate into an infinite loop in that case.

Jan



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