Re: [Xen-devel] [PATCH v3 9/9] vpci/msix: add MSI-X handlers

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Roger Pau Monne <roger.pau@xxxxxxxxxx> 06/28/17 5:37 PM >>>
>On Mon, May 29, 2017 at 07:29:29AM -0600, Jan Beulich wrote:
>> >>> On 27.04.17 at 16:35, <roger.pau@xxxxxxxxxx> wrote:
>> > +    switch ( offset )
>> > +    {
>> > +    case PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET:
>> > +        *data = entry->addr;
>> 
>> You're not clipping off the upper 32 bits here - is that reliably
>> happening elsewhere?
>
>This could be a 64b access, so I though there was no need to
>differentiate between 32/64b in this case, since the underlying
>handlers will already clip it when needed. I could switch it to:
>
>if ( len == 8 )
    >*data = entry->addr;
>else
    >*data = (uint32_t)entry->addr;
>
>I don't think it's happening elsewhere, but I will try to check. Is
>that really an issue?

I would hope it isn't, but I'm not 100% certain, hence my request to at
least check. I agree that it would be nice to not have to do it here. All other
similar read routines I've looked at appear to leave the upper half as zero,
albeit that's always a result of the way the code is written instead of an
explicit truncation as would be required here.

>> > +        break;
>> > +    case PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET:
>> > +        *data = entry->addr >> 32;
>> > +        break;
>> > +    case PCI_MSIX_ENTRY_DATA_OFFSET:
>> > +        *data = entry->data;
>> > +        break;
>> > +    case PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET:
>> > +        *data = entry->masked ? PCI_MSIX_VECTOR_BITMASK : 0;
>> 
>> What about the other 31 bits?
>
>They are all marked as "reserved" in my copy of the PCI spec.

Indeed, but it's at least worth considering to pass through the values (as it's
Dom0 we're talking about here), and having a comment giving a brief explanation
for the choice.

>> > +    /* Find the MSI-X table address. */
>> > +    msix->offset = pci_conf_read32(seg, bus, slot, func,
>> > +                                   msix_table_offset_reg(msix_offset));
>> > +    msix->bir = msix->offset & PCI_MSIX_BIRMASK;
>> > +    msix->offset &= ~PCI_MSIX_BIRMASK;
>> > +
>> > +    ASSERT(pdev->vpci->header.bars[msix->bir].type == VPCI_BAR_MEM ||
>> > +           pdev->vpci->header.bars[msix->bir].type == VPCI_BAR_MEM64_LO);
>> > +    msix->addr = pdev->vpci->header.bars[msix->bir].mapped_addr + 
>> > msix->offset;
>> > +    msix_paddr = pdev->vpci->header.bars[msix->bir].paddr + msix->offset;
>> 
>> I can't seem to find where these addresses get updated in case
>> the BARs are being relocated by the Dom0 kernel.
>
>Is that something that Xen should support? I got the impression that
>the current MSI-X code in Xen didn't support relocating the BARs that
>contain the MSI-X table (but maybe I got it wrong).

Well, the current expectation is that Dom0 would do BAR relocation prior to any 
MSI-X
setup. But since you aim at maximum transparency from PVH Dom0's perspective, 
I'm
not certain latching the addresses here once and for all is sufficient.

>> > +static void vpci_dump_msix(unsigned char key)
>> > +{
>> > +    struct domain *d;
>> > +    struct pci_dev *pdev;
>> > +
>> > +    printk("Guest MSI-X information:\n");
>> > +
>> > +    for_each_domain ( d )
>> > +    {
>> > +        if ( !has_vpci(d) )
>> > +            continue;
>> > +
>> > +        vpci_lock(d);
>> 
>> Dump handlers, even if there are existing examples to the contrary,
>> should only try-lock any locks they mean to hold (and not dump
>> anything if they can't get hold of the lock).
>
>OK, will have to change the MSI dump also. Since you commented on the
>MSI dump handler, I guess this output should also be appended to the
>'M' key?

Yes, that indeed was the implication.

Jan


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