Re: [Xen-devel] [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used

2015-06-15 Thread Wu, Feng 


 -Original Message-
 From: Jan Beulich [mailto:jbeul...@suse.com]
 Sent: Friday, June 12, 2015 6:35 PM
 To: Wu, Feng
 Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin;
 Zhang, Yang Z; xen-devel@lists.xen.org; k...@xen.org
 Subject: RE: [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used
 
  On 12.06.15 at 11:40, feng...@intel.com wrote:
  -Original Message-
  From: Jan Beulich [mailto:jbeul...@suse.com]
   On 08.05.15 at 11:07, feng...@intel.com wrote:
   +static inline void setup_posted_irte(
   +struct iremap_entry *new_ire, struct pi_desc *pi_desc, uint8_t
 gvec)
   +{
   +new_ire-post.urg = 0;
   +new_ire-post.vector = gvec;
   +new_ire-post.pda_l = (((u64)virt_to_maddr(pi_desc)) 
   +   (32 - PDA_LOW_BIT)) 
 PDA_MASK(LOW);
   +new_ire-post.pda_h = (((u64)virt_to_maddr(pi_desc))  32) 
   +   PDA_MASK(HIGH);
 
  Looking at this another time I can't see what the and-ing with
  PAD_MASK() is supposed to be good for.
 
  I cannot understand this well. Do you mean we don't need and PDA_MASK()
  here?
 
 Correct - the bitfield width (where the data gets stored into) already
 enforces the intended truncation afaict.

We may not need PDA_MASK(HIGH), but is PDA_MASK(LOW) really unnecessary here?

Thanks,
Feng

 
 Jan


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

Re: [Xen-devel] [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used

2015-06-15 Thread Wu, Feng 


 -Original Message-
 From: Jan Beulich [mailto:jbeul...@suse.com]
 Sent: Monday, June 15, 2015 5:35 PM
 To: Wu, Feng
 Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin;
 Zhang, Yang Z; xen-devel@lists.xen.org; k...@xen.org
 Subject: RE: [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used
 
  On 15.06.15 at 11:20, feng...@intel.com wrote:
 
 
  -Original Message-
  From: Jan Beulich [mailto:jbeul...@suse.com]
  Sent: Friday, June 12, 2015 6:35 PM
  To: Wu, Feng
  Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin;
  Zhang, Yang Z; xen-devel@lists.xen.org; k...@xen.org
  Subject: RE: [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is 
  used
 
   On 12.06.15 at 11:40, feng...@intel.com wrote:
   -Original Message-
   From: Jan Beulich [mailto:jbeul...@suse.com]
On 08.05.15 at 11:07, feng...@intel.com wrote:
+static inline void setup_posted_irte(
+struct iremap_entry *new_ire, struct pi_desc *pi_desc, uint8_t
  gvec)
+{
+new_ire-post.urg = 0;
+new_ire-post.vector = gvec;
+new_ire-post.pda_l = (((u64)virt_to_maddr(pi_desc)) 
+   (32 - PDA_LOW_BIT)) 
  PDA_MASK(LOW);
+new_ire-post.pda_h = (((u64)virt_to_maddr(pi_desc))  32) 
+   PDA_MASK(HIGH);
  
   Looking at this another time I can't see what the and-ing with
   PAD_MASK() is supposed to be good for.
  
   I cannot understand this well. Do you mean we don't need and
 PDA_MASK()
   here?
 
  Correct - the bitfield width (where the data gets stored into) already
  enforces the intended truncation afaict.
 
  We may not need PDA_MASK(HIGH), but is PDA_MASK(LOW) really
 unnecessary
  here?
 
 I think so - iirc the bitfield is exactly the width you need (or if it
 wasn't, that would be a mistake). What you can't get rid of is the
 shifting.

You are right. I got the same result after removing the PDA_MASK.

Thanks,
Feng

 
 Jan


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

Re: [Xen-devel] [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used

2015-06-15 Thread Jan Beulich 
 On 15.06.15 at 11:20, feng...@intel.com wrote:

 
 -Original Message-
 From: Jan Beulich [mailto:jbeul...@suse.com]
 Sent: Friday, June 12, 2015 6:35 PM
 To: Wu, Feng
 Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin;
 Zhang, Yang Z; xen-devel@lists.xen.org; k...@xen.org 
 Subject: RE: [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used
 
  On 12.06.15 at 11:40, feng...@intel.com wrote:
  -Original Message-
  From: Jan Beulich [mailto:jbeul...@suse.com]
   On 08.05.15 at 11:07, feng...@intel.com wrote:
   +static inline void setup_posted_irte(
   +struct iremap_entry *new_ire, struct pi_desc *pi_desc, uint8_t
 gvec)
   +{
   +new_ire-post.urg = 0;
   +new_ire-post.vector = gvec;
   +new_ire-post.pda_l = (((u64)virt_to_maddr(pi_desc)) 
   +   (32 - PDA_LOW_BIT)) 
 PDA_MASK(LOW);
   +new_ire-post.pda_h = (((u64)virt_to_maddr(pi_desc))  32) 
   +   PDA_MASK(HIGH);
 
  Looking at this another time I can't see what the and-ing with
  PAD_MASK() is supposed to be good for.
 
  I cannot understand this well. Do you mean we don't need and PDA_MASK()
  here?
 
 Correct - the bitfield width (where the data gets stored into) already
 enforces the intended truncation afaict.
 
 We may not need PDA_MASK(HIGH), but is PDA_MASK(LOW) really unnecessary 
 here?

I think so - iirc the bitfield is exactly the width you need (or if it
wasn't, that would be a mistake). What you can't get rid of is the
shifting.

Jan


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

Re: [Xen-devel] [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used

2015-06-12 Thread Wu, Feng 


 -Original Message-
 From: Jan Beulich [mailto:jbeul...@suse.com]
 Sent: Tuesday, June 09, 2015 10:33 PM
 To: Wu, Feng
 Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin;
 Zhang, Yang Z; xen-devel@lists.xen.org; k...@xen.org
 Subject: Re: [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used
 
  On 08.05.15 at 11:07, feng...@intel.com wrote:
  +bool_t pi_update_irte(struct vcpu *v, struct pirq *pirq, uint8_t gvec)
 
 Without seeing the caller right away it's hard to judge, but generally I'd
 prefer functions to return -E... values as error indicators, i.e.
 
  +{
  +struct irq_desc *desc;
  +struct msi_desc *msi_desc;
  +int remap_index;
  +bool_t rc = 0;
  +struct pci_dev *pci_dev;
  +struct acpi_drhd_unit *drhd;
  +struct iommu *iommu;
  +struct ir_ctrl *ir_ctrl;
  +struct iremap_entry *iremap_entries = NULL, *p = NULL;
  +struct iremap_entry new_ire;
  +struct pi_desc *pi_desc = v-arch.hvm_vmx.pi_desc;
  +unsigned long flags;
  +
  +desc = pirq_spin_lock_irq_desc(pirq, NULL);
  +if ( !desc )
  +return 0;
 
 -ENOMEM
 
  +msi_desc = desc-msi_desc;
  +if ( !msi_desc )
  +goto unlock_out;
 
 -EBADSLT
 
  +pci_dev = msi_desc-dev;
  +if ( !pci_dev )
  +goto unlock_out;
 
 -ENODEV
 
  +remap_index = msi_desc-remap_index;
  +drhd = acpi_find_matched_drhd_unit(pci_dev);
  +if ( !drhd )
  +{
  +dprintk(XENLOG_INFO VTDPREFIX,
  +%pv: failed to get drhd, pci device: 
  +%04x:%02x:%02x.%u, guest vector: %u\n,
  +v, pci_dev-seg, pci_dev-bus,
 PCI_SLOT(pci_dev-devfn),
  +PCI_FUNC(pci_dev-devfn), gvec);
  +goto unlock_out;
  +}
  +
  +iommu = drhd-iommu;
  +ir_ctrl = iommu_ir_ctrl(iommu);
  +if ( !ir_ctrl )
  +{
  +dprintk(XENLOG_INFO VTDPREFIX,
  +%pv: failed to get ir_ctrl, pci device: 
  +%04x:%02x:%02x.%u, guest vector: %u\n,
  +v, pci_dev-seg, pci_dev-bus,
 PCI_SLOT(pci_dev-devfn),
  +PCI_FUNC(pci_dev-devfn), gvec);
  +goto unlock_out;
  +}
 
 Do you think these log messages are useful beyond your bringup
 purposes?

So what kind of message do you think I need to show here?

Thanks,
Feng

 
  +spin_lock_irqsave(ir_ctrl-iremap_lock, flags);
  +
  +GET_IREMAP_ENTRY(ir_ctrl-iremap_maddr, remap_index,
 iremap_entries, p);
  +
  +memcpy(new_ire, p, sizeof(new_ire));
 
 Please use structure assignment (being type safe) in preference to
 memcpy() (not being type safe).
 
  --- a/xen/drivers/passthrough/vtd/iommu.h
  +++ b/xen/drivers/passthrough/vtd/iommu.h
  @@ -327,6 +327,10 @@ struct iremap_entry {
 };
   };
 
  +#define PDA_LOW_BIT26
  +#define PDA_HIGH_BIT   32
  +#define PDA_MASK(XX)   (~(-1UL  PDA_##XX##_BIT))
 
 To me it would look more natural if you used ~0UL.
 
 Jan


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

Re: [Xen-devel] [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used

2015-06-12 Thread Wu, Feng 


 -Original Message-
 From: Jan Beulich [mailto:jbeul...@suse.com]
 Sent: Wednesday, June 10, 2015 2:18 PM
 To: Wu, Feng
 Cc: andrew.coop...@citrix.com; george.dun...@eu.citrix.com; Tian, Kevin;
 Zhang, Yang Z; xen-devel@lists.xen.org; k...@xen.org
 Subject: Re: [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used
 
  On 08.05.15 at 11:07, feng...@intel.com wrote:
  +static inline void setup_posted_irte(
  +struct iremap_entry *new_ire, struct pi_desc *pi_desc, uint8_t gvec)
  +{
  +new_ire-post.urg = 0;
  +new_ire-post.vector = gvec;
  +new_ire-post.pda_l = (((u64)virt_to_maddr(pi_desc)) 
  +   (32 - PDA_LOW_BIT))  PDA_MASK(LOW);
  +new_ire-post.pda_h = (((u64)virt_to_maddr(pi_desc))  32) 
  +   PDA_MASK(HIGH);
 
 Looking at this another time I can't see what the and-ing with
 PAD_MASK() is supposed to be good for. 

I cannot understand this well. Do you mean we don't need and PDA_MASK() here?

Thanks,
Feng

Nor can I see the need
 for the u64 casts. The two literal 32-s aren't ideal either, but I
 guess tolerable.
 
 Jan


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

Re: [Xen-devel] [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used

2015-06-12 Thread Jan Beulich 
 On 12.06.15 at 11:40, feng...@intel.com wrote:
 -Original Message-
 From: Jan Beulich [mailto:jbeul...@suse.com]
  On 08.05.15 at 11:07, feng...@intel.com wrote:
  +static inline void setup_posted_irte(
  +struct iremap_entry *new_ire, struct pi_desc *pi_desc, uint8_t gvec)
  +{
  +new_ire-post.urg = 0;
  +new_ire-post.vector = gvec;
  +new_ire-post.pda_l = (((u64)virt_to_maddr(pi_desc)) 
  +   (32 - PDA_LOW_BIT))  PDA_MASK(LOW);
  +new_ire-post.pda_h = (((u64)virt_to_maddr(pi_desc))  32) 
  +   PDA_MASK(HIGH);
 
 Looking at this another time I can't see what the and-ing with
 PAD_MASK() is supposed to be good for. 
 
 I cannot understand this well. Do you mean we don't need and PDA_MASK() 
 here?

Correct - the bitfield width (where the data gets stored into) already
enforces the intended truncation afaict.

Jan


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

Re: [Xen-devel] [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used

2015-06-10 Thread Jan Beulich 
 On 08.05.15 at 11:07, feng...@intel.com wrote:
 +static inline void setup_posted_irte(
 +struct iremap_entry *new_ire, struct pi_desc *pi_desc, uint8_t gvec)
 +{
 +new_ire-post.urg = 0;
 +new_ire-post.vector = gvec;
 +new_ire-post.pda_l = (((u64)virt_to_maddr(pi_desc)) 
 +   (32 - PDA_LOW_BIT))  PDA_MASK(LOW);
 +new_ire-post.pda_h = (((u64)virt_to_maddr(pi_desc))  32) 
 +   PDA_MASK(HIGH);

Looking at this another time I can't see what the and-ing with
PAD_MASK() is supposed to be good for. Nor can I see the need
for the u64 casts. The two literal 32-s aren't ideal either, but I
guess tolerable.

Jan


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

Re: [Xen-devel] [RFC v2 07/15] vt-d: Add API to update IRTE when VT-d PI is used

2015-06-09 Thread Jan Beulich 
 On 08.05.15 at 11:07, feng...@intel.com wrote:
 +bool_t pi_update_irte(struct vcpu *v, struct pirq *pirq, uint8_t gvec)

Without seeing the caller right away it's hard to judge, but generally I'd
prefer functions to return -E... values as error indicators, i.e.

 +{
 +struct irq_desc *desc;
 +struct msi_desc *msi_desc;
 +int remap_index;
 +bool_t rc = 0;
 +struct pci_dev *pci_dev;
 +struct acpi_drhd_unit *drhd;
 +struct iommu *iommu;
 +struct ir_ctrl *ir_ctrl;
 +struct iremap_entry *iremap_entries = NULL, *p = NULL;
 +struct iremap_entry new_ire;
 +struct pi_desc *pi_desc = v-arch.hvm_vmx.pi_desc;
 +unsigned long flags;
 +
 +desc = pirq_spin_lock_irq_desc(pirq, NULL);
 +if ( !desc )
 +return 0;

-ENOMEM

 +msi_desc = desc-msi_desc;
 +if ( !msi_desc )
 +goto unlock_out;

-EBADSLT

 +pci_dev = msi_desc-dev;
 +if ( !pci_dev )
 +goto unlock_out;

-ENODEV

 +remap_index = msi_desc-remap_index;
 +drhd = acpi_find_matched_drhd_unit(pci_dev);
 +if ( !drhd )
 +{
 +dprintk(XENLOG_INFO VTDPREFIX,
 +%pv: failed to get drhd, pci device: 
 +%04x:%02x:%02x.%u, guest vector: %u\n,
 +v, pci_dev-seg, pci_dev-bus, PCI_SLOT(pci_dev-devfn),
 +PCI_FUNC(pci_dev-devfn), gvec);
 +goto unlock_out;
 +}
 +
 +iommu = drhd-iommu;
 +ir_ctrl = iommu_ir_ctrl(iommu);
 +if ( !ir_ctrl )
 +{
 +dprintk(XENLOG_INFO VTDPREFIX,
 +%pv: failed to get ir_ctrl, pci device: 
 +%04x:%02x:%02x.%u, guest vector: %u\n,
 +v, pci_dev-seg, pci_dev-bus, PCI_SLOT(pci_dev-devfn),
 +PCI_FUNC(pci_dev-devfn), gvec);
 +goto unlock_out;
 +}

Do you think these log messages are useful beyond your bringup
purposes?

 +spin_lock_irqsave(ir_ctrl-iremap_lock, flags);
 +
 +GET_IREMAP_ENTRY(ir_ctrl-iremap_maddr, remap_index, iremap_entries, p);
 +
 +memcpy(new_ire, p, sizeof(new_ire));

Please use structure assignment (being type safe) in preference to
memcpy() (not being type safe).

 --- a/xen/drivers/passthrough/vtd/iommu.h
 +++ b/xen/drivers/passthrough/vtd/iommu.h
 @@ -327,6 +327,10 @@ struct iremap_entry {
};
  };
  
 +#define PDA_LOW_BIT26
 +#define PDA_HIGH_BIT   32
 +#define PDA_MASK(XX)   (~(-1UL  PDA_##XX##_BIT))

To me it would look more natural if you used ~0UL.

Jan


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