Re: [Xen-devel] [PVH]: Help: msi.c

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 13 Dec 2012, Jan Beulich wrote:
> >>> On 13.12.12 at 13:19, Stefano Stabellini 
> >>> <stefano.stabellini@xxxxxxxxxxxxx>
> wrote:
> > On Thu, 13 Dec 2012, Jan Beulich wrote:
> >> >>> On 13.12.12 at 02:43, Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> >> > On Wed, 12 Dec 2012 17:15:23 -0800
> >> > Mukesh Rathor <mukesh.rathor@xxxxxxxxxx> wrote:
> >> > 
> >> >> On Tue, 11 Dec 2012 12:10:19 +0000
> >> >> Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> >> >> 
> >> >> > On Tue, 11 Dec 2012, Mukesh Rathor wrote:
> >> >> > > On Mon, 10 Dec 2012 09:43:34 +0000
> >> >> > > "Jan Beulich" <JBeulich@xxxxxxxx> wrote:
> >> >> > 
> >> >> > That's strange because AFAIK Linux is never editing the MSI-X
> >> >> > entries directly: give a look at
> >> >> > arch/x86/pci/xen.c:xen_initdom_setup_msi_irqs, Linux only remaps
> >> >> > MSIs into pirqs using hypercalls. Xen should be the only one to
> >> >> > touch the real MSI-X table.
> >> >> 
> >> >> So, this is what's happening. The side effect of :
> >> >> 
> >> >>         if ( rangeset_add_range(mmio_ro_ranges, dev->msix_table.first,
> >> >>                                 dev->msix_table.last) )
> >> >>             WARN();
> >> >>         if ( rangeset_add_range(mmio_ro_ranges, dev->msix_pba.first,
> >> >>                                 dev->msix_pba.last) )
> >> >>             WARN();
> >> >> 
> >> >> in msix_capability_init() in xen is that the dom0 EPT entries that
> >> >> I've mapped are going from RW to read only. Then when dom0 accesses
> >> >> it, I get EPT violation. In case of pure PV, the PTE entry to access
> >> >> the iomem is RW, and the above rangeset adding doesn't affect it. I
> >> >> don't understand why? Looking into that now...
> >> 
> >> As far as I was able to tell back at the time when I implemented
> >> this, existing code shouldn't have mappings for these tables in
> >> place at the time these ranges get added here. But I noted in
> >> the patch description that this is a potential issue (and may need
> >> fixing if deemed severe enough - back then, apparently nobody
> >> really cared, perhaps largely because passthrough to PV guests
> >> isn't considered fully secure anyway).
> >> 
> >> Now - did that change? I.e. can you describe where the mappings
> >> come from that cause the problem here?
> > 
> > The generic Linux MSI-X handling code does that, before calling the
> > arch specific msi setup function, for which we have a xen version
> > (xen_initdom_setup_msi_irqs):
> > 
> > pci_enable_msix -> msix_capability_init -> msix_map_region
> 
> Ah, okay, (of course?) I had looked only at the forward ported
> version of this. Is all that fiddling with the mask bits really
> being suppressed properly when running under Xen? Otherwise
> pv-ops is quite broken in this regard at present... And if it is,
> I don't see what the respective ioremap() is good for here in
> the Xen case.

Actually I think that you might be right: just looking at the code it
seems that the mask bits get written to the table once as part of the
initialization process:

pci_enable_msix -> msix_capability_init -> msix_program_entries

Unfortunately msix_program_entries is called few lines after
arch_setup_msi_irqs, where we call PHYSDEVOP_map_pirq to map the MSI as
a pirq.
However after that is done, all the masking/unmask is done via irq_mask
that we handle properly masking/unmasking the corresponding event
channels.


Possible solutions on top of my head:

- in msix_program_entries instead of writing to the table directly
(__msix_mask_irq), call desc->irq_data.chip->irq_mask(); 

- replace msix_program_entries with arch_msix_program_entries, but it
would probably be unpopular.

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