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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jun 29, 2017 at 12:19:39AM -0600, Jan Beulich wrote:
> >>> 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.

Ack, I will add a comment to clarify this.

> >> > +        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.

I can certainly do that, but I don't think we should passthrough them
in the write handler. I'm worried that then Dom0 would see an
incoherent value if it attempts to modify some of the bits marked as
reserved.

IMHO it seems better to simply hide them until Xen knows how to deal
with them.

> >> > +    /* 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.

OK, I can implement something a little bit more flexible.

Thanks, Roger.

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