Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-09-21 Thread Julien Grall 

Hi Sameer,

On 21/09/17 01:37, Goel, Sameer wrote:


[...]


diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 995a85a..3785fae 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -9,7 +9,12 @@
   #include 

   #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
-#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
+#define WARN_ON(p) ({  \
+int __ret_warn_on = !!(p); \
+if (unlikely(__ret_warn_on))   \
+WARN();\
+unlikely(__ret_warn_on);   \
+})


h. Why this change?

Was getting a compilation error when I was using WARN_ON as a conditional
in an if statement regarding the return value. So removed the loop. This
looks similar to Linux now.


This should belong to a separate patch with a commit message explaining why the 
change.


Agreed. Since this is more of a Linux compat function, I do not want to change 
the system-wide behavior. I will move this in the IORT code.


I don't see any reason to keep this implementation only for IORT. It 
would be possible to have similar construction in Xen.


So I would first try to see if maintainers would be willing to accept a 
system-wide change before thinking to re-implement WARN_ON in IORT.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-09-20 Thread Goel, Sameer 


On 9/12/2017 5:25 AM, Julien Grall wrote:
> Hi Sameer,
> 
> On 28/08/17 23:21, Goel, Sameer wrote:
>> On 6/12/2017 7:24 AM, Julien Grall wrote:
   static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
  struct fwnode_handle *fwnode,
  const struct iommu_ops *ops)
 @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, 
 u32 streamid,
   return ret;
   }

 -static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
 -    struct acpi_iort_node *node,
 -    u32 streamid)
 +static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node 
 *node,
 +    u32 streamid)
   {
 -    const struct iommu_ops *ops = NULL;
   int ret = -ENODEV;
   struct fwnode_handle *iort_fwnode;

   if (node) {
   iort_fwnode = iort_get_fwnode(node);
   if (!iort_fwnode)
 -    return NULL;
 -
 -    ops = iommu_ops_from_fwnode(iort_fwnode);
 -    if (!ops)
 -    return NULL;
 +    return ret;

 -    ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
 +    ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);
>>>
>>> Why don't you get the IOMMU ops here? This would avoid unecessary change.
>>  From the linux definition it seems that there will be eventually used to 
>> override the ops
>> set by the bus. I did not find a use for this here, so removed it to 
>> simplify code. I can
>> add these back, but see this as dead code.
> 
> You will always have dead code if you import code as it is from Linux. This 
> is the price to pay to help rebase the code in the future.
> 
> In that specific case, I think return the ops is the right thing to do. 
> Potentially it will allow us to support different IOMMU at the same time.
>
Agreed.
 
> [...]
> 
   #define IORT_IRQ_MASK(irq)    (irq & 0xULL)
   #define IORT_IRQ_TRIGGER_MASK(irq)    ((irq >> 32) & 0xULL)

   int iort_register_domain_token(int trans_id, struct fwnode_handle 
 *fw_node);
   void iort_deregister_domain_token(int trans_id);
   struct fwnode_handle *iort_find_domain_token(int trans_id);
 -#ifdef CONFIG_ACPI_IORT
 +#endif
 +
 +#ifdef CONFIG_ARM_64
>>>
>>> Why #ifdef CONFIG_ARM_64?
>> Was trying to keep the impact low for this RFC iteration (My use-case is 
>> arm64 only). Looking for the right recommendation?
> 
> IORT is not specific to ARM64. Even though ACPI is only supported for ARM64 
> on Xen today, we try to keep the code as generic as possible.
> 
> In that case, you could turn CONFIG_ACPI_IORT on in the Kconfig when ACPI is 
> enabled.
Agreed. I will add the new config in the next patch set.
> 
> [...]
> 
>>>
>>> [...]
>>>
 diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
 index 995a85a..3785fae 100644
 --- a/xen/include/xen/lib.h
 +++ b/xen/include/xen/lib.h
 @@ -9,7 +9,12 @@
   #include 

   #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
 -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
 +#define WARN_ON(p) ({  \
 +    int __ret_warn_on = !!(p); \
 +    if (unlikely(__ret_warn_on))   \
 +    WARN();    \
 +    unlikely(__ret_warn_on);   \
 +})
>>>
>>> h. Why this change?
>> Was getting a compilation error when I was using WARN_ON as a conditional
>> in an if statement regarding the return value. So removed the loop. This
>> looks similar to Linux now.
> 
> This should belong to a separate patch with a commit message explaining why 
> the change.

Agreed. Since this is more of a Linux compat function, I do not want to change 
the system-wide behavior. I will move this in the IORT code.
> 
> Cheers,
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-09-12 Thread Julien Grall 

Hi Sameer,

On 28/08/17 23:21, Goel, Sameer wrote:

On 6/12/2017 7:24 AM, Julien Grall wrote:

  static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
 struct fwnode_handle *fwnode,
 const struct iommu_ops *ops)
@@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 
streamid,
  return ret;
  }

-static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
-struct acpi_iort_node *node,
-u32 streamid)
+static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
+u32 streamid)
  {
-const struct iommu_ops *ops = NULL;
  int ret = -ENODEV;
  struct fwnode_handle *iort_fwnode;

  if (node) {
  iort_fwnode = iort_get_fwnode(node);
  if (!iort_fwnode)
-return NULL;
-
-ops = iommu_ops_from_fwnode(iort_fwnode);
-if (!ops)
-return NULL;
+return ret;

-ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, ops);
+ret = arm_smmu_iort_xlate(dev, streamid, iort_fwnode, NULL);


Why don't you get the IOMMU ops here? This would avoid unecessary change.

 From the linux definition it seems that there will be eventually used to 
override the ops
set by the bus. I did not find a use for this here, so removed it to simplify 
code. I can
add these back, but see this as dead code.


You will always have dead code if you import code as it is from Linux. 
This is the price to pay to help rebase the code in the future.


In that specific case, I think return the ops is the right thing to do. 
Potentially it will allow us to support different IOMMU at the same time.


[...]


  #define IORT_IRQ_MASK(irq)(irq & 0xULL)
  #define IORT_IRQ_TRIGGER_MASK(irq)((irq >> 32) & 0xULL)

  int iort_register_domain_token(int trans_id, struct fwnode_handle *fw_node);
  void iort_deregister_domain_token(int trans_id);
  struct fwnode_handle *iort_find_domain_token(int trans_id);
-#ifdef CONFIG_ACPI_IORT
+#endif
+
+#ifdef CONFIG_ARM_64


Why #ifdef CONFIG_ARM_64?

Was trying to keep the impact low for this RFC iteration (My use-case is arm64 
only). Looking for the right recommendation?


IORT is not specific to ARM64. Even though ACPI is only supported for 
ARM64 on Xen today, we try to keep the code as generic as possible.


In that case, you could turn CONFIG_ACPI_IORT on in the Kconfig when 
ACPI is enabled.


[...]



[...]


diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 995a85a..3785fae 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -9,7 +9,12 @@
  #include 

  #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
-#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
+#define WARN_ON(p) ({  \
+int __ret_warn_on = !!(p); \
+if (unlikely(__ret_warn_on))   \
+WARN();\
+unlikely(__ret_warn_on);   \
+})


h. Why this change?

Was getting a compilation error when I was using WARN_ON as a conditional
in an if statement regarding the return value. So removed the loop. This
looks similar to Linux now.


This should belong to a separate patch with a commit message explaining 
why the change.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-08-28 Thread Goel, Sameer 


On 6/12/2017 7:24 AM, Julien Grall wrote:
> Hi Sameer,
> 
> On 08/06/17 20:30, Sameer Goel wrote:
>> Add limited support for parsing IORT table to initialize SMMU devices.
> 
> It would be nice to explain what you actually support in the commit message.
> 
> [...]
> 
>>
>>  #define IORT_TYPE_MASK(type)    (1 << (type))
>>  #define IORT_MSI_TYPE    (1 << ACPI_IORT_NODE_ITS_GROUP)
>>  #define IORT_IOMMU_TYPE    ((1 << ACPI_IORT_NODE_SMMU) |    \
>>  (1 << ACPI_IORT_NODE_SMMU_V3))
>>
>> +#if 0
>>  struct iort_its_msi_chip {
>>  struct list_head    list;
>>  struct fwnode_handle    *fw_node;
>>  u32    translation_id;
>>  };
>>
>> +#endif
>> +
> 
> Why cannot you enable MSI now? Actually this would allow us to know whether 
> we should import the code from Linux or reimplement or own.
Well was not too sure about how this will fit in, so was leaving this for next 
iteration. I will try to enable this.
> 
>>  struct iort_fwnode {
>>  struct list_head list;
>>  struct acpi_iort_node *iort_node;
>> @@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node 
>> *iort_node,
>>  {
>>  struct iort_fwnode *np;
>>
>> -    np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC);
>> +    np = xzalloc(struct iort_fwnode);
> 
> If we decide to keep this code close to Linux you need to:
> - avoid replacing Linux name by Xen name as much as possible. Instead use 
> macro
> - explain above each change why you need then
> 
> See what I did for the ARM SMMU.
Agreed
> 
>>
>>  if (WARN_ON(!np))
>>  return -ENOMEM;
>> @@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct 
>> acpi_iort_node *node)
>>  list_for_each_entry_safe(curr, tmp, _fwnode_list, list) {
>>  if (curr->iort_node == node) {
>>  list_del(>list);
>> -    kfree(curr);
>> +    xfree(curr);
>>  break;
>>  }
>>  }
>> @@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback)
>>  /* Root pointer to the mapped IORT table */
>>  static struct acpi_table_header *iort_table;
>>
>> +#if 0
>>  static LIST_HEAD(iort_msi_chip_list);
>>  static DEFINE_SPINLOCK(iort_msi_chip_lock);
>>
>> @@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int 
>> trans_id)
>>
>>  return fw_node;
>>  }
>> -
>> +#endif
> 
> Please avoid dropping newline.
> 
>>  static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
>>   iort_find_node_callback callback,
>>   void *context)
>> @@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum 
>> acpi_iort_node_type type,
>>  iort_table->length);
>>
>>  for (i = 0; i < iort->node_count; i++) {
>> -    if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
>> -   "IORT node pointer overflows, bad table!\n"))
>> +    if (iort_node >= iort_end) {
>> +    printk(XENLOG_ERR "IORT node pointer overflows, bad table!\n");
>>  return NULL;
>> +    }
>>
>>  if (iort_node->type == type &&
>>  ACPI_SUCCESS(callback(iort_node, context)))
>> @@ -249,6 +262,14 @@ bool iort_node_match(u8 type)
>>  return node != NULL;
>>  }
>>
>> +/*
>> + * Following 2 definies should come from the PCI passthrough implementation.
>> + * Based on the current pci_dev define the bus number and seg number come
>> + * from pci_dev so making an API assumption
>> + */
>> +#define to_pci_dev(p) container_of(p, struct pci_dev,dev)
>> +#define pci_domain_nr(dev) dev->seg
> 
> This should go in pci.h.
> 
>> +
>>  static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
>>  void *context)
>>  {
>> @@ -256,6 +277,11 @@ static acpi_status iort_match_node_callback(struct 
>> acpi_iort_node *node,
>>  acpi_status status;
>>
>>  if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) {
>> +    status = AE_NOT_IMPLEMENTED;
>> +/*
>> + * Named components not supported yet.
> 
> Can you expand?
Were not needed for now, but I agree we can enable the code.
> 
> [...]
> 
>> +/*
>> + * RID is the same as PCI_DEVID(BDF) for QDF2400
> 
> Xen is meant to be agnostic to the platform. Rather than making assumption, 
> we should discuss on way to get the RID. I will answer on Robin's mail about 
> it.
> 
>> + */
>>  static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
>>  {
>>  u32 *rid = data;
>> @@ -510,7 +550,7 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 
>> alias, void *data)
>>  *rid = alias;
>>  return 0;
>>  }
>> -
>> +#endif
> 
> Please avoid dropping newline
> 
>>  static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
>>     struct fwnode_handle *fwnode,
>>     const struct iommu_ops *ops)
>> @@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 
>> streamid,
>>  return ret;
>>  }
>>
>>

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-08-28 Thread Goel, Sameer 


On 6/9/2017 5:15 AM, Robin Murphy wrote:
> On 08/06/17 20:30, Sameer Goel wrote:
> [...]
>>  /**
>> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
>> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
>> + * function sets up the fwspec as needed for a given device. Only PCI
>> + * devices are supported for now.
>>   *
>>   * @dev: device to configure
>>   *
>> - * Returns: iommu_ops pointer on configuration success
>> - *  NULL on configuration failure
>> + * Returns: Appropriate acpi_status
>>   */
>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>> +acpi_status iort_iommu_configure(struct device *dev)
>>  {
>>  struct acpi_iort_node *node, *parent;
>> -const struct iommu_ops *ops = NULL;
>>  u32 streamid = 0;
>> +acpi_status status = AE_OK;
>>  
>>  if (dev_is_pci(dev)) {
>> -struct pci_bus *bus = to_pci_dev(dev)->bus;
>> +struct pci_dev *pci_device = to_pci_dev(dev);
>>  u32 rid;
>>  
>> -pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
>> -   );
>> +rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
> 
> Beware that the Linux code isn't actually correct to begin with[1]. I
> don't know how much Xen deals with PCI bridges and quirks, but as it
> stands you should be able to trivially expose the flaw here by plugging
> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
> (or even both) kick up stream table faults.
> 
Appreciate the feedback Robin. I will try to incorporate the new IORT changes 
when I release the first patch set.  
> Robin.
> 
> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html
> 
>>  
>>  node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
>> -  iort_match_node_callback, >dev);
>> +  iort_match_node_callback, dev);
>>  if (!node)
>> -return NULL;
>> +return AE_NOT_FOUND;
>>  
>>  parent = iort_node_map_rid(node, rid, ,
>> IORT_IOMMU_TYPE);
>>  
>> -ops = iort_iommu_xlate(dev, parent, streamid);
>> +status = iort_iommu_xlate(dev, parent, streamid);
>> +
>> +status = status ? AE_ERROR : AE_OK;
>>  
>>  } else {
>> +status = AE_NOT_IMPLEMENTED;
>> +#if 0
>>  int i = 0;
>>  
>>  node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
>> @@ -616,11 +655,17 @@ const struct iommu_ops *iort_iommu_configure(struct 
>> device *dev)
>>  parent = iort_node_get_id(node, ,
>>IORT_IOMMU_TYPE, i++);
>>  }
>> +#endif
>>  }
>>  
>> -return ops;
>> +return status;
>>  }
> 

-- 
 Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, 
Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux 
Foundation Collaborative Project.

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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-07-14 Thread Jan Beulich 
>>> On 08.06.17 at 21:30,  wrote:
> Add limited support for parsing IORT table to initialize SMMU devices.
> 
> Signed-off-by: Sameer Goel 
> ---
>  xen/arch/arm/setup.c|   3 +
>  xen/drivers/acpi/Makefile   |   1 +
>  xen/drivers/acpi/arm/Makefile   |   1 +
>  xen/drivers/acpi/arm/iort.c | 232 
> +++-

With the amount of changes done to this file I question even more
the value of first pulling in the plain Linux commits.

> --- a/xen/include/xen/lib.h
> +++ b/xen/include/xen/lib.h
> @@ -9,7 +9,12 @@
>  #include 
>  
>  #define BUG_ON(p)  do { if (unlikely(p)) BUG();  } while (0)
> -#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0)
> +#define WARN_ON(p) ({  \
> +int __ret_warn_on = !!(p); \
> +if (unlikely(__ret_warn_on))   \
> +WARN();\
> +unlikely(__ret_warn_on);   \
> +})

This has nothing to do with the intention of the patch. If you want
WARN_ON()s behavior to change, please submit a separate patch
doing just that.

> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -88,6 +88,7 @@ struct pci_dev {
>  #define PT_FAULT_THRESHOLD 10
>  } fault;
>  u64 vf_rlen[6];
> +struct device dev;

Why? Please rationalize your changes in the patch description (and
perhaps split them).

Jan


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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-06-21 Thread Robin Murphy 
On 12/06/17 14:44, Jan Beulich wrote:
 On 12.06.17 at 15:36,  wrote:
>> On 09/06/17 12:15, Robin Murphy wrote:
>>> On 08/06/17 20:30, Sameer Goel wrote:
>>> [...]
  /**
 - * iort_iommu_configure - Set-up IOMMU configuration for a device.
 + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
 + * function sets up the fwspec as needed for a given device. Only PCI
 + * devices are supported for now.
   *
   * @dev: device to configure
   *
 - * Returns: iommu_ops pointer on configuration success
 - *  NULL on configuration failure
 + * Returns: Appropriate acpi_status
   */
 -const struct iommu_ops *iort_iommu_configure(struct device *dev)
 +acpi_status iort_iommu_configure(struct device *dev)
  {
struct acpi_iort_node *node, *parent;
 -  const struct iommu_ops *ops = NULL;
u32 streamid = 0;
 +  acpi_status status = AE_OK;

if (dev_is_pci(dev)) {
 -  struct pci_bus *bus = to_pci_dev(dev)->bus;
 +  struct pci_dev *pci_device = to_pci_dev(dev);
u32 rid;

 -  pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
 - );
 +  rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
>>>
>>> Beware that the Linux code isn't actually correct to begin with[1]. I
>>> don't know how much Xen deals with PCI bridges and quirks, but as it
>>> stands you should be able to trivially expose the flaw here by plugging
>>> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
>>> (or even both) kick up stream table faults.
>>>
>>> Robin.
>>>
>>> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html 
>>
>> I am not sure how x86 is handling that. The closest I can find would be 
>> domain_context_mapping. I have CCed x86 folks to get more feedback here.
> 
> I'm lacking enough context to know whether this is the issue
> with what we call phantom devices, or something else.

Phantom functions, PCIe-PCI bridges, basically anything that can result
in traffic from a single endpoint appearing on multiple RIDs, or a RID
other than the device's BDF, at the root complex (and IOMMUs/MSI
doorbells beyond).

Robin.

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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-06-12 Thread Jan Beulich 
>>> On 12.06.17 at 15:36,  wrote:
> On 09/06/17 12:15, Robin Murphy wrote:
>> On 08/06/17 20:30, Sameer Goel wrote:
>> [...]
>>>  /**
>>> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
>>> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
>>> + * function sets up the fwspec as needed for a given device. Only PCI
>>> + * devices are supported for now.
>>>   *
>>>   * @dev: device to configure
>>>   *
>>> - * Returns: iommu_ops pointer on configuration success
>>> - *  NULL on configuration failure
>>> + * Returns: Appropriate acpi_status
>>>   */
>>> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
>>> +acpi_status iort_iommu_configure(struct device *dev)
>>>  {
>>> struct acpi_iort_node *node, *parent;
>>> -   const struct iommu_ops *ops = NULL;
>>> u32 streamid = 0;
>>> +   acpi_status status = AE_OK;
>>>
>>> if (dev_is_pci(dev)) {
>>> -   struct pci_bus *bus = to_pci_dev(dev)->bus;
>>> +   struct pci_dev *pci_device = to_pci_dev(dev);
>>> u32 rid;
>>>
>>> -   pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
>>> -  );
>>> +   rid = PCI_BDF2(pci_device->bus,pci_device->devfn);
>>
>> Beware that the Linux code isn't actually correct to begin with[1]. I
>> don't know how much Xen deals with PCI bridges and quirks, but as it
>> stands you should be able to trivially expose the flaw here by plugging
>> in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
>> (or even both) kick up stream table faults.
>>
>> Robin.
>>
>> [1]:http://www.spinics.net/lists/linux-acpi/msg74844.html 
> 
> I am not sure how x86 is handling that. The closest I can find would be 
> domain_context_mapping. I have CCed x86 folks to get more feedback here.

I'm lacking enough context to know whether this is the issue
with what we call phantom devices, or something else.

Jan


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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-06-12 Thread Julien Grall 

(CC x86 folks)

Hi,

On 09/06/17 12:15, Robin Murphy wrote:

On 08/06/17 20:30, Sameer Goel wrote:
[...]

 /**
- * iort_iommu_configure - Set-up IOMMU configuration for a device.
+ * iort_iommu_configure - Set-up IOMMU configuration for a device. This
+ * function sets up the fwspec as needed for a given device. Only PCI
+ * devices are supported for now.
  *
  * @dev: device to configure
  *
- * Returns: iommu_ops pointer on configuration success
- *  NULL on configuration failure
+ * Returns: Appropriate acpi_status
  */
-const struct iommu_ops *iort_iommu_configure(struct device *dev)
+acpi_status iort_iommu_configure(struct device *dev)
 {
struct acpi_iort_node *node, *parent;
-   const struct iommu_ops *ops = NULL;
u32 streamid = 0;
+   acpi_status status = AE_OK;

if (dev_is_pci(dev)) {
-   struct pci_bus *bus = to_pci_dev(dev)->bus;
+   struct pci_dev *pci_device = to_pci_dev(dev);
u32 rid;

-   pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
-  );
+   rid = PCI_BDF2(pci_device->bus,pci_device->devfn);


Beware that the Linux code isn't actually correct to begin with[1]. I
don't know how much Xen deals with PCI bridges and quirks, but as it
stands you should be able to trivially expose the flaw here by plugging
in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
(or even both) kick up stream table faults.

Robin.

[1]:http://www.spinics.net/lists/linux-acpi/msg74844.html


I am not sure how x86 is handling that. The closest I can find would be 
domain_context_mapping. I have CCed x86 folks to get more feedback here.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-06-12 Thread Julien Grall 

Hi Sameer,

On 08/06/17 20:30, Sameer Goel wrote:

Add limited support for parsing IORT table to initialize SMMU devices.


It would be nice to explain what you actually support in the commit message.

[...]



 #define IORT_TYPE_MASK(type)   (1 << (type))
 #define IORT_MSI_TYPE  (1 << ACPI_IORT_NODE_ITS_GROUP)
 #define IORT_IOMMU_TYPE((1 << ACPI_IORT_NODE_SMMU) | \
(1 << ACPI_IORT_NODE_SMMU_V3))

+#if 0
 struct iort_its_msi_chip {
struct list_headlist;
struct fwnode_handle*fw_node;
u32 translation_id;
 };

+#endif
+


Why cannot you enable MSI now? Actually this would allow us to know 
whether we should import the code from Linux or reimplement or own.



 struct iort_fwnode {
struct list_head list;
struct acpi_iort_node *iort_node;
@@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node 
*iort_node,
 {
struct iort_fwnode *np;

-   np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC);
+   np = xzalloc(struct iort_fwnode);


If we decide to keep this code close to Linux you need to:
	- avoid replacing Linux name by Xen name as much as possible. Instead 
use macro

- explain above each change why you need then

See what I did for the ARM SMMU.



if (WARN_ON(!np))
return -ENOMEM;
@@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct acpi_iort_node 
*node)
list_for_each_entry_safe(curr, tmp, _fwnode_list, list) {
if (curr->iort_node == node) {
list_del(>list);
-   kfree(curr);
+   xfree(curr);
break;
}
}
@@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback)
 /* Root pointer to the mapped IORT table */
 static struct acpi_table_header *iort_table;

+#if 0
 static LIST_HEAD(iort_msi_chip_list);
 static DEFINE_SPINLOCK(iort_msi_chip_lock);

@@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int trans_id)

return fw_node;
 }
-
+#endif


Please avoid dropping newline.


 static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
 iort_find_node_callback callback,
 void *context)
@@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum 
acpi_iort_node_type type,
iort_table->length);

for (i = 0; i < iort->node_count; i++) {
-   if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
-  "IORT node pointer overflows, bad table!\n"))
+   if (iort_node >= iort_end) {
+   printk(XENLOG_ERR "IORT node pointer overflows, bad 
table!\n");
return NULL;
+   }

if (iort_node->type == type &&
ACPI_SUCCESS(callback(iort_node, context)))
@@ -249,6 +262,14 @@ bool iort_node_match(u8 type)
return node != NULL;
 }

+/*
+ * Following 2 definies should come from the PCI passthrough implementation.
+ * Based on the current pci_dev define the bus number and seg number come
+ * from pci_dev so making an API assumption
+ */
+#define to_pci_dev(p) container_of(p, struct pci_dev,dev)
+#define pci_domain_nr(dev) dev->seg


This should go in pci.h.


+
 static acpi_status iort_match_node_callback(struct acpi_iort_node *node,
void *context)
 {
@@ -256,6 +277,11 @@ static acpi_status iort_match_node_callback(struct 
acpi_iort_node *node,
acpi_status status;

if (node->type == ACPI_IORT_NODE_NAMED_COMPONENT) {
+   status = AE_NOT_IMPLEMENTED;
+/*
+ * Named components not supported yet.


Can you expand?

[...]


+/*
+ * RID is the same as PCI_DEVID(BDF) for QDF2400


Xen is meant to be agnostic to the platform. Rather than making 
assumption, we should discuss on way to get the RID. I will answer on 
Robin's mail about it.



+ */
 static int __get_pci_rid(struct pci_dev *pdev, u16 alias, void *data)
 {
u32 *rid = data;
@@ -510,7 +550,7 @@ static int __get_pci_rid(struct pci_dev *pdev, u16 alias, 
void *data)
*rid = alias;
return 0;
 }
-
+#endif


Please avoid dropping newline


 static int arm_smmu_iort_xlate(struct device *dev, u32 streamid,
   struct fwnode_handle *fwnode,
   const struct iommu_ops *ops)
@@ -523,29 +563,24 @@ static int arm_smmu_iort_xlate(struct device *dev, u32 
streamid,
return ret;
 }

-static const struct iommu_ops *iort_iommu_xlate(struct device *dev,
-   struct acpi_iort_node *node,
-   u32 streamid)
+static int iort_iommu_xlate(struct device *dev, struct acpi_iort_node *node,
+

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-06-09 Thread Robin Murphy 
On 08/06/17 20:30, Sameer Goel wrote:
[...]
>  /**
> - * iort_iommu_configure - Set-up IOMMU configuration for a device.
> + * iort_iommu_configure - Set-up IOMMU configuration for a device. This
> + * function sets up the fwspec as needed for a given device. Only PCI
> + * devices are supported for now.
>   *
>   * @dev: device to configure
>   *
> - * Returns: iommu_ops pointer on configuration success
> - *  NULL on configuration failure
> + * Returns: Appropriate acpi_status
>   */
> -const struct iommu_ops *iort_iommu_configure(struct device *dev)
> +acpi_status iort_iommu_configure(struct device *dev)
>  {
>   struct acpi_iort_node *node, *parent;
> - const struct iommu_ops *ops = NULL;
>   u32 streamid = 0;
> + acpi_status status = AE_OK;
>  
>   if (dev_is_pci(dev)) {
> - struct pci_bus *bus = to_pci_dev(dev)->bus;
> + struct pci_dev *pci_device = to_pci_dev(dev);
>   u32 rid;
>  
> - pci_for_each_dma_alias(to_pci_dev(dev), __get_pci_rid,
> -);
> + rid = PCI_BDF2(pci_device->bus,pci_device->devfn);

Beware that the Linux code isn't actually correct to begin with[1]. I
don't know how much Xen deals with PCI bridges and quirks, but as it
stands you should be able to trivially expose the flaw here by plugging
in a Marvell 88SE912x-based SATA card and watching either DMA or MSIs
(or even both) kick up stream table faults.

Robin.

[1]:http://www.spinics.net/lists/linux-acpi/msg74844.html

>  
>   node = iort_scan_node(ACPI_IORT_NODE_PCI_ROOT_COMPLEX,
> -   iort_match_node_callback, >dev);
> +   iort_match_node_callback, dev);
>   if (!node)
> - return NULL;
> + return AE_NOT_FOUND;
>  
>   parent = iort_node_map_rid(node, rid, ,
>  IORT_IOMMU_TYPE);
>  
> - ops = iort_iommu_xlate(dev, parent, streamid);
> + status = iort_iommu_xlate(dev, parent, streamid);
> +
> + status = status ? AE_ERROR : AE_OK;
>  
>   } else {
> + status = AE_NOT_IMPLEMENTED;
> +#if 0
>   int i = 0;
>  
>   node = iort_scan_node(ACPI_IORT_NODE_NAMED_COMPONENT,
> @@ -616,11 +655,17 @@ const struct iommu_ops *iort_iommu_configure(struct 
> device *dev)
>   parent = iort_node_get_id(node, ,
> IORT_IOMMU_TYPE, i++);
>   }
> +#endif
>   }
>  
> - return ops;
> + return status;
>  }

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

Re: [Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-06-08 Thread Stefano Stabellini 
CCing Jan

On Thu, 8 Jun 2017, Sameer Goel wrote:
> Add limited support for parsing IORT table to initialize SMMU devices.
> 
> Signed-off-by: Sameer Goel 
> ---
>  xen/arch/arm/setup.c|   3 +
>  xen/drivers/acpi/Makefile   |   1 +
>  xen/drivers/acpi/arm/Makefile   |   1 +
>  xen/drivers/acpi/arm/iort.c | 232 
> +++-
>  xen/drivers/passthrough/arm/iommu.c |  15 +--
>  xen/include/acpi/acpi.h |   1 +
>  xen/include/acpi/acpi_iort.h|  25 ++--
>  xen/include/asm-arm/device.h|   2 +
>  xen/include/xen/acpi.h  |  21 
>  xen/include/xen/lib.h   |   7 +-
>  xen/include/xen/pci.h   |   1 +
>  11 files changed, 184 insertions(+), 125 deletions(-)
>  create mode 100644 xen/drivers/acpi/arm/Makefile

This patch doesn't apply with "git am" and doesn't build on x86 with:

In file included from /local/repos/xen-upstream/xen/include/xen/iommu.h:24:0,
 from /local/repos/xen-upstream/xen/include/asm/hvm/domain.h:23,
 from /local/repos/xen-upstream/xen/include/asm/domain.h:7,
 from /local/repos/xen-upstream/xen/include/xen/domain.h:8,
 from /local/repos/xen-upstream/xen/include/xen/sched.h:11,
 from x86_64/asm-offsets.c:9:
/local/repos/xen-upstream/xen/include/xen/pci.h:91:19: error: field ‘dev’ has 
incomplete type
 struct device dev;
   ^
In file included from /local/repos/xen-upstream/xen/include/acpi/acpi.h:63:0,
 from /local/repos/xen-upstream/xen/include/xen/acpi.h:33,
 from /local/repos/xen-upstream/xen/include/asm/fixmap.h:21,
 from x86_64/asm-offsets.c:12:
/local/repos/xen-upstream/xen/include/acpi/acpi_iort.h:60:23: error: expected 
‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘iort_iommu_configure’
 acpi_status iommu_ops iort_iommu_configure(struct device *dev)
   ^
make[3]: *** [asm-offsets.s] Error 1


And it doesn't build on arm64 with:

/local/repos/xen-upstream/xen/arch/arm/setup.c:765: undefined reference to 
`acpi_iort_init'
/local/repos/gcc-linaro-4.9-2014.05-aarch64-linux-gnu-x86_64-linux-gnu/bin/aarch64-linux-gnu-ld:
 /local/repos/xen-upstream/xen/.xen-syms.0: hidden symbol `acpi_iort_init' 
isn't defined
/local/repos/gcc-linaro-4.9-2014.05-aarch64-linux-gnu-x86_64-linux-gnu/bin/aarch64-linux-gnu-ld:
 final link failed: Bad value
make[3]: *** [/local/repos/xen-upstream/xen/xen-syms] Error 1


> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 92a2de6..5dc93ff 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -753,6 +753,9 @@ void __init start_xen(unsigned long boot_phys_offset,
>  /* Parse the ACPI tables for possible boot-time configuration */
>  acpi_boot_table_init();
>  
> +/* Initialize the IORT tables */
> +acpi_iort_init();
> +
>  if ( acpi_disabled )
>  printk("Booting using Device Tree\n");
>  else
> diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
> index 444b11d..4165318 100644
> --- a/xen/drivers/acpi/Makefile
> +++ b/xen/drivers/acpi/Makefile
> @@ -1,5 +1,6 @@
>  subdir-y += tables
>  subdir-y += utilities
> +subdir-$(CONFIG_ARM_64) += arm
>  subdir-$(CONFIG_X86) += apei
>  
>  obj-bin-y += tables.init.o
> diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile
> new file mode 100644
> index 000..7c039bb
> --- /dev/null
> +++ b/xen/drivers/acpi/arm/Makefile
> @@ -0,0 +1 @@
> +obj-y += iort.o
> diff --git a/xen/drivers/acpi/arm/iort.c b/xen/drivers/acpi/arm/iort.c
> index 4a5bb96..c22ec31 100644
> --- a/xen/drivers/acpi/arm/iort.c
> +++ b/xen/drivers/acpi/arm/iort.c
> @@ -14,29 +14,40 @@
>   * This file implements early detection/parsing of I/O mapping
>   * reported to OS through firmware via I/O Remapping Table (IORT)
>   * IORT document number: ARM DEN 0049A
> + *
> + * Based on Linux drivers/acpi/arm64/iort.c
> + * => commit ca78d3173cff3503bcd15723b049757f75762d15
> + *
> + * Xen modification:
> + * Sameer Goel 
> + * Copyright (C) 2017, The Linux Foundation, All rights reserved.
> + *
>   */
>  
> -#define pr_fmt(fmt)  "ACPI: IORT: " fmt
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
>  
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
> -#include 
>  
>  #define IORT_TYPE_MASK(type) (1 << (type))
>  #define IORT_MSI_TYPE(1 << ACPI_IORT_NODE_ITS_GROUP)
>  #define IORT_IOMMU_TYPE  ((1 << ACPI_IORT_NODE_SMMU) |   \
>   (1 << ACPI_IORT_NODE_SMMU_V3))
>  
> +#if 0
>  struct iort_its_msi_chip {
>   struct list_headlist;
>   struct fwnode_handle*fw_node;
>   u32 translation_id;
>  };
>  
> +#endif
> +
>  struct iort_fwnode {
>   struct list_head list;
>

[Xen-devel] [RFC 6/6] acpi:arm64: Add support for parsing IORT table

2017-06-08 Thread Sameer Goel 
Add limited support for parsing IORT table to initialize SMMU devices.

Signed-off-by: Sameer Goel 
---
 xen/arch/arm/setup.c|   3 +
 xen/drivers/acpi/Makefile   |   1 +
 xen/drivers/acpi/arm/Makefile   |   1 +
 xen/drivers/acpi/arm/iort.c | 232 +++-
 xen/drivers/passthrough/arm/iommu.c |  15 +--
 xen/include/acpi/acpi.h |   1 +
 xen/include/acpi/acpi_iort.h|  25 ++--
 xen/include/asm-arm/device.h|   2 +
 xen/include/xen/acpi.h  |  21 
 xen/include/xen/lib.h   |   7 +-
 xen/include/xen/pci.h   |   1 +
 11 files changed, 184 insertions(+), 125 deletions(-)
 create mode 100644 xen/drivers/acpi/arm/Makefile

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 92a2de6..5dc93ff 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -753,6 +753,9 @@ void __init start_xen(unsigned long boot_phys_offset,
 /* Parse the ACPI tables for possible boot-time configuration */
 acpi_boot_table_init();
 
+/* Initialize the IORT tables */
+acpi_iort_init();
+
 if ( acpi_disabled )
 printk("Booting using Device Tree\n");
 else
diff --git a/xen/drivers/acpi/Makefile b/xen/drivers/acpi/Makefile
index 444b11d..4165318 100644
--- a/xen/drivers/acpi/Makefile
+++ b/xen/drivers/acpi/Makefile
@@ -1,5 +1,6 @@
 subdir-y += tables
 subdir-y += utilities
+subdir-$(CONFIG_ARM_64) += arm
 subdir-$(CONFIG_X86) += apei
 
 obj-bin-y += tables.init.o
diff --git a/xen/drivers/acpi/arm/Makefile b/xen/drivers/acpi/arm/Makefile
new file mode 100644
index 000..7c039bb
--- /dev/null
+++ b/xen/drivers/acpi/arm/Makefile
@@ -0,0 +1 @@
+obj-y += iort.o
diff --git a/xen/drivers/acpi/arm/iort.c b/xen/drivers/acpi/arm/iort.c
index 4a5bb96..c22ec31 100644
--- a/xen/drivers/acpi/arm/iort.c
+++ b/xen/drivers/acpi/arm/iort.c
@@ -14,29 +14,40 @@
  * This file implements early detection/parsing of I/O mapping
  * reported to OS through firmware via I/O Remapping Table (IORT)
  * IORT document number: ARM DEN 0049A
+ *
+ * Based on Linux drivers/acpi/arm64/iort.c
+ * => commit ca78d3173cff3503bcd15723b049757f75762d15
+ *
+ * Xen modification:
+ * Sameer Goel 
+ * Copyright (C) 2017, The Linux Foundation, All rights reserved.
+ *
  */
 
-#define pr_fmt(fmt)"ACPI: IORT: " fmt
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
 
 #define IORT_TYPE_MASK(type)   (1 << (type))
 #define IORT_MSI_TYPE  (1 << ACPI_IORT_NODE_ITS_GROUP)
 #define IORT_IOMMU_TYPE((1 << ACPI_IORT_NODE_SMMU) |   \
(1 << ACPI_IORT_NODE_SMMU_V3))
 
+#if 0
 struct iort_its_msi_chip {
struct list_headlist;
struct fwnode_handle*fw_node;
u32 translation_id;
 };
 
+#endif
+
 struct iort_fwnode {
struct list_head list;
struct acpi_iort_node *iort_node;
@@ -60,7 +71,7 @@ static inline int iort_set_fwnode(struct acpi_iort_node 
*iort_node,
 {
struct iort_fwnode *np;
 
-   np = kzalloc(sizeof(struct iort_fwnode), GFP_ATOMIC);
+   np = xzalloc(struct iort_fwnode);
 
if (WARN_ON(!np))
return -ENOMEM;
@@ -114,7 +125,7 @@ static inline void iort_delete_fwnode(struct acpi_iort_node 
*node)
list_for_each_entry_safe(curr, tmp, _fwnode_list, list) {
if (curr->iort_node == node) {
list_del(>list);
-   kfree(curr);
+   xfree(curr);
break;
}
}
@@ -127,6 +138,7 @@ typedef acpi_status (*iort_find_node_callback)
 /* Root pointer to the mapped IORT table */
 static struct acpi_table_header *iort_table;
 
+#if 0
 static LIST_HEAD(iort_msi_chip_list);
 static DEFINE_SPINLOCK(iort_msi_chip_lock);
 
@@ -199,7 +211,7 @@ struct fwnode_handle *iort_find_domain_token(int trans_id)
 
return fw_node;
 }
-
+#endif
 static struct acpi_iort_node *iort_scan_node(enum acpi_iort_node_type type,
 iort_find_node_callback callback,
 void *context)
@@ -219,9 +231,10 @@ static struct acpi_iort_node *iort_scan_node(enum 
acpi_iort_node_type type,
iort_table->length);
 
for (i = 0; i < iort->node_count; i++) {
-   if (WARN_TAINT(iort_node >= iort_end, TAINT_FIRMWARE_WORKAROUND,
-  "IORT node pointer overflows, bad table!\n"))
+   if (iort_node >= iort_end) {
+   printk(XENLOG_ERR "IORT node pointer overflows, bad 
table!\n");
return NULL;
+   }
 
if (iort_node->type == type &&
ACPI_SUCCESS(callback(iort_node, context)))
@@