| Message ID | 20210812165341.40784-8-shashi.mallela@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | GICv3 LPI and ITS feature implementation | expand |
On Thu, 12 Aug 2021 at 17:53, Shashi Mallela <shashi.mallela@linaro.org> wrote: > > Included creation of ITS as part of SBSA platform GIC > initialization. > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > --- > hw/arm/sbsa-ref.c | 79 ++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 75 insertions(+), 4 deletions(-) > > +static char *sbsa_get_version(Object *obj, Error **errp) > +{ > + SBSAMachineState *sms = SBSA_MACHINE(obj); > + > + switch (sms->version) { > + case SBSA_DEFAULT: > + return g_strdup("default"); > + case SBSA_ITS: > + return g_strdup("sbsaits"); > + default: > + g_assert_not_reached(); > + } > +} > + > +static void sbsa_set_version(Object *obj, const char *value, Error **errp) > +{ > + SBSAMachineState *sms = SBSA_MACHINE(obj); > + > + if (!strcmp(value, "sbsaits")) { > + sms->version = SBSA_ITS; > + } else if (!strcmp(value, "default")) { > + sms->version = SBSA_DEFAULT; > + } else { > + error_setg(errp, "Invalid version value"); > + error_append_hint(errp, "Valid values are default, sbsaits.\n"); > + } > +} > > static void sbsa_ref_instance_init(Object *obj) > { > SBSAMachineState *sms = SBSA_MACHINE(obj); > > + sms->version = SBSA_ITS; > sbsa_flash_create(sms); > } > > @@ -850,6 +915,12 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data) > mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids; > mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props; > mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id; > + > + object_class_property_add_str(oc, "version", sbsa_get_version, > + sbsa_set_version); > + object_class_property_set_description(oc, "version", > + "Set the Version type. " > + "Valid values are default & sbsaits"); This doesn't look right; where has it come from ? If you want a command line switch to let the user say whether the ITS should be present or not, you should use the same thing the virt board does, which is a bool property "its". If you want the sbsa-ref board to become a proper "versioned machine type" such that users can say "-M sbsa-ref-6.1" and get the SBSA board exactly as QEMU 6.1 supplied it, that looks completely different (and is a heavy back-compatibility burden, so needs discussion about whether now is the right time to do it), and probably is better not tangled up with this patchseries. thanks -- PMM
On Thu, 2021-08-19 at 14:27 +0100, Peter Maydell wrote: > On Thu, 12 Aug 2021 at 17:53, Shashi Mallela < > shashi.mallela@linaro.org> wrote: > > Included creation of ITS as part of SBSA platform GIC > > initialization. > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > --- > > hw/arm/sbsa-ref.c | 79 > > ++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 75 insertions(+), 4 deletions(-) > > > > +static char *sbsa_get_version(Object *obj, Error **errp) > > +{ > > + SBSAMachineState *sms = SBSA_MACHINE(obj); > > + > > + switch (sms->version) { > > + case SBSA_DEFAULT: > > + return g_strdup("default"); > > + case SBSA_ITS: > > + return g_strdup("sbsaits"); > > + default: > > + g_assert_not_reached(); > > + } > > +} > > + > > +static void sbsa_set_version(Object *obj, const char *value, Error > > **errp) > > +{ > > + SBSAMachineState *sms = SBSA_MACHINE(obj); > > + > > + if (!strcmp(value, "sbsaits")) { > > + sms->version = SBSA_ITS; > > + } else if (!strcmp(value, "default")) { > > + sms->version = SBSA_DEFAULT; > > + } else { > > + error_setg(errp, "Invalid version value"); > > + error_append_hint(errp, "Valid values are default, > > sbsaits.\n"); > > + } > > +} > > > > static void sbsa_ref_instance_init(Object *obj) > > { > > SBSAMachineState *sms = SBSA_MACHINE(obj); > > > > + sms->version = SBSA_ITS; > > sbsa_flash_create(sms); > > } > > > > @@ -850,6 +915,12 @@ static void sbsa_ref_class_init(ObjectClass > > *oc, void *data) > > mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids; > > mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props; > > mc->get_default_cpu_node_id = > > sbsa_ref_get_default_cpu_node_id; > > + > > + object_class_property_add_str(oc, "version", sbsa_get_version, > > + sbsa_set_version); > > + object_class_property_set_description(oc, "version", > > + "Set the Version type. " > > + "Valid values are > > default & sbsaits"); > > This doesn't look right; where has it come from ? > > If you want a command line switch to let the user say whether the > ITS should be present or not, you should use the same thing the > virt board does, which is a bool property "its". > > If you want the sbsa-ref board to become a proper "versioned machine > type" such that users can say "-M sbsa-ref-6.1" and get the SBSA > board exactly as QEMU 6.1 supplied it, that looks completely > different > (and is a heavy back-compatibility burden, so needs discussion about > whether now is the right time to do it), and probably is better not > tangled up with this patchseries. > > thanks > -- PMM Since the memory map for sbsa-ref has been updated with ITS address range added between distributor and redistributor regions(as per last reveiw comments),this has resulted in a change in the redistributor base address(as compared to previous sbsa-ref with no ITS support).Hence there was a subsequent request for creating a versioning logic to differentiate between ITS presence or absence which would be of use to other modules (like TF-A) to pick the relevant redistributor base address based on this versioning.
Hi Peter, On Thu, Aug 19, 2021 at 14:27:19 +0100, Peter Maydell wrote: > On Thu, 12 Aug 2021 at 17:53, Shashi Mallela <shashi.mallela@linaro.org> wrote: > > > > Included creation of ITS as part of SBSA platform GIC > > initialization. > > > > Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> > > --- > > hw/arm/sbsa-ref.c | 79 ++++++++++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 75 insertions(+), 4 deletions(-) > > > > > +static char *sbsa_get_version(Object *obj, Error **errp) > > +{ > > + SBSAMachineState *sms = SBSA_MACHINE(obj); > > + > > + switch (sms->version) { > > + case SBSA_DEFAULT: > > + return g_strdup("default"); > > + case SBSA_ITS: > > + return g_strdup("sbsaits"); > > + default: > > + g_assert_not_reached(); > > + } > > +} > > + > > +static void sbsa_set_version(Object *obj, const char *value, Error **errp) > > +{ > > + SBSAMachineState *sms = SBSA_MACHINE(obj); > > + > > + if (!strcmp(value, "sbsaits")) { > > + sms->version = SBSA_ITS; > > + } else if (!strcmp(value, "default")) { > > + sms->version = SBSA_DEFAULT; > > + } else { > > + error_setg(errp, "Invalid version value"); > > + error_append_hint(errp, "Valid values are default, sbsaits.\n"); > > + } > > +} > > > > static void sbsa_ref_instance_init(Object *obj) > > { > > SBSAMachineState *sms = SBSA_MACHINE(obj); > > > > + sms->version = SBSA_ITS; > > sbsa_flash_create(sms); > > } > > > > @@ -850,6 +915,12 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data) > > mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids; > > mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props; > > mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id; > > + > > + object_class_property_add_str(oc, "version", sbsa_get_version, > > + sbsa_set_version); > > + object_class_property_set_description(oc, "version", > > + "Set the Version type. " > > + "Valid values are default & sbsaits"); > > This doesn't look right; where has it come from ? > > If you want a command line switch to let the user say whether the > ITS should be present or not, you should use the same thing the > virt board does, which is a bool property "its". > > If you want the sbsa-ref board to become a proper "versioned machine > type" such that users can say "-M sbsa-ref-6.1" and get the SBSA > board exactly as QEMU 6.1 supplied it, that looks completely different > (and is a heavy back-compatibility burden, so needs discussion about > whether now is the right time to do it), and probably is better not > tangled up with this patchseries. Hmm. I mean, you're absolutely right about the complexity and need for discussion. My concern is that we cannot insert the ITS block in the memory map where it would be in an ARM GIC implementation without changing the memory map (pushing the redistributor further down). It is also true that simply versioning sbsa-ref does not mean we end up with a properly aligned with ARM IP register frame layout. Some additional logic is required for that. If we assume that we don't want to further complicate this set by adding the additional logic *now*, I see three options: - Implement memory map versioning for sbsa-ref for this set, placing the ITS (if enabled) directly after the DIST for sbsa-ref-6.2. - In this set, place the ITS frames in a different location relative to the REDIST frames than it will end up once the further logic is implemented. - Drop the sbsa-ref ITS support from this set, and bring it in with the set implementing the additional logic. Typing that, I'm getting the feeling that if I was the maintainer, the third option would be my preference... / Leif
On Thu, 2 Sept 2021 at 13:43, Leif Lindholm <leif@nuviainc.com> wrote: > On Thu, Aug 19, 2021 at 14:27:19 +0100, Peter Maydell wrote: > > If you want a command line switch to let the user say whether the > > ITS should be present or not, you should use the same thing the > > virt board does, which is a bool property "its". > > > > If you want the sbsa-ref board to become a proper "versioned machine > > type" such that users can say "-M sbsa-ref-6.1" and get the SBSA > > board exactly as QEMU 6.1 supplied it, that looks completely different > > (and is a heavy back-compatibility burden, so needs discussion about > > whether now is the right time to do it), and probably is better not > > tangled up with this patchseries. > > Hmm. I mean, you're absolutely right about the complexity and need for > discussion. My concern is that we cannot insert the ITS block in the > memory map where it would be in an ARM GIC implementation without > changing the memory map (pushing the redistributor further down). > > It is also true that simply versioning sbsa-ref does not mean we end > up with a properly aligned with ARM IP register frame layout. Some > additional logic is required for that. > > If we assume that we don't want to further complicate this set by > adding the additional logic *now*, I see three options: > - Implement memory map versioning for sbsa-ref for this set, placing > the ITS (if enabled) directly after the DIST for sbsa-ref-6.2. > - In this set, place the ITS frames in a different location relative > to the REDIST frames than it will end up once the further logic is > implemented. > - Drop the sbsa-ref ITS support from this set, and bring it in with > the set implementing the additional logic. I don't think implementing versioned machine types helps you much anyway, because your users are all going to be currently using -M sbsa-ref, so they will by default see the change in device layout. I do think that we should get the "with an ITS" device layout right from the start, so that we're only dealing with 2 possibilities (what we have today, and the final intended layout), not 3 (today, final layout, some intermediate thing). How does guest software on this board figure out the memory layout ? Is there a mechanism for telling it "this is version 2 of this board" ? -- PMM
On Thu, Sep 02, 2021 at 13:51:26 +0100, Peter Maydell wrote: > On Thu, 2 Sept 2021 at 13:43, Leif Lindholm <leif@nuviainc.com> wrote: > > On Thu, Aug 19, 2021 at 14:27:19 +0100, Peter Maydell wrote: > > > If you want a command line switch to let the user say whether the > > > ITS should be present or not, you should use the same thing the > > > virt board does, which is a bool property "its". > > > > > > If you want the sbsa-ref board to become a proper "versioned machine > > > type" such that users can say "-M sbsa-ref-6.1" and get the SBSA > > > board exactly as QEMU 6.1 supplied it, that looks completely different > > > (and is a heavy back-compatibility burden, so needs discussion about > > > whether now is the right time to do it), and probably is better not > > > tangled up with this patchseries. > > > > Hmm. I mean, you're absolutely right about the complexity and need for > > discussion. My concern is that we cannot insert the ITS block in the > > memory map where it would be in an ARM GIC implementation without > > changing the memory map (pushing the redistributor further down). > > > > It is also true that simply versioning sbsa-ref does not mean we end > > up with a properly aligned with ARM IP register frame layout. Some > > additional logic is required for that. > > > > If we assume that we don't want to further complicate this set by > > adding the additional logic *now*, I see three options: > > - Implement memory map versioning for sbsa-ref for this set, placing > > the ITS (if enabled) directly after the DIST for sbsa-ref-6.2. > > - In this set, place the ITS frames in a different location relative > > to the REDIST frames than it will end up once the further logic is > > implemented. > > - Drop the sbsa-ref ITS support from this set, and bring it in with > > the set implementing the additional logic. > > I don't think implementing versioned machine types helps you much > anyway, because your users are all going to be currently using > -M sbsa-ref, so they will by default see the change in device layout. > > I do think that we should get the "with an ITS" device layout right > from the start, so that we're only dealing with 2 possibilities > (what we have today, and the final intended layout), not 3 (today, > final layout, some intermediate thing). > > How does guest software on this board figure out the memory > layout ? Is there a mechanism for telling it "this is version 2 > of this board" ? Not currently. Aiming to look like most SBSA platforms, it is hard-wired, and firmware passes that information to the os. This is the kind of thing I eventually want to do using a PV-model SCP. As a stop-gap, we could push it through the DT, like we do for cpus and memory? / Leif
On Fri, 3 Sept 2021 at 13:01, Leif Lindholm <leif@nuviainc.com> wrote: > On Thu, Sep 02, 2021 at 13:51:26 +0100, Peter Maydell wrote: > > How does guest software on this board figure out the memory > > layout ? Is there a mechanism for telling it "this is version 2 > > of this board" ? > > Not currently. Aiming to look like most SBSA platforms, it is > hard-wired, and firmware passes that information to the os. > > This is the kind of thing I eventually want to do using a PV-model > SCP. As a stop-gap, we could push it through the DT, like we do for > cpus and memory? That's up to you, I guess. You could alternatively have some sort of "board ID" register that the firmware reads ? -- PMM
On Thu, 2 Sept 2021 at 13:43, Leif Lindholm <leif@nuviainc.com> wrote: > Hmm. I mean, you're absolutely right about the complexity and need for > discussion. My concern is that we cannot insert the ITS block in the > memory map where it would be in an ARM GIC implementation without > changing the memory map (pushing the redistributor further down). > > It is also true that simply versioning sbsa-ref does not mean we end > up with a properly aligned with ARM IP register frame layout. Some > additional logic is required for that. > > If we assume that we don't want to further complicate this set by > adding the additional logic *now*, I see three options: > - Implement memory map versioning for sbsa-ref for this set, placing > the ITS (if enabled) directly after the DIST for sbsa-ref-6.2. > - In this set, place the ITS frames in a different location relative > to the REDIST frames than it will end up once the further logic is > implemented. > - Drop the sbsa-ref ITS support from this set, and bring it in with > the set implementing the additional logic. > > Typing that, I'm getting the feeling that if I was the maintainer, > the third option would be my preference... So, we took that third option just to get the initial ITS support into QEMU, and it has now landed. Where do we want to go with the sbsa-ref support ? There doesn't seem like there's much coding required on the QEMU side, it's probably about an afternoon at most to update this patch to match whatever we decide we need to do. But it's very unclear to me what it is we should be doing. Leif, what's your plan/preferences here ? Presumably somebody also needs to do the system-software side of things to handle the ITS being present and the redistributor frames moving... thanks -- PMM
Hi Peter, (Apologies for delay. Alex also tells me you are currently away, but there is no strong urgency here.) On Thu, Sep 23, 2021 at 17:00:35 +0100, Peter Maydell wrote: > > If we assume that we don't want to further complicate this set by > > adding the additional logic *now*, I see three options: > > - Implement memory map versioning for sbsa-ref for this set, placing > > the ITS (if enabled) directly after the DIST for sbsa-ref-6.2. > > - In this set, place the ITS frames in a different location relative > > to the REDIST frames than it will end up once the further logic is > > implemented. > > - Drop the sbsa-ref ITS support from this set, and bring it in with > > the set implementing the additional logic. > > > > Typing that, I'm getting the feeling that if I was the maintainer, > > the third option would be my preference... > > So, we took that third option just to get the initial ITS support > into QEMU, and it has now landed. Where do we want to go with > the sbsa-ref support ? > > There doesn't seem like there's much coding required on the QEMU > side, it's probably about an afternoon at most to update this patch > to match whatever we decide we need to do. But it's very unclear > to me what it is we should be doing. > > Leif, what's your plan/preferences here ? I discussed this with Alex/Shashi. One further complicating aspect is that the EDK2 GIC driver currently relies on GIC addresses being known at compile-time. > Presumably somebody also needs to do the system-software side > of things to handle the ITS being present and the redistributor > frames moving... Since it *would* be useful to get this support in, I think the most pragmatic plan would be: - Add ITS in the location originally proposed by Shashi. - Add information to DT: - Platform version (1). - GICD, GICR, and ITS base addresses. - edk2: Convert GIC driver to support dynamic block addresses. - TF-A: Parse the DT and add SIP SVC calls: - to retrieve it (or return not supported if not found). - to retrieve base addresses for GICD, GICR, and ITS. - edk2-platforms: Query TF-A for platform version. If platform version >= 1, request base addresses for GICD, GICR, and ITS. - Generate IORT if ITS present. - Update GIC frame layout to match an ARM GIC-?00. (Platform version 2) Unrelated to the ITS question, and not requiring any intervention on the QEMU side, we can then also transition the CPU and DRAM discovery to SIP SVC calls, and stop sharing the DT with edk2 completely. And some way further down the line we could do the SCP thing, which would let us support different GIC-layouts/configurations based on platform command line options. (Platform version 3.) (TF-A makes SCP calls if version >= 3) This would then require no changes to edk2-platforms. (Numeric values described as incrementing integer rather than trying to guess at specific qemu release numbers.) This minimises any compatibility breakages, and I think remaining ones are acceptable for this type of platform. / Leif
On Fri, 15 Oct 2021 at 13:23, Leif Lindholm <leif@nuviainc.com> wrote: > (Apologies for delay. Alex also tells me you are currently away, but > there is no strong urgency here.) (Thanks for the ping via Alex -- I missed this email when I was scanning through my qemu-devel mail backlog after my holiday...) > On Thu, Sep 23, 2021 at 17:00:35 +0100, Peter Maydell wrote: > > Leif, what's your plan/preferences here ? > > I discussed this with Alex/Shashi. > > One further complicating aspect is that the EDK2 GIC driver currently > relies on GIC addresses being known at compile-time. > > > Presumably somebody also needs to do the system-software side > > of things to handle the ITS being present and the redistributor > > frames moving... > > Since it *would* be useful to get this support in, I think the most > pragmatic plan would be: > - Add ITS in the location originally proposed by Shashi. > - Add information to DT: > - Platform version (1). > - GICD, GICR, and ITS base addresses. > - edk2: Convert GIC driver to support dynamic block addresses. > - TF-A: Parse the DT and add SIP SVC calls: > - to retrieve it (or return not supported if not found). > - to retrieve base addresses for GICD, GICR, and ITS. > - edk2-platforms: Query TF-A for platform version. > If platform version >= 1, request base addresses for GICD, GICR, and > ITS. > - Generate IORT if ITS present. > - Update GIC frame layout to match an ARM GIC-?00. (Platform version 2) > > Unrelated to the ITS question, and not requiring any intervention on > the QEMU side, we can then also transition the CPU and DRAM discovery > to SIP SVC calls, and stop sharing the DT with edk2 completely. > > And some way further down the line we could do the SCP thing, which > would let us support different GIC-layouts/configurations based on > platform command line options. (Platform version 3.) > (TF-A makes SCP calls if version >= 3) > This would then require no changes to edk2-platforms. This sounds good to me. > (Numeric values described as incrementing integer rather than trying > to guess at specific qemu release numbers.) This is kind of mixing up two separate things. The above describes three "versions" of this machine type, which you might consider as like revision A/B/C of hardware (and where firmware might for instance read a 'board revision' register or something to tell them apart). QEMU release numbers and versioned board types like virt-6.0 are a very specific thing that is taking on a guarantee about maintaining version compatibility of the same board type between different QEMU versions. We can make sbsa-ref a versioned machine type in that sense if you really want to do it, but it makes future changes to the machine rather more painful (everything new immediately needs flags and properties and so on so that it can be added only for newer versions of the machine type and not for the old one -- at a rough count at least 10% of hw/arm/virt.c is purely boilerplate and machinery for versioned machine types). So it's not something we should do for sbsa-ref unless we have a good reason I think. -- PMM
On Tue, Nov 09, 2021 at 13:43:50 +0000, Peter Maydell wrote: > On Fri, 15 Oct 2021 at 13:23, Leif Lindholm <leif@nuviainc.com> wrote: > > (Apologies for delay. Alex also tells me you are currently away, but > > there is no strong urgency here.) > > (Thanks for the ping via Alex -- I missed this email when I was > scanning through my qemu-devel mail backlog after my holiday...) > > > On Thu, Sep 23, 2021 at 17:00:35 +0100, Peter Maydell wrote: > > > Leif, what's your plan/preferences here ? > > > > I discussed this with Alex/Shashi. > > > > One further complicating aspect is that the EDK2 GIC driver currently > > relies on GIC addresses being known at compile-time. > > > > > Presumably somebody also needs to do the system-software side > > > of things to handle the ITS being present and the redistributor > > > frames moving... > > > > Since it *would* be useful to get this support in, I think the most > > pragmatic plan would be: > > - Add ITS in the location originally proposed by Shashi. > > - Add information to DT: > > - Platform version (1). > > - GICD, GICR, and ITS base addresses. > > - edk2: Convert GIC driver to support dynamic block addresses. > > - TF-A: Parse the DT and add SIP SVC calls: > > - to retrieve it (or return not supported if not found). > > - to retrieve base addresses for GICD, GICR, and ITS. > > - edk2-platforms: Query TF-A for platform version. > > If platform version >= 1, request base addresses for GICD, GICR, and > > ITS. > > - Generate IORT if ITS present. > > - Update GIC frame layout to match an ARM GIC-?00. (Platform version 2) > > > > Unrelated to the ITS question, and not requiring any intervention on > > the QEMU side, we can then also transition the CPU and DRAM discovery > > to SIP SVC calls, and stop sharing the DT with edk2 completely. > > > > And some way further down the line we could do the SCP thing, which > > would let us support different GIC-layouts/configurations based on > > platform command line options. (Platform version 3.) > > (TF-A makes SCP calls if version >= 3) > > This would then require no changes to edk2-platforms. > > This sounds good to me. > > > (Numeric values described as incrementing integer rather than trying > > to guess at specific qemu release numbers.) > > This is kind of mixing up two separate things. The above describes > three "versions" of this machine type, which you might consider > as like revision A/B/C of hardware (and where firmware might for > instance read a 'board revision' register or something to tell > them apart). QEMU release numbers and versioned board types like virt-6.0 > are a very specific thing that is taking on a guarantee about > maintaining version compatibility of the same board type between > different QEMU versions. We can make sbsa-ref a versioned machine > type in that sense if you really want to do it, but it makes future > changes to the machine rather more painful (everything new > immediately needs flags and properties and so on so that it can be > added only for newer versions of the machine type and not for the > old one -- at a rough count at least 10% of hw/arm/virt.c is purely > boilerplate and machinery for versioned machine types). > So it's not something we should do for sbsa-ref unless we have a good > reason I think. Hmm, right. So you're thinking containing the versioning fully in the interfaces presented by the model: - Is the version node present? - If so, is it greater than X? - If so, is it great enough to support the SCP interface? And let the firmware deal with that? I was kind of thinking it was expected for incompatible machine versions to be qemu versioned. But I'm good with skipping that bit if it's not. / Leif
On Tue, 9 Nov 2021 at 20:42, Leif Lindholm <leif@nuviainc.com> wrote: > > On Tue, Nov 09, 2021 at 13:43:50 +0000, Peter Maydell wrote: > > On Fri, 15 Oct 2021 at 13:23, Leif Lindholm <leif@nuviainc.com> wrote: > > > (Apologies for delay. Alex also tells me you are currently away, but > > > there is no strong urgency here.) > > > > (Thanks for the ping via Alex -- I missed this email when I was > > scanning through my qemu-devel mail backlog after my holiday...) > > > > > On Thu, Sep 23, 2021 at 17:00:35 +0100, Peter Maydell wrote: > > > (Numeric values described as incrementing integer rather than trying > > > to guess at specific qemu release numbers.) > > > > This is kind of mixing up two separate things. The above describes > > three "versions" of this machine type, which you might consider > > as like revision A/B/C of hardware (and where firmware might for > > instance read a 'board revision' register or something to tell > > them apart). QEMU release numbers and versioned board types like virt-6.0 > > are a very specific thing that is taking on a guarantee about > > maintaining version compatibility of the same board type between > > different QEMU versions. We can make sbsa-ref a versioned machine > > type in that sense if you really want to do it, but it makes future > > changes to the machine rather more painful (everything new > > immediately needs flags and properties and so on so that it can be > > added only for newer versions of the machine type and not for the > > old one -- at a rough count at least 10% of hw/arm/virt.c is purely > > boilerplate and machinery for versioned machine types). > > So it's not something we should do for sbsa-ref unless we have a good > > reason I think. > > Hmm, right. So you're thinking containing the versioning fully in the > interfaces presented by the model: > - Is the version node present? > - If so, is it greater than X? > - If so, is it great enough to support the SCP interface? > And let the firmware deal with that? How the model tells the firmware about the presence/absence of certain things and whether it's one version or another is a different question again :-) I guess since we're using DTB already for passing some info to the firmware that that would be the way to continue. Whether it's better to have a simple "version" node or property, or to have perhaps distinct things in the DTB to indicate presence/absence of important features I don't know and leave up to you. > I was kind of thinking it was expected for incompatible machine > versions to be qemu versioned. But I'm good with skipping that bit if > it's not. The other thing we should nail down is how the user is going to select which flavour of machine they want to provide. Three options: (1) no control, QEMU just emulates whatever the newest flavour is. User needs to go find a firmware image new enough to cope. (2) different flavours exposed as different machine types (analogous to how we have musca-a and musca-b1, or raspi3ap and raspi3b, for instance). Old user command lines keep working because -M sbsa-ref doesn't change; the new stuff would be available via -M sbsa-ref-2 or whatever. (3) different flavours exposed via a property (so you would have -M sbsa-ref,machine-revision=2 or something). If the revision defaults to 1 then old user setups still work but everybody starts to have to cart around an extra command line argument. If it defaults to "newest we know about" you get the opposite set of tradeoffs. -- PMM
On Tue, Nov 09, 2021 at 21:21:46 +0000, Peter Maydell wrote: > > Hmm, right. So you're thinking containing the versioning fully in the > > interfaces presented by the model: > > - Is the version node present? > > - If so, is it greater than X? > > - If so, is it great enough to support the SCP interface? > > And let the firmware deal with that? > > How the model tells the firmware about the presence/absence of > certain things and whether it's one version or another is > a different question again :-) I guess since we're using DTB > already for passing some info to the firmware that that would be > the way to continue. Whether it's better to have a simple > "version" node or property, or to have perhaps distinct things > in the DTB to indicate presence/absence of important features I > don't know and leave up to you. Right. So my preference is to communicate the version only, and then have that version let firmware know whether there are now other interfaces available (i.e. an SCP) to gather additional system information. > > I was kind of thinking it was expected for incompatible machine > > versions to be qemu versioned. But I'm good with skipping that bit if > > it's not. > > The other thing we should nail down is how the user is going to > select which flavour of machine they want to provide. Three > options: > (1) no control, QEMU just emulates whatever the newest flavour is. > User needs to go find a firmware image new enough to cope. > (2) different flavours exposed as different machine types > (analogous to how we have musca-a and musca-b1, or raspi3ap and > raspi3b, for instance). Old user command lines keep working > because -M sbsa-ref doesn't change; the new stuff would be > available via -M sbsa-ref-2 or whatever. > (3) different flavours exposed via a property > (so you would have -M sbsa-ref,machine-revision=2 or something). > If the revision defaults to 1 then old user setups still work > but everybody starts to have to cart around an extra command > line argument. If it defaults to "newest we know about" you > get the opposite set of tradeoffs. I'm leaning towards (1), at least while working towards a "complete" platform (when we may still add/change features, but not how those features are communicated to the firmware). Once the platform is complete, I would very much want to support (3), for example to tweak GIC layout to match GIC-600 or GIC-700, with different configurations. / Leif
On Tue, 9 Nov 2021 at 22:52, Leif Lindholm <leif@nuviainc.com> wrote: > > On Tue, Nov 09, 2021 at 21:21:46 +0000, Peter Maydell wrote: > > The other thing we should nail down is how the user is going to > > select which flavour of machine they want to provide. Three > > options: > > (1) no control, QEMU just emulates whatever the newest flavour is. > > User needs to go find a firmware image new enough to cope. > > (2) different flavours exposed as different machine types > > (analogous to how we have musca-a and musca-b1, or raspi3ap and > > raspi3b, for instance). Old user command lines keep working > > because -M sbsa-ref doesn't change; the new stuff would be > > available via -M sbsa-ref-2 or whatever. > > (3) different flavours exposed via a property > > (so you would have -M sbsa-ref,machine-revision=2 or something). > > If the revision defaults to 1 then old user setups still work > > but everybody starts to have to cart around an extra command > > line argument. If it defaults to "newest we know about" you > > get the opposite set of tradeoffs. > > I'm leaning towards (1), at least while working towards a "complete" > platform (when we may still add/change features, but not how those > features are communicated to the firmware). That's certainly the easiest on the QEMU side; you know the userbase so would know whether that kind of compat break is going to be OK with them. Q1: who is going to write the code for this? Q2: do we want to try to land "ITS in sbsa-ref" in 6.2? Given we're in freeze we're quite short of time even if we handwave the fact it's a new feature, not a bugfix, so I would lean towards 'no'... -- PMM
On Thu, Nov 11, 2021 at 16:55:09 +0000, Peter Maydell wrote: > On Tue, 9 Nov 2021 at 22:52, Leif Lindholm <leif@nuviainc.com> wrote: > > > > On Tue, Nov 09, 2021 at 21:21:46 +0000, Peter Maydell wrote: > > > The other thing we should nail down is how the user is going to > > > select which flavour of machine they want to provide. Three > > > options: > > > (1) no control, QEMU just emulates whatever the newest flavour is. > > > User needs to go find a firmware image new enough to cope. > > > (2) different flavours exposed as different machine types > > > (analogous to how we have musca-a and musca-b1, or raspi3ap and > > > raspi3b, for instance). Old user command lines keep working > > > because -M sbsa-ref doesn't change; the new stuff would be > > > available via -M sbsa-ref-2 or whatever. > > > (3) different flavours exposed via a property > > > (so you would have -M sbsa-ref,machine-revision=2 or something). > > > If the revision defaults to 1 then old user setups still work > > > but everybody starts to have to cart around an extra command > > > line argument. If it defaults to "newest we know about" you > > > get the opposite set of tradeoffs. > > > > I'm leaning towards (1), at least while working towards a "complete" > > platform (when we may still add/change features, but not how those > > features are communicated to the firmware). > > That's certainly the easiest on the QEMU side; you know the > userbase so would know whether that kind of compat break is > going to be OK with them. > > Q1: who is going to write the code for this? Me, my team, and perhaps a little bit of help from Shashi where it intersects his code. > Q2: do we want to try to land "ITS in sbsa-ref" in 6.2? Given > we're in freeze we're quite short of time even if we handwave > the fact it's a new feature, not a bugfix, so I would lean > towards 'no'... Shashi - what is your feeling? If we could make ITS support depend on the platform version being communicated through TF-A, we could simplify the transition a lot. But that would definitely mean missing 6.2. Peter - could we sneak in an "add version node to DT" into 6.2? / Leif
diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c index c1629df603..feadae2f33 100644 --- a/hw/arm/sbsa-ref.c +++ b/hw/arm/sbsa-ref.c @@ -34,7 +34,7 @@ #include "hw/boards.h" #include "hw/ide/internal.h" #include "hw/ide/ahci_internal.h" -#include "hw/intc/arm_gicv3_common.h" +#include "hw/intc/arm_gicv3_its_common.h" #include "hw/loader.h" #include "hw/pci-host/gpex.h" #include "hw/qdev-properties.h" @@ -58,12 +58,26 @@ #define ARCH_TIMER_NS_EL1_IRQ 14 #define ARCH_TIMER_NS_EL2_IRQ 10 +/* + * Enumeration of the possible values of sbsa-ref version + * property. These are arbitrary QEMU-internal values. + * values are :- + * DEFAULT = without ITS memory map + * SBSA_GIC_ITS = with ITS memory map between distributor & redistributor + * regions. This is the current version supported. + */ +typedef enum SbsaRefVersion { + SBSA_DEFAULT, + SBSA_ITS, +} SbsaRefVersion; + enum { SBSA_FLASH, SBSA_MEM, SBSA_CPUPERIPHS, SBSA_GIC_DIST, SBSA_GIC_REDIST, + SBSA_GIC_ITS, SBSA_SECURE_EC, SBSA_GWDT, SBSA_GWDT_REFRESH, @@ -91,6 +105,7 @@ struct SBSAMachineState { void *fdt; int fdt_size; int psci_conduit; + SbsaRefVersion version; DeviceState *gic; PFlashCFI01 *flash[2]; }; @@ -105,8 +120,11 @@ static const MemMapEntry sbsa_ref_memmap[] = { [SBSA_SECURE_MEM] = { 0x20000000, 0x20000000 }, /* Space reserved for CPU peripheral devices */ [SBSA_CPUPERIPHS] = { 0x40000000, 0x00040000 }, + /* GIC components reserved space Start */ [SBSA_GIC_DIST] = { 0x40060000, 0x00010000 }, - [SBSA_GIC_REDIST] = { 0x40080000, 0x04000000 }, + [SBSA_GIC_ITS] = { 0x40070000, 0x00020000 }, + [SBSA_GIC_REDIST] = { 0x400B0000, 0x04000000 }, + /* GIC components reserved space End */ [SBSA_SECURE_EC] = { 0x50000000, 0x00001000 }, [SBSA_GWDT_REFRESH] = { 0x50010000, 0x00001000 }, [SBSA_GWDT_CONTROL] = { 0x50011000, 0x00001000 }, @@ -377,7 +395,20 @@ static void create_secure_ram(SBSAMachineState *sms, memory_region_add_subregion(secure_sysmem, base, secram); } -static void create_gic(SBSAMachineState *sms) +static void create_its(SBSAMachineState *sms) +{ + DeviceState *dev; + + dev = qdev_new(TYPE_ARM_GICV3_ITS); + SysBusDevice *s = SYS_BUS_DEVICE(dev); + + object_property_set_link(OBJECT(dev), "parent-gicv3", OBJECT(sms->gic), + &error_abort); + sysbus_realize_and_unref(s, &error_fatal); + sysbus_mmio_map(s, 0, sbsa_ref_memmap[SBSA_GIC_ITS].base); +} + +static void create_gic(SBSAMachineState *sms, MemoryRegion *mem) { unsigned int smp_cpus = MACHINE(sms)->smp.cpus; SysBusDevice *gicbusdev; @@ -404,6 +435,10 @@ static void create_gic(SBSAMachineState *sms) qdev_prop_set_uint32(sms->gic, "len-redist-region-count", 1); qdev_prop_set_uint32(sms->gic, "redist-region-count[0]", redist0_count); + object_property_set_link(OBJECT(sms->gic), "sysmem", OBJECT(mem), + &error_fatal); + qdev_prop_set_bit(sms->gic, "has-lpi", true); + gicbusdev = SYS_BUS_DEVICE(sms->gic); sysbus_realize_and_unref(gicbusdev, &error_fatal); sysbus_mmio_map(gicbusdev, 0, sbsa_ref_memmap[SBSA_GIC_DIST].base); @@ -450,6 +485,7 @@ static void create_gic(SBSAMachineState *sms) sysbus_connect_irq(gicbusdev, i + 3 * smp_cpus, qdev_get_gpio_in(cpudev, ARM_CPU_VFIQ)); } + create_its(sms); } static void create_uart(const SBSAMachineState *sms, int uart, @@ -755,7 +791,7 @@ static void sbsa_ref_init(MachineState *machine) create_secure_ram(sms, secure_sysmem); - create_gic(sms); + create_gic(sms, sysmem); create_uart(sms, SBSA_UART, sysmem, serial_hd(0)); create_uart(sms, SBSA_SECURE_UART, secure_sysmem, serial_hd(1)); @@ -825,10 +861,39 @@ sbsa_ref_get_default_cpu_node_id(const MachineState *ms, int idx) return idx % ms->numa_state->num_nodes; } +static char *sbsa_get_version(Object *obj, Error **errp) +{ + SBSAMachineState *sms = SBSA_MACHINE(obj); + + switch (sms->version) { + case SBSA_DEFAULT: + return g_strdup("default"); + case SBSA_ITS: + return g_strdup("sbsaits"); + default: + g_assert_not_reached(); + } +} + +static void sbsa_set_version(Object *obj, const char *value, Error **errp) +{ + SBSAMachineState *sms = SBSA_MACHINE(obj); + + if (!strcmp(value, "sbsaits")) { + sms->version = SBSA_ITS; + } else if (!strcmp(value, "default")) { + sms->version = SBSA_DEFAULT; + } else { + error_setg(errp, "Invalid version value"); + error_append_hint(errp, "Valid values are default, sbsaits.\n"); + } +} + static void sbsa_ref_instance_init(Object *obj) { SBSAMachineState *sms = SBSA_MACHINE(obj); + sms->version = SBSA_ITS; sbsa_flash_create(sms); } @@ -850,6 +915,12 @@ static void sbsa_ref_class_init(ObjectClass *oc, void *data) mc->possible_cpu_arch_ids = sbsa_ref_possible_cpu_arch_ids; mc->cpu_index_to_instance_props = sbsa_ref_cpu_index_to_props; mc->get_default_cpu_node_id = sbsa_ref_get_default_cpu_node_id; + + object_class_property_add_str(oc, "version", sbsa_get_version, + sbsa_set_version); + object_class_property_set_description(oc, "version", + "Set the Version type. " + "Valid values are default & sbsaits"); } static const TypeInfo sbsa_ref_info = {
Included creation of ITS as part of SBSA platform GIC initialization. Signed-off-by: Shashi Mallela <shashi.mallela@linaro.org> --- hw/arm/sbsa-ref.c | 79 ++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 75 insertions(+), 4 deletions(-)