* [PATCH 1/2] ARM: ACPI: IORT: Estimate the size of hardware domain IORT table
2017-09-11 21:33 [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table mjaggi
@ 2017-09-11 21:33 ` mjaggi
2017-09-11 21:33 ` [PATCH 2/2] ARM: ACPI: IORT: Write Hardware domain's " mjaggi
2017-09-22 14:12 ` [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware " Andre Przywara
2 siblings, 0 replies; 13+ messages in thread
From: mjaggi @ 2017-09-11 21:33 UTC (permalink / raw)
To: xen-devel
Cc: Andre.Przywara, julien.grall, sstabellini, Manish Jaggi, tomasz.nowicki
From: Manish Jaggi <mjaggi@cavium.com>
This patch estimates size of hardware domain IORT table by parsing all
the pcirc nodes and their idmaps, and thereby calculating size by
removing smmu nodes.
Hardware domain IORT table will have only ITS and PCIRC nodes, and PCIRC
nodes' idmap will have output refrences to ITS group nodes.
Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
xen/arch/arm/acpi/Makefile | 1 +
xen/arch/arm/acpi/iort.c | 242 ++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/domain_build.c | 11 +-
xen/include/asm-arm/iort.h | 14 +++
4 files changed, 267 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/acpi/Makefile b/xen/arch/arm/acpi/Makefile
index 23963f8..93d8868 100644
--- a/xen/arch/arm/acpi/Makefile
+++ b/xen/arch/arm/acpi/Makefile
@@ -1,2 +1,3 @@
obj-y += lib.o
obj-y += boot.init.o
+obj-y += iort.o
diff --git a/xen/arch/arm/acpi/iort.c b/xen/arch/arm/acpi/iort.c
new file mode 100644
index 0000000..01914cb
--- /dev/null
+++ b/xen/arch/arm/acpi/iort.c
@@ -0,0 +1,242 @@
+#include <xen/init.h>
+#include <xen/compile.h>
+#include <xen/lib.h>
+#include <xen/mm.h>
+#include <xen/domain_page.h>
+#include <xen/sched.h>
+#include <xen/acpi.h>
+#include <acpi/actables.h>
+#include <xen/list.h>
+
+struct pcirc_idmap
+{
+ struct acpi_iort_id_mapping idmap;
+ struct list_head entry;
+};
+
+
+int add_to_new_idmap_list(struct list_head *new_idmap_list,
+ unsigned int ib, unsigned int ob,
+ unsigned int oref, unsigned int idc)
+{
+ struct pcirc_idmap *new_e = xzalloc(struct pcirc_idmap);
+ if ( !new_e )
+ {
+ printk("Unable to allocate memory\n");
+ return -ENOMEM;
+ }
+
+ new_e->idmap.input_base = ib;
+ new_e->idmap.output_base = ob;
+ new_e->idmap.output_reference = oref;
+ new_e->idmap.id_count = idc;
+
+ list_add_tail(&new_e->entry, new_idmap_list);
+
+ return 0;
+}
+
+/*
+ * returns the number of pci_idmaps created as a result of parsing
+ * the smmu nodes for this pci_idmap
+ * the pci_idmaps are added to the new_idmap_list
+ *
+ * if get_count_only is set new_idmap_list is not populated, just the
+ * id_count is updated.
+ *
+ * This method is called in estimate size flow and write hwdom iort flow
+ */
+static int update_id_mapping(struct acpi_iort_id_mapping *pci_idmap,
+ struct acpi_iort_node *smmu_node,
+ struct list_head *new_idmap_list /* [out] */,
+ int get_count_only,
+ unsigned int *id_count /* [out] */)
+{
+ /* local varialbes to hold input output base and count values.
+ * p_ for pci idmap; s for smmu idmap */
+ unsigned int p_ib, p_ob, p_idc, s_ib, s_ob, s_idc, delta, i;
+ struct acpi_iort_id_mapping *smmu_idmap = NULL;
+ int ret;
+
+ p_ib = pci_idmap->input_base;
+ p_ob = pci_idmap->output_base;
+ p_idc = pci_idmap->id_count;
+
+ if(get_count_only)
+ *id_count = 0;
+
+ for ( i = 0; i < smmu_node->mapping_count; i++ )
+ {
+ smmu_idmap = (struct acpi_iort_id_mapping*)((u8*)smmu_node
+ + smmu_node->mapping_offset);
+ s_ib = smmu_idmap->input_base;
+ s_ob = smmu_idmap->output_base;
+ s_idc = smmu_idmap->id_count;
+
+ if ( s_ib <= p_ob )
+ {
+ if ( s_ib + s_idc < p_ob )
+ continue;
+
+ delta = p_ob - s_ib;
+
+ if ( get_count_only )
+ {
+ (*id_count)++;
+ continue;
+ }
+
+ ret = add_to_new_idmap_list(new_idmap_list, p_ib, s_ob + delta,
+ smmu_idmap->output_reference,
+ s_ib + s_idc <= p_ob + p_idc ? s_idc - delta : p_idc);
+ if (ret)
+ goto err;
+ }
+ else
+ {
+ if( p_ob + p_idc < s_ib )
+ continue;
+
+ delta = s_ib - p_ob;
+
+ if ( get_count_only )
+ {
+ (*id_count)++;
+ continue;
+ }
+
+ ret = add_to_new_idmap_list(new_idmap_list, p_ib + delta, s_ob,
+ smmu_idmap->output_reference,
+ s_ib + s_idc < p_ob + p_idc ? s_idc : p_idc - delta);
+ if (ret)
+ goto err;
+ }
+ }
+
+ return 0;
+err:
+ return ret;
+}
+
+/*
+ * This method scans the idarray for a pcirc_node
+ * For each idmap there could be one or more entries in smmu idmap array
+ *
+ * update_id_mapping will add correspoding smmu idmap entries but will
+ * change mapping, so a pci idmap entry would have output_ref as its group
+ *
+ * idlist is populated when get_num_ids == 0
+ * - stores updated ids by removing smmu output reference
+ */
+static int scan_idarray(u8 *iort_base_ptr, struct acpi_iort_node *node,
+ struct list_head *idmaplist, int get_num_ids,
+ unsigned int *num_ids)
+{
+ int i = 0, ret = 0;
+ unsigned int id_count = 0;
+ struct acpi_iort_node *onode; /* output node */
+ struct acpi_iort_id_mapping *idmap = (struct acpi_iort_id_mapping*)
+ ((u8*)node + node->mapping_offset);
+
+ for ( i = 0; i < node->mapping_count; i++ )
+ {
+ onode = (struct acpi_iort_node*)(iort_base_ptr +
+ idmap->output_reference);
+ switch ( onode->type )
+ {
+ case ACPI_IORT_NODE_ITS_GROUP:
+ continue;
+ case ACPI_IORT_NODE_SMMU:
+ case ACPI_IORT_NODE_SMMU_V3:
+ ret = update_id_mapping(idmap, onode, idmaplist, get_num_ids,
+ &id_count);
+ if ( ret )
+ goto end;
+ if (get_num_ids)
+ *num_ids += id_count;
+ break;
+ }
+ idmap++;
+ }
+
+end:
+ return ret;
+}
+
+/*
+ * This method estimates the size of pci_rc node
+ * and returns any extra bytes added due to accomodting smmu_id_array elements
+ * which map to a single element of pci_rc id_array.
+ */
+static int estimate_pcirc_node_size(u8 *iort_base_ptr,
+ struct acpi_iort_node *node,
+ unsigned int *node_size)
+{
+ int ret;
+ unsigned int num_ids = 0;
+
+ *node_size = 0;
+ ret = scan_idarray(iort_base_ptr, node, NULL, 1, &num_ids);
+ if ( !ret )
+ *node_size += num_ids*sizeof(struct acpi_iort_id_mapping) +
+ sizeof(struct acpi_iort_node) -1 +
+ sizeof(struct acpi_iort_root_complex);
+
+ return ret;
+}
+
+/*
+ * This method parses the iort_table pci_rc nodes.
+ * For each element in the pci_rc: id_array smmu output reference
+ * has to be patched up with ITS group output reference.
+ * Each element in the id_array may map to mulpile entries in smmu id_array.
+ * Thus when patching the smmu output refrence, extra elements in id_array
+ * need to be created.
+ * Thus the size of the resultant id_array need to identified before the actual
+ * patching is done hardware domains's IORT table will have PCIRC
+ * and ITS group nodes only.
+ */
+int estimate_iort_size(size_t *iort_size)
+{
+ unsigned int i;
+ unsigned int node_size = 0;
+ struct acpi_table_iort *fw_iort;
+ struct acpi_iort_node *node = NULL;
+
+ if ( acpi_get_table(ACPI_SIG_IORT, 0,
+ (struct acpi_table_header **)&fw_iort) )
+ {
+ printk("Failed to get IORT table\n");
+ goto err;
+ }
+
+ *iort_size = sizeof(struct acpi_table_iort);
+ node = (struct acpi_iort_node *)((u8*)fw_iort + fw_iort->node_offset);
+
+ if (!node)
+ goto err;
+
+ /* Iterate over the pci_rc nodes in firmware iort table */
+ for ( i = 0; i < fw_iort->node_count; i++ )
+ {
+ switch (node->type)
+ {
+ case ACPI_IORT_NODE_PCI_ROOT_COMPLEX:
+ if( !estimate_pcirc_node_size((u8*)fw_iort, node, &node_size) )
+ *iort_size += node_size;
+ else
+ goto err;
+ break;
+
+ case ACPI_IORT_NODE_ITS_GROUP:
+ *iort_size += node->length;
+ break;
+ }
+
+ node = (struct acpi_iort_node *)((uint8_t*)node + node->length);
+ }
+
+ return 0;
+err:
+ return -EINVAL;
+}
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 5739ea4..0328926 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -20,6 +20,7 @@
#include <asm/psci.h>
#include <asm/setup.h>
#include <asm/cpufeature.h>
+#include <asm/iort.h>
#include <asm/gic.h>
#include <xen/irq.h>
@@ -1796,7 +1797,7 @@ static int acpi_create_fadt(struct domain *d, struct membank tbl_add[])
static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
{
- size_t efi_size, acpi_size, madt_size;
+ size_t efi_size, acpi_size, madt_size, iort_size;
u64 addr;
struct acpi_table_rsdp *rsdp_tbl;
struct acpi_table_header *table;
@@ -1837,6 +1838,14 @@ static int estimate_acpi_efi_size(struct domain *d, struct kernel_info *kinfo)
acpi_os_unmap_memory(table, sizeof(struct acpi_table_header));
acpi_size += ROUNDUP(sizeof(struct acpi_table_rsdp), 8);
+
+ if( estimate_iort_size(&iort_size) )
+ {
+ printk("Unable to get hwdom iort size\n");
+ return -EINVAL;
+ }
+ acpi_size += iort_size;
+
d->arch.efi_acpi_len = PAGE_ALIGN(ROUNDUP(efi_size, 8)
+ ROUNDUP(acpi_size, 8));
diff --git a/xen/include/asm-arm/iort.h b/xen/include/asm-arm/iort.h
new file mode 100644
index 0000000..3b4cd7b
--- /dev/null
+++ b/xen/include/asm-arm/iort.h
@@ -0,0 +1,14 @@
+#ifndef _XEN_ASM_IORT_H
+#define _XEN_ASM_IORT_H
+
+int estimate_iort_size(size_t *iort_size);
+
+#endif /* _XEN_ASM_IORT_H */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH 2/2] ARM: ACPI: IORT: Write Hardware domain's IORT table
2017-09-11 21:33 [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table mjaggi
2017-09-11 21:33 ` [PATCH 1/2] ARM: ACPI: IORT: Estimate the size of hardware domain " mjaggi
@ 2017-09-11 21:33 ` mjaggi
2017-09-22 14:12 ` [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware " Andre Przywara
2 siblings, 0 replies; 13+ messages in thread
From: mjaggi @ 2017-09-11 21:33 UTC (permalink / raw)
To: xen-devel
Cc: Andre.Przywara, julien.grall, sstabellini, Manish Jaggi, tomasz.nowicki
From: Manish Jaggi <mjaggi@cavium.com>
This patch writes hardware domain's IORT table. The steps are:
a. First ITS group nodes are written and their offsets are saved
along with the respective offsets from the firmware table.
This is required when smmu node is hidden and smmu node still points
to the old output_reference.
b. PCIRC idmap is parsed and a list of idmaps is created which will
have PCIRC idmap -> ITS group nodes.
Each idmap is written by resolving ITS offset from the map saved in
previous step.
Signed-off-by: Manish Jaggi <mjaggi@cavium.com>
---
xen/arch/arm/acpi/iort.c | 172 ++++++++++++++++++++++++++++++++++++++++++++
xen/arch/arm/domain_build.c | 38 ++++++++++
xen/include/asm-arm/acpi.h | 1 +
xen/include/asm-arm/iort.h | 3 +
4 files changed, 214 insertions(+)
diff --git a/xen/arch/arm/acpi/iort.c b/xen/arch/arm/acpi/iort.c
index 01914cb..d506adb 100644
--- a/xen/arch/arm/acpi/iort.c
+++ b/xen/arch/arm/acpi/iort.c
@@ -240,3 +240,175 @@ int estimate_iort_size(size_t *iort_size)
err:
return -EINVAL;
}
+
+void write_itsgroup_nodes (struct acpi_table_iort *hwdom_iort,
+ struct acpi_table_iort *fw_iort,
+ uint64_t *ofstmap, unsigned int *pos)
+{
+ int i;
+ struct acpi_iort_node *node = NULL;
+ /* Iterate over all its groups in firmware IORT table
+ * and copy them in hwdom iort table.
+ */
+
+ /* First Node */
+ node = (struct acpi_iort_node *)((u8*)fw_iort + fw_iort->node_offset);
+ for ( i = 0; i < fw_iort->node_count; i++ )
+ {
+ if ( node->type == ACPI_IORT_NODE_ITS_GROUP )
+ {
+ u32 l,u;
+ /* Copy ITS group node into dom0 IORT Table */
+ ACPI_MEMCPY((u8*)hwdom_iort + *pos, node, node->length);
+
+ /* keep the new and old offsets, this would resolve smmu idarray's
+ * output ref offsets to the new offsets in hwdom iort table */
+ l = (uint32_t)((u8*)node - (u8*)fw_iort);
+ u = *pos ;
+ *ofstmap = ((uint64_t)(u) << 32)| l;
+
+ hwdom_iort->node_count++;
+ *pos += node->length;
+ ofstmap++;
+ }
+ node = (struct acpi_iort_node *)((u8*)node + node->length);
+ }
+}
+
+unsigned int write_single_pcirc_node(u8 *hwdom_iort, unsigned int pos,
+ struct acpi_iort_node *node,
+ struct list_head *new_idmap_list)
+{
+ unsigned int sz;
+ struct acpi_iort_node *local;
+ struct pcirc_idmap *pidmap;
+ local = (struct acpi_iort_node *)(hwdom_iort + pos);
+
+ /* write the pci_rc node */
+ sz = sizeof(struct acpi_iort_node) -1 +
+ /* -1 as acpi_iort_node has an extra char */
+ sizeof (struct acpi_iort_root_complex) ;
+ ACPI_MEMCPY(hwdom_iort + pos, node, sz);
+
+ pos += sz;
+ local->mapping_offset = sz;
+ local->mapping_count = 0;
+ local->length = sz;
+
+ list_for_each_entry(pidmap, new_idmap_list, entry)
+ {
+ ACPI_MEMCPY(hwdom_iort + pos, &pidmap->idmap,
+ sizeof(struct acpi_iort_id_mapping));
+ pos += sizeof(struct acpi_iort_id_mapping);
+ local->mapping_count++;
+ local->length += sizeof(struct acpi_iort_id_mapping);
+ }
+
+ return pos;
+}
+
+int write_pcirc_nodes(struct acpi_table_iort *hwdom_iort,
+ struct acpi_table_iort *fw_iort,
+ uint64_t *its_ofstmap, unsigned int *pos)
+{
+ int i, j, ret;
+ struct acpi_iort_node *node = NULL;
+
+ /* Iterate over all PCI_Nodes */
+ /* First Node */
+ node = (struct acpi_iort_node *)((u8*)fw_iort + fw_iort->node_offset);
+ for ( i = 0; i < fw_iort->node_count; i++ )
+ {
+ if ( node->type == ACPI_IORT_NODE_PCI_ROOT_COMPLEX )
+ {
+ struct pcirc_idmap *pidmap;
+ struct list_head new_idmap_list;
+ INIT_LIST_HEAD(&new_idmap_list);
+
+ /* hide smmu nodes and update new_idmap_list with output
+ * refrence of pci idarray as its group
+ */
+ ret = scan_idarray((u8*)fw_iort, node, &new_idmap_list, 0, NULL);
+ if ( ret ) {
+ printk("%s: scan_idarry Failed \r\n", __func__);
+ goto end;
+ }
+
+ /* fixup its_group offsets as per new iort table */
+ list_for_each_entry(pidmap, &new_idmap_list, entry)
+ {
+ /* search output reference offset in its_ofmap
+ * and replace with new offset
+ */
+ for (j =0; j < fw_iort->node_count; j++)
+ {
+ if( !its_ofstmap[j] )
+ break;
+
+ if(pidmap->idmap.output_reference
+ == (its_ofstmap[j] & 0xffffffff))
+ {
+ pidmap->idmap.output_reference = its_ofstmap[j] >> 32;
+ break;
+ }
+
+ }
+ }
+
+ *pos = write_single_pcirc_node((u8*)hwdom_iort, *pos, node,
+ &new_idmap_list);
+ hwdom_iort->node_count++;
+
+ /* free new_idmap_list */
+ list_for_each_entry(pidmap, &new_idmap_list, entry)
+ xfree(pidmap);
+ }
+ /* Next Node */
+ node = (struct acpi_iort_node *)((u8*)node + node->length);
+ }
+ return 0;
+end:
+ return -EINVAL;
+}
+
+/*
+ * Prepares IORT table for hardware domain, removing all the smmu nodes.
+ * IORT table for hardware domain will have the following structure.
+ * [IORT Header]
+ * [ITS Group Nodes]
+ * [PCI RC Node -N]
+ */
+int prepare_iort(struct acpi_table_iort *fw_iort,
+ struct acpi_table_iort *hwdom_iort,
+ unsigned int *iort_size)
+{
+ unsigned int pos; /* offset into local iort table */
+ uint64_t *its_ofstmap;
+
+ pos = sizeof(struct acpi_table_iort);
+
+ its_ofstmap = xzalloc_array(uint64_t, fw_iort->node_count);
+ if (!its_ofstmap)
+ goto end;
+
+ /* Copy FW iort header, node_count and node_offset updated later */
+ ACPI_MEMCPY(hwdom_iort, fw_iort, sizeof(struct acpi_table_iort));
+ hwdom_iort->node_offset = pos;
+ hwdom_iort->node_count = 0;
+
+ write_itsgroup_nodes(hwdom_iort, fw_iort, its_ofstmap, &pos);
+
+ if( write_pcirc_nodes(hwdom_iort, fw_iort, its_ofstmap, &pos) )
+ {
+ printk("Error in write_pcirc_nodes \r\n");
+ goto end;
+ }
+
+ hwdom_iort->header.length = pos;
+
+ xfree(its_ofstmap);
+ *iort_size = pos;
+ return 0;
+end:
+ return -EINVAL;
+}
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 0328926..0fcc206 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -1653,6 +1653,9 @@ static int acpi_create_xsdt(struct domain *d, struct membank tbl_add[])
ACPI_SIG_MADT, tbl_add[TBL_MADT].start);
xsdt->table_offset_entry[entry_count] = tbl_add[TBL_STAO].start;
+ acpi_xsdt_modify_entry(xsdt->table_offset_entry, entry_count,
+ ACPI_SIG_IORT, tbl_add[TBL_IORT].start);
+
xsdt->header.length = table_size;
checksum = acpi_tb_checksum(ACPI_CAST_PTR(u8, xsdt), table_size);
xsdt->header.checksum -= checksum;
@@ -1701,6 +1704,37 @@ static int acpi_create_stao(struct domain *d, struct membank tbl_add[])
return 0;
}
+static int acpi_create_iort(struct domain *d, struct membank tbl_add[])
+{
+ struct acpi_table_iort *fw_table;
+ struct acpi_table_iort *hwdom_table;
+ acpi_status status;
+ u32 size;
+
+ status = acpi_get_table(ACPI_SIG_IORT, 0,
+ (struct acpi_table_header **)&fw_table);
+ if ( ACPI_FAILURE(status) )
+ {
+ printk("Failed to get IORT table\n");
+ return -EINVAL;
+ }
+
+ tbl_add[TBL_IORT].start = d->arch.efi_acpi_gpa
+ + acpi_get_table_offset(tbl_add, TBL_IORT);
+ hwdom_table = d->arch.efi_acpi_table
+ + acpi_get_table_offset(tbl_add, TBL_IORT);
+
+ if ( prepare_iort(fw_table, hwdom_table, &size) )
+ {
+ printk("Failed to write IORT table\n");
+ return -EINVAL;
+ }
+ tbl_add[TBL_IORT].size = size;
+
+ return 0;
+}
+
+
static int acpi_create_madt(struct domain *d, struct membank tbl_add[])
{
struct acpi_table_header *table = NULL;
@@ -1895,6 +1929,10 @@ static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
if ( rc != 0 )
return rc;
+ rc = acpi_create_iort(d, tbl_add);
+ if ( rc != 0 )
+ return rc;
+
rc = acpi_create_xsdt(d, tbl_add);
if ( rc != 0 )
return rc;
diff --git a/xen/include/asm-arm/acpi.h b/xen/include/asm-arm/acpi.h
index 9f954d3..1cc0167 100644
--- a/xen/include/asm-arm/acpi.h
+++ b/xen/include/asm-arm/acpi.h
@@ -36,6 +36,7 @@ typedef enum {
TBL_FADT,
TBL_MADT,
TBL_STAO,
+ TBL_IORT,
TBL_XSDT,
TBL_RSDP,
TBL_EFIT,
diff --git a/xen/include/asm-arm/iort.h b/xen/include/asm-arm/iort.h
index 3b4cd7b..229fe7b 100644
--- a/xen/include/asm-arm/iort.h
+++ b/xen/include/asm-arm/iort.h
@@ -3,6 +3,9 @@
int estimate_iort_size(size_t *iort_size);
+int prepare_iort(struct acpi_table_iort *fw_iort_table,
+ struct acpi_table_iort *hwdom_table, u32 *iort_size);
+
#endif /* _XEN_ASM_IORT_H */
/*
* Local variables:
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-09-11 21:33 [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table mjaggi
2017-09-11 21:33 ` [PATCH 1/2] ARM: ACPI: IORT: Estimate the size of hardware domain " mjaggi
2017-09-11 21:33 ` [PATCH 2/2] ARM: ACPI: IORT: Write Hardware domain's " mjaggi
@ 2017-09-22 14:12 ` Andre Przywara
2017-09-25 4:22 ` Manish Jaggi
2 siblings, 1 reply; 13+ messages in thread
From: Andre Przywara @ 2017-09-22 14:12 UTC (permalink / raw)
To: mjaggi, xen-devel; +Cc: tomasz.nowicki, julien.grall, sstabellini, Manish Jaggi
Hi Manish,
On 11/09/17 22:33, mjaggi@caviumnetworks.com wrote:
> From: Manish Jaggi <mjaggi@cavium.com>
>
> The set is divided into two patches. First one calculates the size of IORT
> while second one writes the IORT table itself.
It would be good if you could give a quick introduction *why* this set
is needed here (and introduce IORT to the casual reader).
In general some more high-level documentation on your functions would be
good, as it took me quite some time to understand what each function does.
So my understanding is:
phase 1:
- go over each entry in each RC node
- if that points to an SMMU node, go over each outgoing ITS entry and
find overlaps with this RC entry
- for each overlap create a new entry in a list with this RC
pointing to the ITS directly
phase 2, creating the new IORT
- go over each RC node
- if that points to an ITS, copy through IORT entries
- if that points to an SMMU, replace with the remapped entries
- go over each ITS node
- copy through IORT entries
So I believe this would do the trick and you end up with an efficient
representation of the IORT without SMMUs - at least for RC nodes.
After some brainstorming with Julien we found two problems:
1) This only covers RC nodes, but not "named components" (platform
devices), which we will need. That should be fixable by removing the
hardcoded IORT node types in the code and treating NC nodes like RC nodes.
2) Eventually we will need *virtual* deviceID support, for DomUs. Now we
could start introducing that already, also doing some virtual mapping
for Dom0. The ITS code would then translate each virtual device ID that
Dom0 requests into a hardware device ID.
I agree that this means a lot more work, but we will need it anyway.
I think 1) can be solved using this series as a base. I have quite some
comments ready for the patches, shall we follow this route.
2) obviously would change the game completely. We need to sit down and
design this properly. Probably this means that Xen parses the IORT and
builds internal representations of the mappings, which are consulted as
needed when passing through devices. The guest's (that would include
Dom0) IORT would then be generated completely from scratch.
I would like to hear your opinion on this. I will try to discuss the
feasibility of 2) with people at Connect. It would be good if we could
decide whether this is the way to go or we should use a solution based
on this series.
Cheers,
Andre.
> patch1: estimates size of hardware domain IORT table by parsing all
> the pcirc nodes and their idmaps, and thereby calculating size by
> removing smmu nodes.
>
> Hardware domain IORT table will have only ITS and PCIRC nodes, and PCIRC
> nodes' idmap will have output refrences to ITS group nodes.
>
> patch 2: The steps are:
> a. First ITS group nodes are written and their offsets are saved
> along with the respective offsets from the firmware table.
> This is required when smmu node is hidden and smmu node still points
> to the old output_reference.
>
> b. PCIRC idmap is parsed and a list of idmaps is created which will
> have PCIRC idmap -> ITS group nodes.
> Each idmap is written by resolving ITS offset from the map saved in
> previous step.
>
> Changes wrt v1:
> No assumption is made wrt format of IORT / hw support
>
> Manish Jaggi (2):
> ARM: ACPI: IORT: Estimate the size of hardware domain IORT table
> ARM: ACPI: IORT: Write Hardware domain's IORT table
>
> xen/arch/arm/acpi/Makefile | 1 +
> xen/arch/arm/acpi/iort.c | 414 ++++++++++++++++++++++++++++++++++++++++++++
> xen/arch/arm/domain_build.c | 49 +++++-
> xen/include/asm-arm/acpi.h | 1 +
> xen/include/asm-arm/iort.h | 17 ++
> 5 files changed, 481 insertions(+), 1 deletion(-)
> create mode 100644 xen/arch/arm/acpi/iort.c
> create mode 100644 xen/include/asm-arm/iort.h
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-09-22 14:12 ` [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware " Andre Przywara
@ 2017-09-25 4:22 ` Manish Jaggi
2017-10-03 18:42 ` Julien Grall
0 siblings, 1 reply; 13+ messages in thread
From: Manish Jaggi @ 2017-09-25 4:22 UTC (permalink / raw)
To: Andre Przywara, xen-devel
Cc: tomasz.nowicki, julien.grall, sstabellini, Manish Jaggi
Hi Andre,
On 9/22/2017 7:42 PM, Andre Przywara wrote:
> Hi Manish,
>
> On 11/09/17 22:33, mjaggi@caviumnetworks.com wrote:
>> From: Manish Jaggi <mjaggi@cavium.com>
>>
>> The set is divided into two patches. First one calculates the size of IORT
>> while second one writes the IORT table itself.
> It would be good if you could give a quick introduction *why* this set
> is needed here (and introduce IORT to the casual reader).
> In general some more high-level documentation on your functions would be
> good, as it took me quite some time to understand what each function does.
ok, will add more documentation.
> So my understanding is:
> phase 1:
> - go over each entry in each RC node
Rather than each entry (which could be a large number) I am taking the
complete range and checking it with the same logic.
If the ID range is a subset or a super-set of id range in smmu, new id
range is created.
So if pci_rc node has an id map {p_input-base,p_output-base,p_out_ref,
p_count} and it an output reference to smmu node with id-map
{s_input-base, s_output-base,s_out_ref, s_count}, based on the the the
s_count and s_input/p_output the new id-map is created with {p_input,
s_output, s_out_ref, adjusted_count}
update_id_mapping function does that.
So I am following the same logic. We can chat over IRC / I can give a
code walk-through ...
> - if that points to an SMMU node, go over each outgoing ITS entry and
> find overlaps with this RC entry
> - for each overlap create a new entry in a list with this RC
> pointing to the ITS directly
>
> phase 2, creating the new IORT
> - go over each RC node
> - if that points to an ITS, copy through IORT entries
> - if that points to an SMMU, replace with the remapped entries
> - go over each ITS node
> - copy through IORT entries
Thats exactly what this patch does.
> So I believe this would do the trick and you end up with an efficient
> representation of the IORT without SMMUs - at least for RC nodes.
>
> After some brainstorming with Julien we found two problems:
> 1) This only covers RC nodes, but not "named components" (platform
> devices), which we will need. That should be fixable by removing the
> hardcoded IORT node types in the code and treating NC nodes like RC nodes.
Yes, so first we can take this as a base, once this is ok, I can add
support for named components.
> 2) Eventually we will need *virtual* deviceID support, for DomUs. Now we
> could start introducing that already, also doing some virtual mapping
> for Dom0. The ITS code would then translate each virtual device ID that
> Dom0 requests into a hardware device ID.
> I agree that this means a lot more work, but we will need it anyway.
>
> I think 1) can be solved using this series as a base. I have quite some
> comments ready for the patches, shall we follow this route.
>
> 2) obviously would change the game completely. We need to sit down and
> design this properly. Probably this means that Xen parses the IORT and
> builds internal representations of the mappings, which are consulted as
> needed when passing through devices. The guest's (that would include
> Dom0) IORT would then be generated completely from scratch.
>
> I would like to hear your opinion on this. I will try to discuss the
> feasibility of 2) with people at Connect. It would be good if we could
> decide whether this is the way to go or we should use a solution based
> on this series.
>
> Cheers,
> Andre.
>
>
>> patch1: estimates size of hardware domain IORT table by parsing all
>> the pcirc nodes and their idmaps, and thereby calculating size by
>> removing smmu nodes.
>>
>> Hardware domain IORT table will have only ITS and PCIRC nodes, and PCIRC
>> nodes' idmap will have output refrences to ITS group nodes.
>>
>> patch 2: The steps are:
>> a. First ITS group nodes are written and their offsets are saved
>> along with the respective offsets from the firmware table.
>> This is required when smmu node is hidden and smmu node still points
>> to the old output_reference.
>>
>> b. PCIRC idmap is parsed and a list of idmaps is created which will
>> have PCIRC idmap -> ITS group nodes.
>> Each idmap is written by resolving ITS offset from the map saved in
>> previous step.
>>
>> Changes wrt v1:
>> No assumption is made wrt format of IORT / hw support
>>
>> Manish Jaggi (2):
>> ARM: ACPI: IORT: Estimate the size of hardware domain IORT table
>> ARM: ACPI: IORT: Write Hardware domain's IORT table
>>
>> xen/arch/arm/acpi/Makefile | 1 +
>> xen/arch/arm/acpi/iort.c | 414 ++++++++++++++++++++++++++++++++++++++++++++
>> xen/arch/arm/domain_build.c | 49 +++++-
>> xen/include/asm-arm/acpi.h | 1 +
>> xen/include/asm-arm/iort.h | 17 ++
>> 5 files changed, 481 insertions(+), 1 deletion(-)
>> create mode 100644 xen/arch/arm/acpi/iort.c
>> create mode 100644 xen/include/asm-arm/iort.h
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-09-25 4:22 ` Manish Jaggi
@ 2017-10-03 18:42 ` Julien Grall
2017-10-04 5:22 ` Manish Jaggi
0 siblings, 1 reply; 13+ messages in thread
From: Julien Grall @ 2017-10-03 18:42 UTC (permalink / raw)
To: Manish Jaggi, Andre Przywara, xen-devel
Cc: tomasz.nowicki, sstabellini, Manish Jaggi
Hello,
On 25/09/17 05:22, Manish Jaggi wrote:
> On 9/22/2017 7:42 PM, Andre Przywara wrote:
>> Hi Manish,
>>
>> On 11/09/17 22:33, mjaggi@caviumnetworks.com wrote:
>>> From: Manish Jaggi <mjaggi@cavium.com>
>>>
>>> The set is divided into two patches. First one calculates the size of
>>> IORT
>>> while second one writes the IORT table itself.
>> It would be good if you could give a quick introduction *why* this set
>> is needed here (and introduce IORT to the casual reader).
>> In general some more high-level documentation on your functions would be
>> good, as it took me quite some time to understand what each function
>> does.
> ok, will add more documentation.
>> So my understanding is:
>> phase 1:
>> - go over each entry in each RC node
> Rather than each entry (which could be a large number) I am taking the
> complete range and checking it with the same logic.
> If the ID range is a subset or a super-set of id range in smmu, new id
> range is created.
>
> So if pci_rc node has an id map {p_input-base,p_output-base,p_out_ref,
> p_count} and it an output reference to smmu node with id-map
> {s_input-base, s_output-base,s_out_ref, s_count}, based on the the the
> s_count and s_input/p_output the new id-map is created with {p_input,
> s_output, s_out_ref, adjusted_count}
>
> update_id_mapping function does that.
>
> So I am following the same logic. We can chat over IRC / I can give a
> code walk-through ...
>
>> - if that points to an SMMU node, go over each outgoing ITS entry and
>> find overlaps with this RC entry
>> - for each overlap create a new entry in a list with this RC
>> pointing to the ITS directly
>>
>> phase 2, creating the new IORT
>> - go over each RC node
>> - if that points to an ITS, copy through IORT entries
>> - if that points to an SMMU, replace with the remapped entries
>> - go over each ITS node
>> - copy through IORT entries
> Thats exactly what this patch does.
>> So I believe this would do the trick and you end up with an efficient
>> representation of the IORT without SMMUs - at least for RC nodes.
>>
>> After some brainstorming with Julien we found two problems:
>> 1) This only covers RC nodes, but not "named components" (platform
>> devices), which we will need. That should be fixable by removing the
>> hardcoded IORT node types in the code and treating NC nodes like RC
>> nodes.
> Yes, so first we can take this as a base, once this is ok, I can add
> support for named components.
>> 2) Eventually we will need *virtual* deviceID support, for DomUs. Now we
I am a bit surprised that you answered to the e-mail but didn't provide
any opinion on 2).
>> could start introducing that already, also doing some virtual mapping
>> for Dom0. The ITS code would then translate each virtual device ID that
>> Dom0 requests into a hardware device ID.
>> I agree that this means a lot more work, but we will need it anyway.
I am a bit surprised that you answered to the e-mail but didn't provide
any opinion on 2).
>>
>> I think 1) can be solved using this series as a base. I have quite some
>> comments ready for the patches, shall we follow this route.
>>
>> 2) obviously would change the game completely. We need to sit down and
>> design this properly. Probably this means that Xen parses the IORT and
>> builds internal representations of the mappings, which are consulted as
>> needed when passing through devices. The guest's (that would include
>> Dom0) IORT would then be generated completely from scratch.
>>
>> I would like to hear your opinion on this. I will try to discuss the
>> feasibility of 2) with people at Connect. It would be good if we could
>> decide whether this is the way to go or we should use a solution based
>> on this series.
Andre, Stefano and I had a chat during connect about how we want to see
IORT support in Xen. In the near future, IORT will be used for different
components accross the hypervisor (ITS, SMMU...) and as a way to
communicate the topology to the guests.
The IORT for the hardware domain is just a specific case as it is based
pre-existing information. But because of removing nodes (e.g SMMU nodes
and probably the PMU nodes), it is basically a full re-write.
So I would consider of full separate the logic of generating the IORT
table from the host IORT table. By that I mean not browsing the host
IORT when generating the host.
This has two benefits:
1) The code generation could be re-used for generating the guest IORT
later on.
2) See 2) from Andre
3) We could decide in a finer grain which devices (e.g platform device)
Dom0 can see.
So, IHMO, we should take a different approach from this series and
extending the scope of it. Rather than focusing on only IORT for the
hardware, I would be better to see IORT as a whole. I.e how IORT will
interact with the hypervisor?
For instance, likely you would need to parse the IORT in quite a few
places in Xen. It would be better to get IORT parsed only once and store
the information in Xen like data-structures.
This require more work than this current scope of this series. But I
think it would helful for future work such as PCI passthrough support.
Any opinions?
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-10-03 18:42 ` Julien Grall
@ 2017-10-04 5:22 ` Manish Jaggi
2017-10-06 14:24 ` Julien Grall
0 siblings, 1 reply; 13+ messages in thread
From: Manish Jaggi @ 2017-10-04 5:22 UTC (permalink / raw)
To: Julien Grall, Andre Przywara, xen-devel
Cc: tomasz.nowicki, sstabellini, Manish Jaggi
Hello Julien,
On 10/4/2017 12:12 AM, Julien Grall wrote:
> Hello,
>
> On 25/09/17 05:22, Manish Jaggi wrote:
>> On 9/22/2017 7:42 PM, Andre Przywara wrote:
>>> Hi Manish,
>>>
>>> On 11/09/17 22:33, mjaggi@caviumnetworks.com wrote:
>>>> From: Manish Jaggi <mjaggi@cavium.com>
>>>>
>>>> The set is divided into two patches. First one calculates the size
>>>> of IORT
>>>> while second one writes the IORT table itself.
>>> It would be good if you could give a quick introduction *why* this set
>>> is needed here (and introduce IORT to the casual reader).
>>> In general some more high-level documentation on your functions
>>> would be
>>> good, as it took me quite some time to understand what each function
>>> does.
>> ok, will add more documentation.
>>> So my understanding is:
>>> phase 1:
>>> - go over each entry in each RC node
>> Rather than each entry (which could be a large number) I am taking
>> the complete range and checking it with the same logic.
>> If the ID range is a subset or a super-set of id range in smmu, new
>> id range is created.
>>
>> So if pci_rc node has an id map
>> {p_input-base,p_output-base,p_out_ref, p_count} and it an output
>> reference to smmu node with id-map
>> {s_input-base, s_output-base,s_out_ref, s_count}, based on the the
>> the s_count and s_input/p_output the new id-map is created with
>> {p_input, s_output, s_out_ref, adjusted_count}
>>
>> update_id_mapping function does that.
>>
>> So I am following the same logic. We can chat over IRC / I can give a
>> code walk-through ...
>>
>>> - if that points to an SMMU node, go over each outgoing ITS entry and
>>> find overlaps with this RC entry
>>> - for each overlap create a new entry in a list with this RC
>>> pointing to the ITS directly
>>>
>>> phase 2, creating the new IORT
>>> - go over each RC node
>>> - if that points to an ITS, copy through IORT entries
>>> - if that points to an SMMU, replace with the remapped entries
>>> - go over each ITS node
>>> - copy through IORT entries
>> Thats exactly what this patch does.
What are you comments on the current patch approach to hide smmu nodes.
I have answered to your comments, see below.
IMHO we can reuse most of the fixup code here.
>>> So I believe this would do the trick and you end up with an efficient
>>> representation of the IORT without SMMUs - at least for RC nodes.
>>>
>>> After some brainstorming with Julien we found two problems:
>>> 1) This only covers RC nodes, but not "named components" (platform
>>> devices), which we will need. That should be fixable by removing the
>>> hardcoded IORT node types in the code and treating NC nodes like RC
>>> nodes.
>> Yes, so first we can take this as a base, once this is ok, I can add
>> support for named components.
>>> 2) Eventually we will need *virtual* deviceID support, for DomUs.
>>> Now we
> I am a bit surprised that you answered to the e-mail but didn't
> provide any opinion on 2).
Apologies for that.
>>> could start introducing that already, also doing some virtual mapping
>>> for Dom0. The ITS code would then translate each virtual device ID that
>>> Dom0 requests into a hardware device ID.
>>> I agree that this means a lot more work, but we will need it anyway.
>
> I am a bit surprised that you answered to the e-mail but didn't
> provide any opinion on 2).
Apologies for that. Sorry to surprise you twice :)
IMHO It was a bit obvious for DomU and I was waiting to hear what other
would say on this.
as (2) below.
Moreover we need to discuss IORT generation for DomU
- could be done by xl tools
or xen should do it.
Also this is the part of PCI passthrough flow so that also might change
few things.
But from pov of dom0 smmu hiding, it is a different flow and is coupled
with PCI PT.
>
>>>
>>> I think 1) can be solved using this series as a base. I have quite some
>>> comments ready for the patches, shall we follow this route.
>>>
>>> 2) obviously would change the game completely. We need to sit down and
>>> design this properly. Probably this means that Xen parses the IORT and
>>> builds internal representations of the mappings,
Can you please add more detail on the internal representations of the
mappings.
IIUC the information is already there in ACPI tables, would it not add
extra overhead of abstractions to maintain.
Enumeration of PCI devices would generate a pci list which would be
anyways separate.
>>> which are consulted as
>>> needed when passing through devices. The guest's (that would include
>>> Dom0) IORT would then be generated completely from scratch.
>>>
I have a different opinion here, dom0 IORT would is most cases be very
close to host IORT sans smmu nodes and few platform devices.
And which platform devices to hide would probably depend on the xen
command line,
For instance for dom0 we would copy ITS information while for domU it
would have to be generated, so scratch would be more for domU.
We could have a common code for creating IORT structure but it would be
a bit complex code with lot of abstractions and callbacks, so I suggest
that keeping code simpler would be better.
>>> I would like to hear your opinion on this. I will try to discuss the
>>> feasibility of 2) with people at Connect. It would be good if we could
>>> decide whether this is the way to go or we should use a solution based
>>> on this series.
>
> Andre, Stefano and I had a chat during connect about how we want to
> see IORT support in Xen. In the near future, IORT will be used for
> different components accross the hypervisor (ITS, SMMU...) and as a
> way to
> communicate the topology to the guests.
Are we thinking about virtual SMMU here as well?
>
> The IORT for the hardware domain is just a specific case as it is
> based pre-existing information. But because of removing nodes (e.g
> SMMU nodes and probably the PMU nodes), it is basically a full re-write.
>
> So I would consider of full separate the logic of generating the IORT
> table from the host IORT table. By that I mean not browsing the host
> IORT when generating the host.
>
by "the host" you mean dom0 IORT ?
> This has two benefits:
> 1) The code generation could be re-used for generating the guest
> IORT later on.
See my comment above
> 2) See 2) from Andre
> 3) We could decide in a finer grain which devices (e.g platform
> device) Dom0 can see.
ok,
>
> So, IHMO, we should take a different approach from this series and
> extending the scope of it. Rather than focusing on only IORT for the
> hardware, I would be better to see IORT as a whole. I.e how IORT will
> interact with the hypervisor?
>
> For instance, likely you would need to parse the IORT in quite a few
> places in Xen. It would be better to get IORT parsed only once and
> store the information in Xen like data-structures.
>
I am thinking of reusing much of ACPI tables for that and introducing
less abstractions.
> This require more work than this current scope of this series. But I
> think it would helful for future work such as PCI passthrough support.
>
> Any opinions?
>
for DomUs it is tied to PCI PT, so both design should evolve together.
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-10-04 5:22 ` Manish Jaggi
@ 2017-10-06 14:24 ` Julien Grall
2017-10-12 6:11 ` Manish Jaggi
0 siblings, 1 reply; 13+ messages in thread
From: Julien Grall @ 2017-10-06 14:24 UTC (permalink / raw)
To: Manish Jaggi, Andre Przywara, xen-devel
Cc: tomasz.nowicki, sstabellini, Manish Jaggi
Hello,
On 04/10/17 06:22, Manish Jaggi wrote:
> On 10/4/2017 12:12 AM, Julien Grall wrote:
>> On 25/09/17 05:22, Manish Jaggi wrote:
>>> On 9/22/2017 7:42 PM, Andre Przywara wrote:
>>>> Hi Manish,
>>>>
>>>> On 11/09/17 22:33, mjaggi@caviumnetworks.com wrote:
>>>>> From: Manish Jaggi <mjaggi@cavium.com>
>>>>>
>>>>> The set is divided into two patches. First one calculates the size
>>>>> of IORT
>>>>> while second one writes the IORT table itself.
>>>> It would be good if you could give a quick introduction *why* this set
>>>> is needed here (and introduce IORT to the casual reader).
>>>> In general some more high-level documentation on your functions
>>>> would be
>>>> good, as it took me quite some time to understand what each function
>>>> does.
>>> ok, will add more documentation.
>>>> So my understanding is:
>>>> phase 1:
>>>> - go over each entry in each RC node
>>> Rather than each entry (which could be a large number) I am taking
>>> the complete range and checking it with the same logic.
>>> If the ID range is a subset or a super-set of id range in smmu, new
>>> id range is created.
>>>
>>> So if pci_rc node has an id map
>>> {p_input-base,p_output-base,p_out_ref, p_count} and it an output
>>> reference to smmu node with id-map
>>> {s_input-base, s_output-base,s_out_ref, s_count}, based on the the
>>> the s_count and s_input/p_output the new id-map is created with
>>> {p_input, s_output, s_out_ref, adjusted_count}
>>>
>>> update_id_mapping function does that.
>>>
>>> So I am following the same logic. We can chat over IRC / I can give a
>>> code walk-through ...
>>>
>>>> - if that points to an SMMU node, go over each outgoing ITS entry and
>>>> find overlaps with this RC entry
>>>> - for each overlap create a new entry in a list with this RC
>>>> pointing to the ITS directly
>>>>
>>>> phase 2, creating the new IORT
>>>> - go over each RC node
>>>> - if that points to an ITS, copy through IORT entries
>>>> - if that points to an SMMU, replace with the remapped entries
>>>> - go over each ITS node
>>>> - copy through IORT entries
>>> Thats exactly what this patch does.
> What are you comments on the current patch approach to hide smmu nodes.
> I have answered to your comments, see below.
I am not sure to understand the 2 sentences above. What are they related?
> IMHO we can reuse most of the fixup code here.
That's your choice as long as it is properly documented and fits the end
goal.
>
>>>> So I believe this would do the trick and you end up with an efficient
>>>> representation of the IORT without SMMUs - at least for RC nodes.
>>>>
>>>> After some brainstorming with Julien we found two problems:
>>>> 1) This only covers RC nodes, but not "named components" (platform
>>>> devices), which we will need. That should be fixable by removing the
>>>> hardcoded IORT node types in the code and treating NC nodes like RC
>>>> nodes.
>>> Yes, so first we can take this as a base, once this is ok, I can add
>>> support for named components.
>>>> 2) Eventually we will need *virtual* deviceID support, for DomUs.
>>>> Now we
>> I am a bit surprised that you answered to the e-mail but didn't
>> provide any opinion on 2).
> Apologies for that.
>>>> could start introducing that already, also doing some virtual mapping
>>>> for Dom0. The ITS code would then translate each virtual device ID that
>>>> Dom0 requests into a hardware device ID.
>>>> I agree that this means a lot more work, but we will need it anyway.
>>
>> I am a bit surprised that you answered to the e-mail but didn't
>> provide any opinion on 2).
> Apologies for that. Sorry to surprise you twice :)
Damm, I moved the sentence but forgot to drop the original one.
>
> IMHO It was a bit obvious for DomU and I was waiting to hear what other
> would say on this.
> as (2) below.
> Moreover we need to discuss IORT generation for DomU
> - could be done by xl tools
> or xen should do it.
The ACPI tables for DomU are generated by the toolstack today. So I
don't see why we would change that to support IORT.
However, you can have a file shared between the toolstack and Xen and
contain the generation of IORT.
For instance, this is what we already does with libelf (see common/libelf).
I am not asking to write the DomU support, but at least have a full
separation between the Parsing and Generation. So we could easily adapt
and re-use the code when we get the DomU support.
>
> Also this is the part of PCI passthrough flow so that also might change
> few things.
>
> But from pov of dom0 smmu hiding, it is a different flow and is coupled
> with PCI PT.
>
>>
>>>>
>>>> I think 1) can be solved using this series as a base. I have quite some
>>>> comments ready for the patches, shall we follow this route.
>>>>
>>>> 2) obviously would change the game completely. We need to sit down and
>>>> design this properly. Probably this means that Xen parses the IORT and
>>>> builds internal representations of the mappings,
> Can you please add more detail on the internal representations of the
> mappings.
What exactly do you want? This is likely going to be decided once you
looked what is the expected interaction between IORT and Xen.
> IIUC the information is already there in ACPI tables, would it not add
> extra overhead of abstractions to maintain.
> Enumeration of PCI devices would generate a pci list which would be
> anyways separate.
>>>> which are consulted as
>>>> needed when passing through devices. The guest's (that would include
>>>> Dom0) IORT would then be generated completely from scratch.
>>>>
> I have a different opinion here, dom0 IORT would is most cases be very
> close to host IORT sans smmu nodes and few platform devices.
And also without PMUs nodes... Potentially in a long-term we may want to
add nodes (i.e Virtual PMU, virtual SMMU...). So it may not be that
close of the original host IORT.
However, whether it is close or not does not much matter here. The goal
is to fully separate the parsing and the generation. So parsing could be
re-used for other bits of Xen. Similarly for the generation code.
> And which platform devices to hide would probably depend on the xen
> command line,
> For instance for dom0 we would copy ITS information while for domU it
> would have to be generated, so scratch would be more for domU.
> We could have a common code for creating IORT structure but it would be
> a bit complex code with lot of abstractions and callbacks, so I suggest
> that keeping code simpler would be better.
I am not sure to understand why you think it would create too much
abstractions. After all the IORT is just a series of nodes with a bunch
of informations. This could easily be described in a structure that
would be used to generate the tables.
In any case, we will need that for DomU. So why not doing it now?
>>>> I would like to hear your opinion on this. I will try to discuss the
>>>> feasibility of 2) with people at Connect. It would be good if we could
>>>> decide whether this is the way to go or we should use a solution based
>>>> on this series.
>>
>> Andre, Stefano and I had a chat during connect about how we want to
>> see IORT support in Xen. In the near future, IORT will be used for
>> different components accross the hypervisor (ITS, SMMU...) and as a
>> way to
>> communicate the topology to the guests.
> Are we thinking about virtual SMMU here as well?
It is a potential long-term usage.
>>
>> The IORT for the hardware domain is just a specific case as it is
>> based pre-existing information. But because of removing nodes (e.g
>> SMMU nodes and probably the PMU nodes), it is basically a full re-write.
>>
>> So I would consider of full separate the logic of generating the IORT
>> table from the host IORT table. By that I mean not browsing the host
>> IORT when generating the host.
>>
> by "the host" you mean dom0 IORT ?
yes.
>> This has two benefits:
>> 1) The code generation could be re-used for generating the guest
>> IORT later on.
> See my comment above
>> 2) See 2) from Andre
>> 3) We could decide in a finer grain which devices (e.g platform
>> device) Dom0 can see.
> ok,
>>
>> So, IHMO, we should take a different approach from this series and
>> extending the scope of it. Rather than focusing on only IORT for the
>> hardware, I would be better to see IORT as a whole. I.e how IORT will
>> interact with the hypervisor?
>>
>> For instance, likely you would need to parse the IORT in quite a few
>> places in Xen. It would be better to get IORT parsed only once and
>> store the information in Xen like data-structures.
>>
> I am thinking of reusing much of ACPI tables for that and introducing
> less abstractions.
>> This require more work than this current scope of this series. But I
>> think it would helful for future work such as PCI passthrough support.
>>
>> Any opinions?
>>
> for DomUs it is tied to PCI PT, so both design should evolve together.
Why that? The generation of IORT is pretty standard.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-10-06 14:24 ` Julien Grall
@ 2017-10-12 6:11 ` Manish Jaggi
2017-10-12 11:04 ` Julien Grall
0 siblings, 1 reply; 13+ messages in thread
From: Manish Jaggi @ 2017-10-12 6:11 UTC (permalink / raw)
To: Julien Grall, Andre Przywara, xen-devel
Cc: tomasz.nowicki, sstabellini, Manish Jaggi
On 10/6/2017 7:54 PM, Julien Grall wrote:
> Hello,
>
> On 04/10/17 06:22, Manish Jaggi wrote:
>> On 10/4/2017 12:12 AM, Julien Grall wrote:
>>> On 25/09/17 05:22, Manish Jaggi wrote:
>>>> On 9/22/2017 7:42 PM, Andre Przywara wrote:
>>>>> Hi Manish,
>>>>>
>>>>> On 11/09/17 22:33, mjaggi@caviumnetworks.com wrote:
>>>>>> From: Manish Jaggi <mjaggi@cavium.com>
>>>>>>
>>>>>> The set is divided into two patches. First one calculates the
>>>>>> size of IORT
>>>>>> while second one writes the IORT table itself.
>>>>> It would be good if you could give a quick introduction *why* this
>>>>> set
>>>>> is needed here (and introduce IORT to the casual reader).
>>>>> In general some more high-level documentation on your functions
>>>>> would be
>>>>> good, as it took me quite some time to understand what each
>>>>> function does.
>>>> ok, will add more documentation.
>>>>> So my understanding is:
>>>>> phase 1:
>>>>> - go over each entry in each RC node
>>>> Rather than each entry (which could be a large number) I am taking
>>>> the complete range and checking it with the same logic.
>>>> If the ID range is a subset or a super-set of id range in smmu, new
>>>> id range is created.
>>>>
>>>> So if pci_rc node has an id map
>>>> {p_input-base,p_output-base,p_out_ref, p_count} and it an output
>>>> reference to smmu node with id-map
>>>> {s_input-base, s_output-base,s_out_ref, s_count}, based on the the
>>>> the s_count and s_input/p_output the new id-map is created with
>>>> {p_input, s_output, s_out_ref, adjusted_count}
>>>>
>>>> update_id_mapping function does that.
>>>>
>>>> So I am following the same logic. We can chat over IRC / I can give
>>>> a code walk-through ...
>>>>
>>>>> - if that points to an SMMU node, go over each outgoing ITS
>>>>> entry and
>>>>> find overlaps with this RC entry
>>>>> - for each overlap create a new entry in a list with this RC
>>>>> pointing to the ITS directly
>>>>>
>>>>> phase 2, creating the new IORT
>>>>> - go over each RC node
>>>>> - if that points to an ITS, copy through IORT entries
>>>>> - if that points to an SMMU, replace with the remapped entries
>>>>> - go over each ITS node
>>>>> - copy through IORT entries
>>>> Thats exactly what this patch does.
>> What are you comments on the current patch approach to hide smmu nodes.
>> I have answered to your comments, see below.
>
> I am not sure to understand the 2 sentences above. What are they related?
>
>> IMHO we can reuse most of the fixup code here.
>
> That's your choice as long as it is properly documented and fits the
> end goal.
>
>>
>>>>> So I believe this would do the trick and you end up with an efficient
>>>>> representation of the IORT without SMMUs - at least for RC nodes.
>>>>>
>>>>> After some brainstorming with Julien we found two problems:
>>>>> 1) This only covers RC nodes, but not "named components" (platform
>>>>> devices), which we will need. That should be fixable by removing the
>>>>> hardcoded IORT node types in the code and treating NC nodes like
>>>>> RC nodes.
>>>> Yes, so first we can take this as a base, once this is ok, I can
>>>> add support for named components.
>>>>> 2) Eventually we will need *virtual* deviceID support, for DomUs.
>>>>> Now we
>>> I am a bit surprised that you answered to the e-mail but didn't
>>> provide any opinion on 2).
>> Apologies for that.
>>>>> could start introducing that already, also doing some virtual mapping
>>>>> for Dom0. The ITS code would then translate each virtual device ID
>>>>> that
>>>>> Dom0 requests into a hardware device ID.
>>>>> I agree that this means a lot more work, but we will need it anyway.
>>>
>>> I am a bit surprised that you answered to the e-mail but didn't
>>> provide any opinion on 2).
>> Apologies for that. Sorry to surprise you twice :)
>
> Damm, I moved the sentence but forgot to drop the original one.
>
>>
>> IMHO It was a bit obvious for DomU and I was waiting to hear what
>> other would say on this.
>> as (2) below.
>> Moreover we need to discuss IORT generation for DomU
>> - could be done by xl tools
>> or xen should do it.
>
> The ACPI tables for DomU are generated by the toolstack today. So I
> don't see why we would change that to support IORT.
>
> However, you can have a file shared between the toolstack and Xen and
> contain the generation of IORT.
>
> For instance, this is what we already does with libelf (see
> common/libelf).
This will amount to adding a function make_iort in libxl__prepare_acpi,
which would use the common code that is also use generate dom0
IORT (domain_build.c).
Correct ?
If we go by this logic, then libxl_prepare_acpi and domain_build.c
should use a common code for all acpi tables.
- Are you suggesting we change that as well and make it part of common code.
>
> I am not asking to write the DomU support, but at least have a full
> separation between the Parsing and Generation. So we could easily
> adapt and re-use the code when we get the DomU support.
>
I got your point, but as of today there is no code reuse for most of
apci_tables. So code result _only_ for IORT but not for acpi is not
correct approach.
>>
>> Also this is the part of PCI passthrough flow so that also might
>> change few things.
>>
>> But from pov of dom0 smmu hiding, it is a different flow and is
>> coupled with PCI PT.
>>
>>>
>>>>>
>>>>> I think 1) can be solved using this series as a base. I have quite
>>>>> some
>>>>> comments ready for the patches, shall we follow this route.
>>>>>
>>>>> 2) obviously would change the game completely. We need to sit down
>>>>> and
>>>>> design this properly. Probably this means that Xen parses the IORT
>>>>> and
>>>>> builds internal representations of the mappings,
>> Can you please add more detail on the internal representations of the
>> mappings.
>
> What exactly do you want? This is likely going to be decided once you
> looked what is the expected interaction between IORT and Xen.
More details on this line "Probably this means that Xen parses the IORT and
builds internal representations of the mappings,"
>
>> IIUC the information is already there in ACPI tables, would it not
>> add extra overhead of abstractions to maintain.
>> Enumeration of PCI devices would generate a pci list which would be
>> anyways separate.
>>>>> which are consulted as
>>>>> needed when passing through devices. The guest's (that would include
>>>>> Dom0) IORT would then be generated completely from scratch.
>>>>>
>> I have a different opinion here, dom0 IORT would is most cases be
>> very close to host IORT sans smmu nodes and few platform devices.
>
> And also without PMUs nodes... Potentially in a long-term we may want
> to add nodes (i.e Virtual PMU, virtual SMMU...). So it may not be that
> close of the original host IORT.
>
> However, whether it is close or not does not much matter here. The
> goal is to fully separate the parsing and the generation. So parsing
> could be re-used for other bits of Xen. Similarly for the generation
> code.
>
See above.
>> And which platform devices to hide would probably depend on the xen
>> command line,
>> For instance for dom0 we would copy ITS information while for domU it
>> would have to be generated, so scratch would be more for domU.
>> We could have a common code for creating IORT structure but it would
>> be a bit complex code with lot of abstractions and callbacks, so I
>> suggest that keeping code simpler would be better.
>
> I am not sure to understand why you think it would create too much
> abstractions. After all the IORT is just a series of nodes with a
> bunch of informations. This could easily be described in a structure
> that would be used to generate the tables.
>
> In any case, we will need that for DomU. So why not doing it now?
>
ok
>>>>> I would like to hear your opinion on this. I will try to discuss the
>>>>> feasibility of 2) with people at Connect. It would be good if we
>>>>> could
>>>>> decide whether this is the way to go or we should use a solution
>>>>> based
>>>>> on this series.
>>>
>>> Andre, Stefano and I had a chat during connect about how we want to
>>> see IORT support in Xen. In the near future, IORT will be used for
>>> different components accross the hypervisor (ITS, SMMU...) and as a
>>> way to
>>> communicate the topology to the guests.
>> Are we thinking about virtual SMMU here as well?
>
> It is a potential long-term usage.
>
>>>
>>> The IORT for the hardware domain is just a specific case as it is
>>> based pre-existing information. But because of removing nodes (e.g
>>> SMMU nodes and probably the PMU nodes), it is basically a full
>>> re-write.
>>>
>>> So I would consider of full separate the logic of generating the
>>> IORT table from the host IORT table. By that I mean not browsing the
>>> host IORT when generating the host.
>>>
>> by "the host" you mean dom0 IORT ?
>
> yes.
>
Something on the lines of this ....
struct iort_table_struct {
header h;
list_head pci_rc_list;
list_head its_groups;
list_head smmu;
list_head platForm_devices;
} host_iort;
>>> This has two benefits:
>>> 1) The code generation could be re-used for generating the guest
>>> IORT later on.
>> See my comment above
>>> 2) See 2) from Andre
>>> 3) We could decide in a finer grain which devices (e.g platform
>>> device) Dom0 can see.
>> ok,
>>>
>>> So, IHMO, we should take a different approach from this series and
>>> extending the scope of it. Rather than focusing on only IORT for the
>>> hardware, I would be better to see IORT as a whole. I.e how IORT
>>> will interact with the hypervisor?
>>>
>>> For instance, likely you would need to parse the IORT in quite a few
>>> places in Xen. It would be better to get IORT parsed only once and
>>> store the information in Xen like data-structures.
>>>
>> I am thinking of reusing much of ACPI tables for that and introducing
>> less abstractions.
>>> This require more work than this current scope of this series. But I
>>> think it would helful for future work such as PCI passthrough support.
>>>
>>> Any opinions?
>>>
>> for DomUs it is tied to PCI PT, so both design should evolve together.
>
> Why that? The generation of IORT is pretty standard.
DomU IORT will have a virtual PCI_RC and virtual ITS_GROUP and IIUC at
the time of generation of domU IORT (with device passthrough) toolstack
will communicate to xen virtual / physical deviceID.
So Generation logic is standard what to put in IORT and what mappings to
add, is a design consideration which is based on PCI_PT.
I have a question here: if a virtual e1000 device is added to domU
(non-PCI PT case),
should it be added as a DomU platform device or a virtual pci device.
> Cheers,
>
Cheers!
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-10-12 6:11 ` Manish Jaggi
@ 2017-10-12 11:04 ` Julien Grall
2017-10-12 11:22 ` Manish Jaggi
0 siblings, 1 reply; 13+ messages in thread
From: Julien Grall @ 2017-10-12 11:04 UTC (permalink / raw)
To: Manish Jaggi, Julien Grall, Andre Przywara, xen-devel
Cc: tomasz.nowicki, sstabellini, Manish Jaggi
Hello,
On 12/10/17 07:11, Manish Jaggi wrote:
> On 10/6/2017 7:54 PM, Julien Grall wrote:
>> I am not asking to write the DomU support, but at least have a full
>> separation between the Parsing and Generation. So we could easily
>> adapt and re-use the code when we get the DomU support.
>>
> I got your point, but as of today there is no code reuse for most of
> apci_tables. So code result _only_ for IORT but not for acpi is not
> correct approach.
Why? The generation of IORT is fairly standalone.
And again, this was suggestion to share in the future and an expectation
for this series. What I care the most is the generation to be fully
separated from the rest.
>>>
>>> Also this is the part of PCI passthrough flow so that also might
>>> change few things.
>>>
>>> But from pov of dom0 smmu hiding, it is a different flow and is
>>> coupled with PCI PT.
>>>
>>>>
>>>>>>
>>>>>> I think 1) can be solved using this series as a base. I have quite
>>>>>> some
>>>>>> comments ready for the patches, shall we follow this route.
>>>>>>
>>>>>> 2) obviously would change the game completely. We need to sit down
>>>>>> and
>>>>>> design this properly. Probably this means that Xen parses the IORT
>>>>>> and
>>>>>> builds internal representations of the mappings,
>>> Can you please add more detail on the internal representations of the
>>> mappings.
>>
>> What exactly do you want? This is likely going to be decided once you
>> looked what is the expected interaction between IORT and Xen.
> More details on this line "Probably this means that Xen parses the IORT and
> builds internal representations of the mappings,"
I think you have enough meat in this thread to come up with a
proposition based on the feedback. Once you send it, we can have a
discussion and find agreement.
[...]
>>>> The IORT for the hardware domain is just a specific case as it is
>>>> based pre-existing information. But because of removing nodes (e.g
>>>> SMMU nodes and probably the PMU nodes), it is basically a full
>>>> re-write.
>>>>
>>>> So I would consider of full separate the logic of generating the
>>>> IORT table from the host IORT table. By that I mean not browsing the
>>>> host IORT when generating the host.
>>>>
>>> by "the host" you mean dom0 IORT ?
>>
>> yes.
>>
> Something on the lines of this ....
> struct iort_table_struct {
> header h;
> list_head pci_rc_list;
> list_head its_groups;
> list_head smmu;
> list_head platForm_devices;
> } host_iort;
Some context is probably missing.
>>>> This has two benefits:
>>>> 1) The code generation could be re-used for generating the guest
>>>> IORT later on.
>>> See my comment above
>>>> 2) See 2) from Andre
>>>> 3) We could decide in a finer grain which devices (e.g platform
>>>> device) Dom0 can see.
>>> ok,
>>>>
>>>> So, IHMO, we should take a different approach from this series and
>>>> extending the scope of it. Rather than focusing on only IORT for the
>>>> hardware, I would be better to see IORT as a whole. I.e how IORT
>>>> will interact with the hypervisor?
>>>>
>>>> For instance, likely you would need to parse the IORT in quite a few
>>>> places in Xen. It would be better to get IORT parsed only once and
>>>> store the information in Xen like data-structures.
>>>>
>>> I am thinking of reusing much of ACPI tables for that and introducing
>>> less abstractions.
>>>> This require more work than this current scope of this series. But I
>>>> think it would helful for future work such as PCI passthrough support.
>>>>
>>>> Any opinions?
>>>>
>>> for DomUs it is tied to PCI PT, so both design should evolve together.
>>
>> Why that? The generation of IORT is pretty standard.
> DomU IORT will have a virtual PCI_RC and virtual ITS_GROUP and IIUC at
> the time of generation of domU IORT (with device passthrough) toolstack
> will communicate to xen virtual / physical deviceID.
> So Generation logic is standard what to put in IORT and what mappings to
> add, is a design consideration which is based on PCI_PT.
I still don't get why you are trying to argue on the information written
in the IORT. The generation does not care whether those information are
taken from the host IORT, virtual, a mix...
It just takes in input the information and output the IORT. That's it.
>
> I have a question here: if a virtual e1000 device is added to domU
> (non-PCI PT case),
> should it be added as a DomU platform device or a virtual pci device.
Hu? Is that e1000 devices a PCI or platform device? If you answer that,
you answer to your question here.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-10-12 11:04 ` Julien Grall
@ 2017-10-12 11:22 ` Manish Jaggi
2017-10-12 11:44 ` Julien Grall
0 siblings, 1 reply; 13+ messages in thread
From: Manish Jaggi @ 2017-10-12 11:22 UTC (permalink / raw)
To: Julien Grall, Julien Grall, Andre Przywara, xen-devel
Cc: tomasz.nowicki, sstabellini, Manish Jaggi
Hi Julien,
Why do you omit parts of mail where I have asked a question , please
avoid skiping that removes the context.
I raised a valid point and it was totally ignored and you asked me to
explain the rationale on a later point.
So if you choose to ignore my first point, how can I put any point.
This is what I have asked
>>The ACPI tables for DomU are generated by the toolstack today. So I
don't see why we would change that to support IORT.
>>
>>However, you can have a file shared between the toolstack and Xen and
contain the generation of IORT.
>>
>>For instance, this is what we already does with libelf (see
common/libelf).
This will amount to adding a function make_iort in libxl__prepare_acpi,
which would use the common code that is also use generate dom0
IORT (domain_build.c).
Correct ?
If we go by this logic, then libxl_prepare_acpi and domain_build.c
should use a common code for all acpi tables.
- Are you suggesting we change that as well and make it part of common
code.
The code in domain_build.c and in libxl__prepare_acpi is very similar,
see the code below.
static int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
{
....
d->arch.efi_acpi_table = alloc_xenheap_pages(order, 0);
...
rc = acpi_create_fadt(d, tbl_add);
if ( rc != 0 )
return rc;
rc = acpi_create_madt(d, tbl_add);
if ( rc != 0 )
return rc;
rc = acpi_create_stao(d, tbl_add);
if ( rc != 0 )
return rc;
rc = acpi_create_xsdt(d, tbl_add);
if ( rc != 0 )
return rc;
rc = acpi_create_rsdp(d, tbl_add);
if ( rc != 0 )
return rc;
...
}
int libxl__prepare_acpi(libxl__gc *gc, libxl_domain_build_info *info,
struct xc_dom_image *dom)
{
...
rc = libxl__allocate_acpi_tables(gc, info, dom, acpitables);
if (rc)
goto out;
make_acpi_rsdp(gc, dom, acpitables);
make_acpi_xsdt(gc, dom, acpitables);
make_acpi_gtdt(gc, dom, acpitables);
rc = make_acpi_madt(gc, dom, info, acpitables);
if (rc)
goto out;
make_acpi_fadt(gc, dom, acpitables);
make_acpi_dsdt(gc, dom, acpitables);
out:
return rc;
}
Now if you see both the codes are quite similar and there is redundancy
in libxl and in xen code for preparing ACPI tables for dom0 and domU.
The point I am raising is quite clear, if all other tables like MADT,
XSDT, RSDP, GTDT etc does not share a common generation code with xen
what is so special about IORT.
Either we move all generation into a common code or keep redundancy for
IORT.
I hope I have shown the code and made the point quite clear.
Please provide a technical answer rather than a simple "Why".
Cheers!
Manish
On 10/12/2017 4:34 PM, Julien Grall wrote:
> Hello,
>
> On 12/10/17 07:11, Manish Jaggi wrote:
>> On 10/6/2017 7:54 PM, Julien Grall wrote:
>>> I am not asking to write the DomU support, but at least have a full
>>> separation between the Parsing and Generation. So we could easily
>>> adapt and re-use the code when we get the DomU support.
>>>
>> I got your point, but as of today there is no code reuse for most of
>> apci_tables. So code result _only_ for IORT but not for acpi is not
>> correct approach.
>
> Why? The generation of IORT is fairly standalone.
>
> And again, this was suggestion to share in the future and an
> expectation for this series. What I care the most is the generation to
> be fully separated from the rest.
>
>>>>
>>>> Also this is the part of PCI passthrough flow so that also might
>>>> change few things.
>>>>
>>>> But from pov of dom0 smmu hiding, it is a different flow and is
>>>> coupled with PCI PT.
>>>>
>>>>>
>>>>>>>
>>>>>>> I think 1) can be solved using this series as a base. I have
>>>>>>> quite some
>>>>>>> comments ready for the patches, shall we follow this route.
>>>>>>>
>>>>>>> 2) obviously would change the game completely. We need to sit
>>>>>>> down and
>>>>>>> design this properly. Probably this means that Xen parses the
>>>>>>> IORT and
>>>>>>> builds internal representations of the mappings,
>>>> Can you please add more detail on the internal representations of
>>>> the mappings.
>>>
>>> What exactly do you want? This is likely going to be decided once
>>> you looked what is the expected interaction between IORT and Xen.
>> More details on this line "Probably this means that Xen parses the
>> IORT and
>> builds internal representations of the mappings,"
>
> I think you have enough meat in this thread to come up with a
> proposition based on the feedback. Once you send it, we can have a
> discussion and find agreement.
>
> [...]
>
>>>>> The IORT for the hardware domain is just a specific case as it is
>>>>> based pre-existing information. But because of removing nodes (e.g
>>>>> SMMU nodes and probably the PMU nodes), it is basically a full
>>>>> re-write.
>>>>>
>>>>> So I would consider of full separate the logic of generating the
>>>>> IORT table from the host IORT table. By that I mean not browsing
>>>>> the host IORT when generating the host.
>>>>>
>>>> by "the host" you mean dom0 IORT ?
>>>
>>> yes.
>>>
>> Something on the lines of this ....
>> struct iort_table_struct {
>> header h;
>> list_head pci_rc_list;
>> list_head its_groups;
>> list_head smmu;
>> list_head platForm_devices;
>> } host_iort;
>
> Some context is probably missing.
>
>>>>> This has two benefits:
>>>>> 1) The code generation could be re-used for generating the
>>>>> guest IORT later on.
>>>> See my comment above
>>>>> 2) See 2) from Andre
>>>>> 3) We could decide in a finer grain which devices (e.g
>>>>> platform device) Dom0 can see.
>>>> ok,
>>>>>
>>>>> So, IHMO, we should take a different approach from this series and
>>>>> extending the scope of it. Rather than focusing on only IORT for
>>>>> the hardware, I would be better to see IORT as a whole. I.e how
>>>>> IORT will interact with the hypervisor?
>>>>>
>>>>> For instance, likely you would need to parse the IORT in quite a
>>>>> few places in Xen. It would be better to get IORT parsed only once
>>>>> and store the information in Xen like data-structures.
>>>>>
>>>> I am thinking of reusing much of ACPI tables for that and
>>>> introducing less abstractions.
>>>>> This require more work than this current scope of this series. But
>>>>> I think it would helful for future work such as PCI passthrough
>>>>> support.
>>>>>
>>>>> Any opinions?
>>>>>
>>>> for DomUs it is tied to PCI PT, so both design should evolve together.
>>>
>>> Why that? The generation of IORT is pretty standard.
>> DomU IORT will have a virtual PCI_RC and virtual ITS_GROUP and IIUC
>> at the time of generation of domU IORT (with device passthrough)
>> toolstack will communicate to xen virtual / physical deviceID.
>> So Generation logic is standard what to put in IORT and what mappings
>> to add, is a design consideration which is based on PCI_PT.
> I still don't get why you are trying to argue on the information
> written in the IORT. The generation does not care whether those
> information are taken from the host IORT, virtual, a mix...
>
> It just takes in input the information and output the IORT. That's it.
>
>>
>> I have a question here: if a virtual e1000 device is added to domU
>> (non-PCI PT case),
>> should it be added as a DomU platform device or a virtual pci device.
>
> Hu? Is that e1000 devices a PCI or platform device? If you answer
> that, you answer to your question here.
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-10-12 11:22 ` Manish Jaggi
@ 2017-10-12 11:44 ` Julien Grall
2017-10-12 21:08 ` Manish Jaggi
0 siblings, 1 reply; 13+ messages in thread
From: Julien Grall @ 2017-10-12 11:44 UTC (permalink / raw)
To: Manish Jaggi, Julien Grall, Andre Przywara
Cc: xen-devel, sstabellini, Manish Jaggi, tomasz.nowicki
On 12/10/17 12:22, Manish Jaggi wrote:
> Hi Julien,
>
> Why do you omit parts of mail where I have asked a question , please
> avoid skiping that removes the context.
I believe I answered it just after because you asked twice the same
thing. So may I dropped the context but the answer was there...
For your convenience here the replicated answer.
"Why? The generation of IORT is fairly standalone.
And again, this was suggestion to share in the future and an expectation
for this series. What I care the most is the generation to be fully
separated from the rest."
> I raised a valid point and it was totally ignored and you asked me to
> explain the rationale on a later point.
> So if you choose to ignore my first point, how can I put any point.
Well, maybe you should read the e-mail more carefully because your point
have been addressed. If they are not, then please say it rather than
accusing the reviewers on spending not enough time on your series...
[...]
> Now if you see both the codes are quite similar and there is redundancy
> in libxl and in xen code for preparing ACPI tables for dom0 and domU.
> The point I am raising is quite clear, if all other tables like MADT,
> XSDT, RSDP, GTDT etc does not share a common generation code with xen
> what is so special about IORT.
> Either we move all generation into a common code or keep redundancy for
> IORT.
>
> I hope I have shown the code and made the point quite clear.
> Please provide a technical answer rather than a simple "Why".
Why do you still continue arguing on how this is going to interact with
libxl when your only work now (as I stated in every single e-mail) is
for Dom0.
If the generation is generic enough, it will require little code to
interface. After all, you only need:
- informations (e.g DeviceID, MasterID...)
- buffer for writing the generated IORT
So now it is maybe time for you to suggest an interface we can discuss on.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2] ARM: ACPI: IORT: Hide SMMU from hardware domain's IORT table
2017-10-12 11:44 ` Julien Grall
@ 2017-10-12 21:08 ` Manish Jaggi
0 siblings, 0 replies; 13+ messages in thread
From: Manish Jaggi @ 2017-10-12 21:08 UTC (permalink / raw)
To: Julien Grall, Julien Grall, Andre Przywara
Cc: xen-devel, sstabellini, Manish Jaggi, tomasz.nowicki
On 10/12/2017 5:14 PM, Julien Grall wrote:
>
>
> On 12/10/17 12:22, Manish Jaggi wrote:
>> Hi Julien,
>>
>> Why do you omit parts of mail where I have asked a question , please
>> avoid skiping that removes the context.
>
> I believe I answered it just after because you asked twice the same
> thing. So may I dropped the context but the answer was there...
>
> For your convenience here the replicated answer.
>
> "Why? The generation of IORT is fairly standalone.
>
> And again, this was suggestion to share in the future and an
> expectation for this series. What I care the most is the generation to
> be fully separated from the rest."
>
>> I raised a valid point and it was totally ignored and you asked me to
>> explain the rationale on a later point.
>> So if you choose to ignore my first point, how can I put any point.
>
> Well, maybe you should read the e-mail more carefully because your
> point have been addressed. If they are not, then please say it rather
> than accusing the reviewers on spending not enough time on your series...
>
> [...]
>
>> Now if you see both the codes are quite similar and there is
>> redundancy in libxl and in xen code for preparing ACPI tables for
>> dom0 and domU.
>> The point I am raising is quite clear, if all other tables like MADT,
>> XSDT, RSDP, GTDT etc does not share a common generation code with xen
>> what is so special about IORT.
>> Either we move all generation into a common code or keep redundancy
>> for IORT.
>>
>> I hope I have shown the code and made the point quite clear.
>> Please provide a technical answer rather than a simple "Why".
>
> Why do you still continue arguing on how this is going to interact
> with libxl when your only work now (as I stated in every single
> e-mail) is for Dom0.
>
> If the generation is generic enough, it will require little code to
> interface. After all, you only need:
> - informations (e.g DeviceID, MasterID...)
> - buffer for writing the generated IORT
>
> So now it is maybe time for you to suggest an interface we can discuss
> on.
Sure. A quick draft is shared on mailing list. [1]
[1] https://marc.info/?l=xen-devel&m=150784236208192&w=2
>
> Cheers,
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread