Hi,
On 3/11/25 3:10 PM, Shameer Kolothum wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
>
> With nested translation, the underlying HW could support those two fields.
> Allow them according to the updated idr registers after the hw_info ioctl.
s/idr/IDR
>
> When substreams are enabled (S1CDMax != 0), S1DSS field determines
> the behavior of a transaction.
>
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
> hw/arm/smmuv3-internal.h | 1 +
> hw/arm/smmuv3.c | 15 +++++++++++++--
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 546f8faac0..530284a9c0 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -612,6 +612,7 @@ static inline void smmuv3_accel_install_nested_ste(SMMUDevice *sdev, int sid)
>
> #define STE_S1FMT(x) extract32((x)->word[0], 4 , 2)
> #define STE_S1CDMAX(x) extract32((x)->word[1], 27, 5)
> +#define STE_S1DSS(x) extract32((x)->word[2], 0, 2)
> #define STE_S1STALLD(x) extract32((x)->word[2], 27, 1)
> #define STE_EATS(x) extract32((x)->word[2], 28, 2)
> #define STE_STRW(x) extract32((x)->word[2], 30, 2)
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index e0f225d0df..e8a6c50056 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -561,6 +561,16 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
>
> decode_ste_config(cfg, config);
>
> + /* S1DSS.Terminate is same as Config.abort for default stream */
S1DSS. Termination
> + if (STE_CFG_S1_ENABLED(config) && STE_S1DSS(ste) == 0) {
> + cfg->aborted = true;
The spec says:
"
When substreams are enabled (STE.S1CDMax != 0), this field determines
the behavior of a transaction or
translation request that arrives without an associated substream:
"
So I understand you should also check STE.S1CDMax. Also how do we check
that the incoming transacrion arrives without a substream?
In general shouldn't we add the support of subtreams in the emulated
code too?
the spec also says that in that case you should record a
F_STREAM_DISABLED event.
> + }
> +
> + /* S1DSS.Bypass is same as Config.bypass for default stream */
S1DSS. Bypass
> + if (STE_CFG_S1_ENABLED(config) && STE_S1DSS(ste) == 0x1) {
> + cfg->bypassed = true;
> + }
> +
> if (cfg->aborted || cfg->bypassed) {
> return 0;
> }
> @@ -598,13 +608,14 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> }
> }
>
> - if (STE_S1CDMAX(ste) != 0) {
> + if (!FIELD_EX32(s->idr[1], IDR1, SSIDSIZE) && STE_S1CDMAX(ste) != 0) {
> qemu_log_mask(LOG_UNIMP,
> "SMMUv3 does not support multiple context descriptors yet\n");
the log message should be different because it becomes a guest error:
qemu_log_mask(LOG_GUEST_ERROR, "invalid S1CDMAX as SSIDSIZE==0") or
something alike
> goto bad_ste;
> }
>
> - if (STE_S1STALLD(ste)) {
> + /* STALL_MODEL being 0b01 means "stall is not supported" */
> + if ((FIELD_EX32(s->idr[0], IDR0, STALL_MODEL) & 0x1) && STE_S1STALLD(ste)) {
> qemu_log_mask(LOG_UNIMP,
> "SMMUv3 S1 stalling fault model not allowed yet\n");
same here.
Again I think we need to understand the consequence of having a more
comprehensive support of SSID. This also holds with old the IDR fields
that may be inherited from the HW and we don't support yet in the
emulation code
Thanks
Eric
> goto bad_ste;