Re: [PATCH 11/18 V2]: PVH xen: some misc changes like mtrr, intr, msi.

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Mar 19, 2013 at 08:55:20AM +0000, Jan Beulich wrote:
> >>> On 19.03.13 at 02:20, Mukesh Rathor <mukesh.rathor@oracle.com> wrote:
> > On Mon, 18 Mar 2013 12:21:11 +0000
> > "Jan Beulich" <JBeulich@suse.com> wrote:
> > 
> >>  >>> On 16.03.13 at 01:46, Mukesh Rathor <mukesh.rathor@oracle.com>
> >>  >>> wrote:
> >> > +++ b/xen/arch/x86/msi.c
> >> > @@ -787,10 +787,15 @@ static int msix_capability_init(struct
> >> > pci_dev *dev, 
> >> >      if ( !dev->msix_used_entries )
> >> >      {
> >> > -        if ( rangeset_add_range(mmio_ro_ranges,
> >> > dev->msix_table.first,
> >> > +        /* PVH: this is temporary only until linux msi.c is fixed.
> >> > See xen-devel
> >> > +         * thread: "[PVH]: Help: msi.c".
> >> > +         */
> >> > +        if ( !is_pvh_domain(dev->domain) &&
> >> > +             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,
> >> > +        if ( !is_pvh_domain(dev->domain) &&
> >> > +             rangeset_add_range(mmio_ro_ranges,
> >> > dev->msix_pba.first, dev->msix_pba.last) )
> >> >              WARN();
> >> >  
> >> 
> >> As already said before, the whole change above has my explicit
> >> NAK.
> >> Jan
> > 
> > Do you have any suggestions? Do you want to hold off on entire xen patch
> > until we go fix linux for this? Or can we just omit this change in the next
> > V 3 and come back to this later.  Would you be OK with that?
> 
> If you can't get MSI (or perhaps just MSI-X) to work properly
> for PVH, failing the respective setup operations just for PVH
> guests would be the right approach - the guest has to be
> prepared to run without MSI generally anyway (minus devices
> that have no interrupt pin at all of course). Opening security
> holes certainly isn't acceptable, even for code that is considered
> experimental only (mostly because it way too easily happens
> that the experimental status then gets dropped without
> remembering all the pieces that need fixing).

And it looks like it can actually work

[   19.931793] atl1c 0000:03:00.0: xen map irq failed -22 for 32752 domain
[   19.938464] atl1c 0000:03:00.0: Unable to allocate MSI interrupt Error: -22
[   19.945989] atl1c 0000:03:00.0: atl1c: eth0 NIC Link is Up<1000 Mbps Full Duplex>

Albeit the Linux code looks to have a bug.

(This is with a really silly code):

diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 876ac9d..2571e6b 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -19,6 +19,9 @@
 #include <xsm/xsm.h>
 #include <asm/p2m.h>
 
+static bool_t msi_enabled  = 1;
+boolean_param("msi_enabled", msi_enabled);
+
 int physdev_map_pirq(domid_t, int type, int *index, int *pirq_p,
                      struct msi_info *);
 int physdev_unmap_pirq(domid_t, int pirq);
@@ -140,6 +143,9 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
         break;
 
     case MAP_PIRQ_TYPE_MSI:
+        if ( !msi_enabled )
+            goto out;
+
         irq = *index;
         if ( irq == -1 )
             irq = create_irq(NUMA_NO_NODE);
@@ -157,6 +163,7 @@ int physdev_map_pirq(domid_t domid, int type, int *index, int *pirq_p,
         break;
 
     default:
+out:
         dprintk(XENLOG_G_ERR, "dom%d: wrong map_pirq type %x\n",
                 d->domain_id, type);
         ret = -EINVAL;