[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v4.1 4/6] xen/x86: Allow stubdom access to irq created for msi.



On Thu, Feb 21, 2019 at 06:40:40PM +0100, Marek Marczykowski-Górecki wrote:
> On Thu, Feb 21, 2019 at 05:47:51PM +0100, Roger Pau Monné wrote:
> > On Fri, Feb 08, 2019 at 11:17:05AM +0100, Marek Marczykowski-Górecki wrote:
> > >          return -EINVAL;
> > >      }
> > >  
> > > diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> > > index 8b44d6ce0b..d41b32b2f4 100644
> > > --- a/xen/arch/x86/irq.c
> > > +++ b/xen/arch/x86/irq.c
> > > @@ -157,7 +157,7 @@ int __init bind_irq_vector(int irq, int vector, const 
> > > cpumask_t *cpu_mask)
> > >  /*
> > >   * Dynamic irq allocate and deallocation for MSI
> > >   */
> > > -int create_irq(nodeid_t node)
> > > +int create_irq(nodeid_t node, struct domain *dm_domain)
> > 
> > Using plain 'd' would be fine for me here, same below for
> > destroy_irq.
> 
> I think it may be misleading, as the parameter is not about domain that
> will handle that IRQ, but what domain will get access to it.

Right, but dom0 will also end up using this function for it's own irqs
(irqs allocated and mapped to dom0), in which case naming this
parameter dm_domain is misleading. IMO it's better to just name it
'd', which is the domain that gets permissions over the created irq.

> > >  {
> > >      int irq, ret;
> > >      struct irq_desc *desc;
> > > @@ -190,19 +190,19 @@ int create_irq(nodeid_t node)
> > >          desc->arch.used = IRQ_UNUSED;
> > >          irq = ret;
> > >      }
> > > -    else if ( hardware_domain )
> > > +    else if ( dm_domain )
> > 
> > Can you guarantee that dm_domain is always current->domain?
> 
> No, in some cases it will be hardware_domain.

Right, but in that case current->domain == hardware_domain I guess?

> 
> > I think you need to assert for this, or else take a reference to
> > dm_domain (get_domain) before accessing it's fields, or else you risk
> > the domain being destroyed while modifying it's fields.
> 
> Can hardware_domain be destroyed without panicking Xen? If so,
> get_domain would be indeed needed.

What about other callers that are not the hardware_domain? You need to
make sure those domains are not destroyed while {create/destroy}_irq
is changing the permissions.

If you can guarantee that dm_domain == current->domain then you are
safe, if not you need to get a reference before modifying any fields
of the domain struct.

So IMO you either need to add an assert or a get_domain depending on
the answer to the question above.

> > >              break;
> > >          }
> > >      }
> > > diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
> > > index babc4147c4..66026e3ca5 100644
> > > --- a/xen/arch/x86/msi.c
> > > +++ b/xen/arch/x86/msi.c
> > > @@ -633,7 +633,7 @@ int __setup_msi_irq(struct irq_desc *desc, struct 
> > > msi_desc *msidesc,
> > >      return ret;
> > >  }
> > >  
> > > -int msi_free_irq(struct msi_desc *entry)
> > > +int msi_free_irq(struct msi_desc *entry, struct domain *dm_domain)
> > >  {
> > >      unsigned int nr = entry->msi_attrib.type != PCI_CAP_ID_MSIX
> > >                        ? entry->msi.nvec : 1;
> > > @@ -641,7 +641,7 @@ int msi_free_irq(struct msi_desc *entry)
> > >      while ( nr-- )
> > >      {
> > >          if ( entry[nr].irq >= 0 )
> > > -            destroy_irq(entry[nr].irq);
> > > +            destroy_irq(entry[nr].irq, dm_domain);
> > >  
> > >          /* Free the unused IRTE if intr remap enabled */
> > >          if ( iommu_intremap )
> > > @@ -1280,7 +1280,7 @@ static void msi_free_irqs(struct pci_dev* dev)
> > >      list_for_each_entry_safe( entry, tmp, &dev->msi_list, list )
> > >      {
> > >          pci_disable_msi(entry);
> > > -        msi_free_irq(entry);
> > > +        msi_free_irq(entry, hardware_domain);
> > 
> > This likely requires some comment to clarify why is the hardcoding of
> > hardware_domain correct here. AFAICT this will be called by
> > pci_remove_device, which I assume assures that the device has been
> > deassigned from any domain before attempting to remove it, hence it
> > can only have irqs assigned to the hardware_domain if any?
> 
> That's also my understanding, but I'm not 100% sure about that.
> See comment just bellow the commit message.

I would add an assert that pdev->domain == hardware_domain just to be
sure.

And then in pci_remove_device I think Xen should also check that
pdev->domain == hardware_domain or else refuse to remove the device.

Jan do you know whether pci_remove_device is supposed to be used
against devices assigned to a domain different than the hardware
domain?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.