From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38754) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bnTUT-00032H-VS for qemu-devel@nongnu.org; Fri, 23 Sep 2016 12:38:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bnTUO-0001zj-Hb for qemu-devel@nongnu.org; Fri, 23 Sep 2016 12:38:21 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53830) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bnTUO-0001zO-7u for qemu-devel@nongnu.org; Fri, 23 Sep 2016 12:38:16 -0400 References: <20160822161740.4252-1-prem.mallappa@broadcom.com> <20160822161740.4252-10-prem.mallappa@broadcom.com> <664e505c-70f9-99b4-3b8d-8e52e1dea4eb@redhat.com> From: Auger Eric Message-ID: <9cee49f8-a419-221b-5de3-138c680aeeb4@redhat.com> Date: Fri, 23 Sep 2016 18:38:11 +0200 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 9/9] [optional] arm: smmu-v3: ACPI IORT initial support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Prem Mallappa Cc: Peter Maydell , QEMU Developers , "Edgar E . Iglesias" Hi Prem, On 23/09/2016 16:07, Prem Mallappa wrote: > On Fri, Sep 23, 2016 at 6:40 PM, Auger Eric > wrote: > > Hi Prem, > > On 12/09/2016 22:42, Prem Mallappa wrote: > > On Fri, Sep 9, 2016 at 8:54 PM, Auger Eric > wrote: > > > >> Hi Prem, > >> > >> On 22/08/2016 18:17, Prem Mallappa wrote: > >>> Added ACPI IORT tables, was needed for internal project purpose, but > >>> posting here for anyone looking for testing ACPI on ARM platforms. > >>> (P.S: Linux side IORT patches are WIP) > >> I am also interested in IORT ITS group and currently prototyping > >> something, hence my few comments/questions. > >>> > >>> Signed-off-by: Prem Mallappa > > >>> --- > >>> hw/arm/virt-acpi-build.c | 43 +++++++++++++++++++++++ > >>> include/hw/acpi/acpi-defs.h | 84 ++++++++++++++++++++++++++++++ > >> +++++++++++++++ > >>> 2 files changed, 127 insertions(+) > >>> > >>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > >>> index 1fa0581..d5fb69e 100644 > >>> --- a/hw/arm/virt-acpi-build.c > >>> +++ b/hw/arm/virt-acpi-build.c > >>> @@ -382,6 +382,45 @@ build_rsdp(GArray *rsdp_table, BIOSLinker > *linker, > >> unsigned rsdt_tbl_offset) > >>> return rsdp_table; > >>> } > >>> > >>> +/* > >>> + * TODO: Simple IORT for now, will add ID mappings as we go > >>> + * basic idea is to instantiate SMMU from ACPI > >>> + */ > >>> +static void > >>> +build_iort(GArray *table_data, BIOSLinker *linker, VirtGuestInfo > >> *guest_info) > >>> +{ > >>> + int iort_start = table_data->len; > >>> + AcpiIortTable *iort; > >>> + AcpiIortNode *iort_node; > >>> + AcpiIortSmmu3 *smmu; > >>> + AcpiIortRC *rc; > >>> + const MemMapEntry *memmap = guest_info->memmap; > >>> + > >>> + iort = acpi_data_push(table_data, sizeof(*iort)); > >>> + > >>> + iort->length = sizeof(*iort); > >> Isn't is supposed to be the length of the whole IORT (including > the node > >> cumulated sizes?) > >>> + iort->node_offset = table_data->len - iort_start; > >>> + iort->num_nodes++; > >>> + > >>> + smmu = acpi_data_push(table_data, sizeof(*smmu)); > >>> + iort_node = &smmu->iort_node; > >>> + iort_node->type = 0x04; /* SMMUv3 */ > >> To match existing code (include/hw/acpi/acpi-defs.h), maybe enum > values > >> can be created (ACPI_IORT_NODE_SMMU_V3). This also matches kernel > enum. > >> > >> I have made these changes, will send out ASAP. > > > > > >> More generally Shannon advised to use the same field names as the > ones > >> used in the kernel header: acpi_iort_node_type in > include/acpi/actbl2.h > >> > > > > Will change this accordingly > > > > > >>> + iort_node->length = sizeof(*smmu); > >>> + smmu->base_addr = cpu_to_le64(memmap[VIRT_SMMU].base); > >>> + > >>> + iort->num_nodes++; > >>> + > >>> + rc = acpi_data_push(table_data, sizeof(*rc)); > >>> + iort_node = &rc->iort_node; > >>> + iort_node->type = 0x02; /* RC */ > >>> + iort_node->length = sizeof(*rc); > >> I think the mem_access_prop field should be set to 1 now the host > >> controller is assumed to be cache coherent. > >>> + rc->ats_attr = 1; > >> no ATS support instead? > >>> + rc->pci_seg_num = 0; > >> ID mappings are mandated for me to support MSIs with ITS. > >> > > > > These changes are made as I write, > > > > > >> Shannon told me we should match the kernel datatypes & fields > >> > >> for instance in include/acpi/actbl2.h we have: > >> > >> struct acpi_iort_id_mapping { > >> u32 input_base; /* Lowest value in input range */ > >> u32 id_count; /* Number of IDs */ > >> u32 output_base; /* Lowest value in output range */ > >> u32 output_reference; /* A reference to the output node */ > >> u32 flags; > >> }; > >> > >> This also holds for other struct definitions. > >> > >> > > Sure will change this accordingly. > > I currently have a series creating the IORT with an RC node and an ITS > node. It is needed to complete the integration of the virtual ITS (to > connect it with the PCI host controller). This originates from this > patch: I added the RC->ITS ID mapping + the ITS node and tested it. > > I don't know how to proceed to get the 2 features (vSMMU and vITS) > progress separately. Do you plan to respin this patch quickly? > Otherwise, if you are busy with other things I propose you to respin > fixing the few things above, splitting it into 3 patches, header [1], > ITS node creation [2], RC node creation with RC->ITS mapping [3] while > keeping credit to you on [1] and [3]. > > Then we can have a 4th patch adding RC-> SMMU ID > ITS mapping? > > Please let me know what are your plans and what do you think. OK thanks. Looking forward to reviewing your patch Best Regards Eric > > Thanks > > Eric > > > > > > > Hi Eric, > I have been busy with something else, however I have a wokring patch set > (unclean version) > which creates the IORT tables, with SMMU->RC->ITS with ITS id mapping > (routing all interrupts to single ITS). > I'll push them by to unstable branch by this Sunday. > > -- > Cheers, > /Prem