Re: [Xen-devel] [PATCH v8 4/7] VT-d: Use one function to update both remapped and posted IRTE

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Tian, Kevin
> Sent: Friday, November 18, 2016 12:31 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: jbeulich@xxxxxxxx; andrew.cooper3@xxxxxxxxxx;
> george.dunlap@xxxxxxxxxxxxx; dario.faggioli@xxxxxxxxxx
> Subject: RE: [PATCH v8 4/7] VT-d: Use one function to update both remapped
> and posted IRTE
> 
> > From: Wu, Feng
> > Sent: Friday, November 18, 2016 9:57 AM
> >
> > Use one function to update both remapped IRTE and posted IRET.
> >
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > ---
> > v8:
> > - Newly added
> >
> >  xen/drivers/passthrough/vtd/intremap.c | 162 
> > ++++++++++++++-----------------
> --
> >  1 file changed, 66 insertions(+), 96 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/vtd/intremap.c
> > b/xen/drivers/passthrough/vtd/intremap.c
> > index bfd468b..fd2a49a 100644
> > --- a/xen/drivers/passthrough/vtd/intremap.c
> > +++ b/xen/drivers/passthrough/vtd/intremap.c
> > @@ -420,7 +420,7 @@ void io_apic_write_remap_rte(
> >          __ioapic_write_entry(apic, ioapic_pin, 1, old_rte);
> >  }
> >
> > -static void set_msi_source_id(struct pci_dev *pdev, struct iremap_entry 
> > *ire)
> > +static void set_msi_source_id(const struct pci_dev *pdev, struct
> iremap_entry *ire)
> >  {
> >      u16 seg;
> >      u8 bus, devfn, secbus;
> > @@ -548,11 +548,12 @@ static int remap_entry_to_msi_msg(
> >  }
> >
> >  static int msi_msg_to_remap_entry(
> > -    struct iommu *iommu, struct pci_dev *pdev,
> > -    struct msi_desc *msi_desc, struct msi_msg *msg)
> > +    struct iommu *iommu, const struct pci_dev *pdev,
> > +    struct msi_desc *msi_desc, struct msi_msg *msg,
> > +    const struct pi_desc *pi_desc, const uint8_t gvec)
> >  {
> >      struct iremap_entry *iremap_entry = NULL, *iremap_entries;
> > -    struct iremap_entry new_ire;
> > +    struct iremap_entry new_ire, old_ire;
> 
> old_ire is used only in later 'else' branch. move definition there.
> 
> >      struct msi_msg_remap_entry *remap_rte;
> >      unsigned int index, i, nr = 1;
> >      unsigned long flags;
> > @@ -597,31 +598,50 @@ static int msi_msg_to_remap_entry(
> >
> >      memcpy(&new_ire, iremap_entry, sizeof(struct iremap_entry));
> >
> > -    /* Set interrupt remapping table entry */
> > -    new_ire.remap.fpd = 0;
> > -    new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT)
> & 0x1;
> > -    new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
> > -    new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT) &
> 0x1;
> > -    /* Hardware require RH = 1 for LPR delivery mode */
> > -    new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
> > -    new_ire.remap.avail = 0;
> > -    new_ire.remap.res_1 = 0;
> > -    new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
> > -                            MSI_DATA_VECTOR_MASK;
> > -    new_ire.remap.res_2 = 0;
> > -    if ( x2apic_enabled )
> > -        new_ire.remap.dst = msg->dest32;
> > +    if ( !pi_desc )
> > +    {
> > +        /* Set interrupt remapping table entry */
> > +        new_ire.remap.fpd = 0;
> > +        new_ire.remap.dm = (msg->address_lo >> MSI_ADDR_DESTMODE_SHIFT)
> &
> > 0x1;
> > +        new_ire.remap.tm = (msg->data >> MSI_DATA_TRIGGER_SHIFT) & 0x1;
> > +        new_ire.remap.dlm = (msg->data >> MSI_DATA_DELIVERY_MODE_SHIFT)
> &
> > 0x1;
> > +        /* Hardware require RH = 1 for LPR delivery mode */
> > +        new_ire.remap.rh = (new_ire.remap.dlm == dest_LowestPrio);
> > +        new_ire.remap.avail = 0;
> > +        new_ire.remap.res_1 = 0;
> > +        new_ire.remap.vector = (msg->data >> MSI_DATA_VECTOR_SHIFT) &
> > +                                MSI_DATA_VECTOR_MASK;
> > +        new_ire.remap.res_2 = 0;
> > +        if ( x2apic_enabled )
> > +            new_ire.remap.dst = msg->dest32;
> > +        else
> > +            new_ire.remap.dst = ((msg->address_lo >> 
> > MSI_ADDR_DEST_ID_SHIFT)
> > +                                 & 0xff) << 8;
> > +
> > +        new_ire.remap.res_3 = 0;
> > +        new_ire.remap.res_4 = 0;
> > +        new_ire.remap.p = 1;    /* finally, set present bit */
> > +    }
> >      else
> > -        new_ire.remap.dst = ((msg->address_lo >> MSI_ADDR_DEST_ID_SHIFT)
> > -                             & 0xff) << 8;
> > +    {
> > +        new_ire.post.fpd = 0;
> > +        new_ire.post.res_1 = 0;
> > +        new_ire.post.res_2 = 0;
> > +        new_ire.post.urg = 0;
> > +        new_ire.post.im = 1;
> > +        new_ire.post.vector = gvec;
> > +        new_ire.post.res_3 = 0;
> > +        new_ire.post.res_4 = 0;
> > +        new_ire.post.res_5 = 0;
> > +        new_ire.post.pda_l = virt_to_maddr(pi_desc) >> (32 - PDA_LOW_BIT);
> > +        new_ire.post.pda_h = virt_to_maddr(pi_desc) >> 32;
> > +        new_ire.post.p = 1;    /* finally, set present bit */
> > +    }
> 
> It's a little confusing here:
> 
> - you first copy current remapping entry to new_ire and then do some
> initialization.
> Is it necessary? If there are some fields we'd like to inherit, better to 
> describe
> them
> in comment so others can have a clear understanding of what exactly fields
> must
> be updated here;

This is needed, please refer to my replay to [5/7].

> 
> - in original code of setup_posted_irte, you actually do conditional 
> assignment
> based on format of old_ire. Above you changed to non-conditional assignment,
> not relying on old_ire. Is it desired?

In setup_posted_irte() we need to inherit the common fields (fpd, sid, sq, svt)
from the old IRTE. But now this common fields are setup by set_msi_source_id()
in this function.

Thanks,
Feng

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