Re: [PATCH v3 8/9] vpci/msi: add MSI handlers

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

>>> Roger Pau Monne <roger.pau@citrix.com> 06/27/17 12:23 PM >>>
>On Fri, May 26, 2017 at 09:26:03AM -0600, Jan Beulich wrote:
>> >>> On 27.04.17 at 16:35, <roger.pau@citrix.com> wrote:
>> > +    pinfo = pirq_info(current->domain, arch->pirq + entry);
>> > +    ASSERT(pinfo);
>> > +
>> > +    irq = pinfo->arch.irq;
>> > +    ASSERT(irq < nr_irqs);
>> > +
>> > +    desc = irq_to_desc(irq);
>> > +    ASSERT(desc);
>> 
>> It's not entirely clear to me where all the checks are which allow
>> the checks here to be ASSERT()s.
>
>Hm, this function is only called if the pirq is set (ie: if the
>interrupt is bound to the domain). AFAICT if Xen cannot get the irq or
>the desc related to this pirq it means that something/someone has
>unbound or changed the pirq under Xen's feet, and thus the expected
>state is no longer valid.
>
>I could add something like:
>
>if ( irq >= nr_irqs || irq < 0 )
>{
    >ASSERET_UNREACABLE();
    >return;
>}
>
>And the same for the desc check if that seems more sensible.

Well, if the function indeed is being called only when everything has already
been set up (and can't be torn down in a racy way), then I'm not overly
concerned which of the two forms you use. 

>> > +        pcidevs_lock();
>> > +        rc = pt_irq_create_bind(pdev->domain, &bind);
>> > +        if ( rc )
>> 
>> I don't think you need to hold the lock for the if() and its body. While
>> I see unmap_domain_pirq(), I don't really see why it does, so perhaps
>> there's some cleanup potential up front.
>
>unmap_domain_pirq might call into pci_disable_msi which I assume
>requires the pci lock to be hold (although has no assert to that
>effect).

Yeah, maybe it's indeed better to keep it. List access strictly requires
holding the lock, while I think we don't consistently hold the lock for
mere use of individual devices. If there was any real issue with that, I
think we'd rather need to refcount the devices.

>> > +static int vpci_msi_address_upper_write(struct pci_dev *pdev, unsigned int reg,
>> > +                                        union vpci_val val, void *data)
>> > +{
>> > +    struct vpci_msi *msi = data;
>> > +
>> > +    /* Clear high part. */
>> > +    msi->address &= ~GENMASK(63, 32);
>> > +    msi->address |= (uint64_t)val.double_word << 32;
>> 
>> Is the value written here actually being used for anything other than
>> for reading back (also applicable to the high bits of the low half of the
>> address)?
>
>It's used in a arch-specific way. But Xen needs to store it anyway, so
>the guest can read back whatever it writes. I have no idea what ARM
>might store here.

Hmm, I'm concerned you may introduce incorrect behavior here if the value
written is other than expected. But perhaps not much of a problem as long
as all this is Dom0 only.

Jan


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