* [RFC PATCH v2 01/11] hw/arm/smmuv3: Add missing fields for IDR0
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-02-26 22:06 ` [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2 Mostafa Saleh
` (9 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
In preparation for adding stage-2 support.
Add IDR0 fields related to stage-2.
VMID16: 16-bit VMID supported.
S2P: Stage-2 translation supported.
They are described in 6.3.1 SMMU_IDR0.
No functional change intended.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in V2:
- Collected Reviewed-by tags.
---
hw/arm/smmuv3-internal.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index e8f0ebf25e..183d5ac8dc 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -34,10 +34,12 @@ typedef enum SMMUTranslationStatus {
/* MMIO Registers */
REG32(IDR0, 0x0)
+ FIELD(IDR0, S2P, 0 , 1)
FIELD(IDR0, S1P, 1 , 1)
FIELD(IDR0, TTF, 2 , 2)
FIELD(IDR0, COHACC, 4 , 1)
FIELD(IDR0, ASID16, 12, 1)
+ FIELD(IDR0, VMID16, 18, 1)
FIELD(IDR0, TTENDIAN, 21, 2)
FIELD(IDR0, STALL_MODEL, 24, 2)
FIELD(IDR0, TERM_MODEL, 26, 1)
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
2023-02-26 22:06 ` [RFC PATCH v2 01/11] hw/arm/smmuv3: Add missing fields for IDR0 Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-17 11:37 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 03/11] hw/arm/smmuv3: Refactor stage-1 PTW Mostafa Saleh
` (8 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
In preparation for adding stage-2 support, add a S2 config
struct(SMMUS2Cfg), composed of the following fields and embedded in
the main SMMUTransCfg:
-tsz: Input range
-sl0: start level of translation
-affd: AF fault disable
-granule_sz: Granule page shift
-vmid: VMID
-vttb: PA of translation table
-oas: Output address size
They will be used in the next patches in stage-2 address translation.
No functional change intended.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v2:
-Add oas
---
include/hw/arm/smmu-common.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 9fcff26357..2deead08d6 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -58,6 +58,16 @@ typedef struct SMMUTLBEntry {
uint8_t granule;
} SMMUTLBEntry;
+typedef struct SMMUS2Cfg {
+ uint8_t tsz; /* Input range */
+ uint8_t sl0; /* Start level of translation */
+ bool affd; /* AF Fault Disable */
+ uint8_t granule_sz; /* Granule page shift */
+ uint8_t oas; /* Output address size */
+ uint16_t vmid; /* Virtual machine ID */
+ uint64_t vttb; /* PA of translation table */
+} SMMUS2Cfg;
+
/*
* Generic structure populated by derived SMMU devices
* after decoding the configuration information and used as
@@ -77,6 +87,7 @@ typedef struct SMMUTransCfg {
SMMUTransTableInfo tt[2];
uint32_t iotlb_hits; /* counts IOTLB hits for this asid */
uint32_t iotlb_misses; /* counts IOTLB misses for this asid */
+ struct SMMUS2Cfg s2cfg;
} SMMUTransCfg;
typedef struct SMMUDevice {
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2
2023-02-26 22:06 ` [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2 Mostafa Saleh
@ 2023-03-17 11:37 ` Eric Auger
2023-03-17 14:43 ` Mostafa Saleh
0 siblings, 1 reply; 37+ messages in thread
From: Eric Auger @ 2023-03-17 11:37 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi Mostafa,
On 2/26/23 23:06, Mostafa Saleh wrote:
> In preparation for adding stage-2 support, add a S2 config
> struct(SMMUS2Cfg), composed of the following fields and embedded in
> the main SMMUTransCfg:
> -tsz: Input range
> -sl0: start level of translation
> -affd: AF fault disable
> -granule_sz: Granule page shift
> -vmid: VMID
> -vttb: PA of translation table
> -oas: Output address size
>
> They will be used in the next patches in stage-2 address translation.
>
> No functional change intended.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in v2:
> -Add oas
> ---
> include/hw/arm/smmu-common.h | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 9fcff26357..2deead08d6 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -58,6 +58,16 @@ typedef struct SMMUTLBEntry {
> uint8_t granule;
> } SMMUTLBEntry;
>
> +typedef struct SMMUS2Cfg {
> + uint8_t tsz; /* Input range */
nit: Size of IPA input region. Called t0sz
> + uint8_t sl0; /* Start level of translation */
> + bool affd; /* AF Fault Disable */
> + uint8_t granule_sz; /* Granule page shift */
> + uint8_t oas; /* Output address size */
called s2ps. if you don't want to rename you may add this in the comment.
I am suprised to not see S2R which would match S1 record_faults.
Also without further reading we can wonder wheter the parent
iotlb_hits/misses also record S2 events?
I think we need to be/make clear what fields of the original S1 parent
struct also are relevant for the S2 only case.
Maybe tag them with a specific comment S1-only or S1 | S2. It may be
cleaner to introduce a union and common fields in the parent struct though.
Thanks
Eric
> + uint16_t vmid; /* Virtual machine ID */
> + uint64_t vttb; /* PA of translation table */
> +} SMMUS2Cfg;
> +
> /*
> * Generic structure populated by derived SMMU devices
> * after decoding the configuration information and used as
> @@ -77,6 +87,7 @@ typedef struct SMMUTransCfg {
> SMMUTransTableInfo tt[2];
> uint32_t iotlb_hits; /* counts IOTLB hits for this asid */
> uint32_t iotlb_misses; /* counts IOTLB misses for this asid */
> + struct SMMUS2Cfg s2cfg;
> } SMMUTransCfg;
>
> typedef struct SMMUDevice {
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2
2023-03-17 11:37 ` Eric Auger
@ 2023-03-17 14:43 ` Mostafa Saleh
2023-03-17 17:36 ` Eric Auger
0 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-17 14:43 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Eric,
Thanks for reviewing the patch.
On Fri, Mar 17, 2023 at 12:37:11PM +0100, Eric Auger wrote:
> Hi Mostafa,
>
> On 2/26/23 23:06, Mostafa Saleh wrote:
> > In preparation for adding stage-2 support, add a S2 config
> > struct(SMMUS2Cfg), composed of the following fields and embedded in
> > the main SMMUTransCfg:
> > -tsz: Input range
> > -sl0: start level of translation
> > -affd: AF fault disable
> > -granule_sz: Granule page shift
> > -vmid: VMID
> > -vttb: PA of translation table
> > -oas: Output address size
> >
> > They will be used in the next patches in stage-2 address translation.
> >
> > No functional change intended.
> >
> > Signed-off-by: Mostafa Saleh <smostafa@google.com>
> > ---
> > Changes in v2:
> > -Add oas
> > ---
> > include/hw/arm/smmu-common.h | 11 +++++++++++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> > index 9fcff26357..2deead08d6 100644
> > --- a/include/hw/arm/smmu-common.h
> > +++ b/include/hw/arm/smmu-common.h
> > @@ -58,6 +58,16 @@ typedef struct SMMUTLBEntry {
> > uint8_t granule;
> > } SMMUTLBEntry;
> >
> > +typedef struct SMMUS2Cfg {
> > + uint8_t tsz; /* Input range */
> nit: Size of IPA input region. Called t0sz
I named it this way to be similar to stage-1, as SMMUTransTableInfo
just calls it tsz, I guess that is because it can hold either t0sz or
t1sz.
I can rename it for stage-2 to t0sz.
> > + uint8_t sl0; /* Start level of translation */
> > + bool affd; /* AF Fault Disable */
> > + uint8_t granule_sz; /* Granule page shift */
> > + uint8_t oas; /* Output address size */
> called s2ps. if you don't want to rename you may add this in the comment.
This is not the S2PS parsed from the STE, but the effective value which is
capped to SMMU_IDR5.OAS, which is used for checking output size, I can add
a clarifying comment.
> I am suprised to not see S2R which would match S1 record_faults.
I was thinking about this also, right now we piggy back on record_faults
in SMMUTransCfg.
But it makes more sense to separate this from the beginning as other
fields. I will update that by adding record_faults field in SMMUS2Cfg.
> Also without further reading we can wonder wheter the parent
> iotlb_hits/misses also record S2 events?
For iotlb_*, they are shared also. However, I think this is not important for now as
it is not architectural, and it produces the correct output as only one
stage is advertised at the moment.
When nesting is supported, TLB implementation might change and then we
can take a decision about this.
> I think we need to be/make clear what fields of the original S1 parent
> struct also are relevant for the S2 only case.
> Maybe tag them with a specific comment S1-only or S1 | S2. It may be
> cleaner to introduce a union and common fields in the parent struct though.
I agree, maybe we encapsulate stage-1 only fields in a similar struct
and leave common fields outside.
struct SMMUTransCfg:
stage, bypassed, disabled, iotlb_* //common fields
struct SMMUS2Cfg
struct SMMUS1Cfg
ttb,asid....
Or we can add SMMUS1Cfg and SMMUS2Cfg in a union for now and when nesting is supported we
separate them.
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2
2023-03-17 14:43 ` Mostafa Saleh
@ 2023-03-17 17:36 ` Eric Auger
0 siblings, 0 replies; 37+ messages in thread
From: Eric Auger @ 2023-03-17 17:36 UTC (permalink / raw)
To: Mostafa Saleh
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Mostafa,
On 3/17/23 15:43, Mostafa Saleh wrote:
> Hi Eric,
>
> Thanks for reviewing the patch.
>
> On Fri, Mar 17, 2023 at 12:37:11PM +0100, Eric Auger wrote:
>> Hi Mostafa,
>>
>> On 2/26/23 23:06, Mostafa Saleh wrote:
>>> In preparation for adding stage-2 support, add a S2 config
>>> struct(SMMUS2Cfg), composed of the following fields and embedded in
>>> the main SMMUTransCfg:
>>> -tsz: Input range
>>> -sl0: start level of translation
>>> -affd: AF fault disable
>>> -granule_sz: Granule page shift
>>> -vmid: VMID
>>> -vttb: PA of translation table
>>> -oas: Output address size
>>>
>>> They will be used in the next patches in stage-2 address translation.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Mostafa Saleh <smostafa@google.com>
>>> ---
>>> Changes in v2:
>>> -Add oas
>>> ---
>>> include/hw/arm/smmu-common.h | 11 +++++++++++
>>> 1 file changed, 11 insertions(+)
>>>
>>> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
>>> index 9fcff26357..2deead08d6 100644
>>> --- a/include/hw/arm/smmu-common.h
>>> +++ b/include/hw/arm/smmu-common.h
>>> @@ -58,6 +58,16 @@ typedef struct SMMUTLBEntry {
>>> uint8_t granule;
>>> } SMMUTLBEntry;
>>>
>>> +typedef struct SMMUS2Cfg {
>>> + uint8_t tsz; /* Input range */
>> nit: Size of IPA input region. Called t0sz
> I named it this way to be similar to stage-1, as SMMUTransTableInfo
> just calls it tsz, I guess that is because it can hold either t0sz or
> t1sz.
> I can rename it for stage-2 to t0sz.
no strong opinion though, you can simply add this in the comment
>
>>> + uint8_t sl0; /* Start level of translation */
>>> + bool affd; /* AF Fault Disable */
>>> + uint8_t granule_sz; /* Granule page shift */
>>> + uint8_t oas; /* Output address size */
>> called s2ps. if you don't want to rename you may add this in the comment.
> This is not the S2PS parsed from the STE, but the effective value which is
> capped to SMMU_IDR5.OAS, which is used for checking output size, I can add
> a clarifying comment.
ok
>
>> I am suprised to not see S2R which would match S1 record_faults.
> I was thinking about this also, right now we piggy back on record_faults
> in SMMUTransCfg.
> But it makes more sense to separate this from the beginning as other
> fields. I will update that by adding record_faults field in SMMUS2Cfg.
ok
>
>> Also without further reading we can wonder wheter the parent
>> iotlb_hits/misses also record S2 events?
> For iotlb_*, they are shared also. However, I think this is not important for now as
> it is not architectural, and it produces the correct output as only one
> stage is advertised at the moment.
> When nesting is supported, TLB implementation might change and then we
> can take a decision about this.
>
>> I think we need to be/make clear what fields of the original S1 parent
>> struct also are relevant for the S2 only case.
>> Maybe tag them with a specific comment S1-only or S1 | S2. It may be
>> cleaner to introduce a union and common fields in the parent struct though.
> I agree, maybe we encapsulate stage-1 only fields in a similar struct
> and leave common fields outside.
>
> struct SMMUTransCfg:
> stage, bypassed, disabled, iotlb_* //common fields
> struct SMMUS2Cfg
> struct SMMUS1Cfg
> ttb,asid....
>
> Or we can add SMMUS1Cfg and SMMUS2Cfg in a union for now and when nesting is supported we
> separate them.
yeah that would be cleaner but it is also more invasive. At least make
it clear in the comments and commit msg what is used for each stage.
Thanks
Eric
>
>
> Thanks,
> Mostafa
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 03/11] hw/arm/smmuv3: Refactor stage-1 PTW
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
2023-02-26 22:06 ` [RFC PATCH v2 01/11] hw/arm/smmuv3: Add missing fields for IDR0 Mostafa Saleh
2023-02-26 22:06 ` [RFC PATCH v2 02/11] hw/arm/smmuv3: Update translation config to hold stage-2 Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-17 18:31 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2 Mostafa Saleh
` (7 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
In preparation for adding stage-2 support, rename smmu_ptw_64 to
smmu_ptw_64_s1 and refactor some of the code so it can be reused in
stage-2 page table walk.
Remove AA64 check from PTW as decode_cd already ensures that AA64 is
used, otherwise it faults with C_BAD_CD.
A stage member is added to SMMUPTWEventInfo to differentiate
between stage-1 and stage-2 ptw faults.
Add stage argument to trace_smmu_ptw_level be consistent with other
trace events.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v2:
- Refactor common functions to be use in stage-2.
- Add stage to SMMUPTWEventInfo.
- Remove AA64 check.
---
hw/arm/smmu-common.c | 27 ++++++++++-----------------
hw/arm/smmuv3.c | 2 ++
hw/arm/trace-events | 2 +-
include/hw/arm/smmu-common.h | 15 ++++++++++++---
4 files changed, 25 insertions(+), 21 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 0a5a60ca1e..b49c1affdb 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -264,7 +264,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
}
/**
- * smmu_ptw_64 - VMSAv8-64 Walk of the page tables for a given IOVA
+ * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
* @cfg: translation config
* @iova: iova to translate
* @perm: access type
@@ -276,9 +276,9 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
* Upon success, @tlbe is filled with translated_addr and entry
* permission rights.
*/
-static int smmu_ptw_64(SMMUTransCfg *cfg,
- dma_addr_t iova, IOMMUAccessFlags perm,
- SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
+static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
+ dma_addr_t iova, IOMMUAccessFlags perm,
+ SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
{
dma_addr_t baseaddr, indexmask;
int stage = cfg->stage;
@@ -291,14 +291,14 @@ static int smmu_ptw_64(SMMUTransCfg *cfg,
}
granule_sz = tt->granule_sz;
- stride = granule_sz - 3;
+ stride = SMMU_STRIDE(granule_sz);
inputsize = 64 - tt->tsz;
level = 4 - (inputsize - 4) / stride;
- indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
+ indexmask = SMMU_IDXMSK(inputsize, stride, level);
baseaddr = extract64(tt->ttb, 0, 48);
baseaddr &= ~indexmask;
- while (level <= 3) {
+ while (level < SMMU_LEVELS) {
uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
uint64_t mask = subpage_size - 1;
uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
@@ -309,7 +309,7 @@ static int smmu_ptw_64(SMMUTransCfg *cfg,
if (get_pte(baseaddr, offset, &pte, info)) {
goto error;
}
- trace_smmu_ptw_level(level, iova, subpage_size,
+ trace_smmu_ptw_level(stage, level, iova, subpage_size,
baseaddr, offset, pte);
if (is_invalid_pte(pte) || is_reserved_pte(pte, level)) {
@@ -358,6 +358,7 @@ static int smmu_ptw_64(SMMUTransCfg *cfg,
info->type = SMMU_PTW_ERR_TRANSLATION;
error:
+ info->stage = 1;
tlbe->entry.perm = IOMMU_NONE;
return -EINVAL;
}
@@ -376,15 +377,7 @@ error:
int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
{
- if (!cfg->aa64) {
- /*
- * This code path is not entered as we check this while decoding
- * the configuration data in the derived SMMU model.
- */
- g_assert_not_reached();
- }
-
- return smmu_ptw_64(cfg, iova, perm, tlbe, info);
+ return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
}
/**
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 270c80b665..4e90343996 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -716,6 +716,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
cached_entry = g_new0(SMMUTLBEntry, 1);
if (smmu_ptw(cfg, aligned_addr, flag, cached_entry, &ptw_info)) {
+ /* All faults from PTW has S2 field. */
+ event.u.f_walk_eabt.s2 = (ptw_info.stage == 2);
g_free(cached_entry);
switch (ptw_info.type) {
case SMMU_PTW_ERR_WALK_EABT:
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 2dee296c8f..205ac04573 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -5,7 +5,7 @@ virt_acpi_setup(void) "No fw cfg or ACPI disabled. Bailing out."
# smmu-common.c
smmu_add_mr(const char *name) "%s"
-smmu_ptw_level(int level, uint64_t iova, size_t subpage_size, uint64_t baseaddr, uint32_t offset, uint64_t pte) "level=%d iova=0x%"PRIx64" subpage_sz=0x%zx baseaddr=0x%"PRIx64" offset=%d => pte=0x%"PRIx64
+smmu_ptw_level(int stage, int level, uint64_t iova, size_t subpage_size, uint64_t baseaddr, uint32_t offset, uint64_t pte) "stage=%d level=%d iova=0x%"PRIx64" subpage_sz=0x%zx baseaddr=0x%"PRIx64" offset=%d => pte=0x%"PRIx64
smmu_ptw_invalid_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint32_t offset, uint64_t pte) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" offset=%d pte=0x%"PRIx64
smmu_ptw_page_pte(int stage, int level, uint64_t iova, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t address) "stage=%d level=%d iova=0x%"PRIx64" base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" page address = 0x%"PRIx64
smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t iova, uint64_t gpa, int bsize_mb) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" iova=0x%"PRIx64" block address = 0x%"PRIx64" block size = %d MiB"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 2deead08d6..1ada792122 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -23,9 +23,17 @@
#include "hw/pci/pci.h"
#include "qom/object.h"
-#define SMMU_PCI_BUS_MAX 256
-#define SMMU_PCI_DEVFN_MAX 256
-#define SMMU_PCI_DEVFN(sid) (sid & 0xFF)
+#define SMMU_PCI_BUS_MAX 256
+#define SMMU_PCI_DEVFN_MAX 256
+#define SMMU_PCI_DEVFN(sid) (sid & 0xFF)
+
+#define SMMU_LEVELS 4
+
+#define SMMU_STRIDE(gran) ((gran) - SMMU_LEVELS + 1)
+#define SMMU_BIT_LVL(isz, strd, lvl) ((isz) - (strd) * \
+ (SMMU_LEVELS - (lvl)))
+#define SMMU_IDXMSK(isz, strd, lvl) ((1ULL << \
+ SMMU_BIT_LVL(isz, strd, lvl)) - 1)
/*
* Page table walk error types
@@ -40,6 +48,7 @@ typedef enum {
} SMMUPTWEventType;
typedef struct SMMUPTWEventInfo {
+ int stage;
SMMUPTWEventType type;
dma_addr_t addr; /* fetched address that induced an abort, if any */
} SMMUPTWEventInfo;
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 03/11] hw/arm/smmuv3: Refactor stage-1 PTW
2023-02-26 22:06 ` [RFC PATCH v2 03/11] hw/arm/smmuv3: Refactor stage-1 PTW Mostafa Saleh
@ 2023-03-17 18:31 ` Eric Auger
2023-03-19 8:38 ` Mostafa Saleh
0 siblings, 1 reply; 37+ messages in thread
From: Eric Auger @ 2023-03-17 18:31 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi Mostafa,
On 2/26/23 23:06, Mostafa Saleh wrote:
> In preparation for adding stage-2 support, rename smmu_ptw_64 to
> smmu_ptw_64_s1 and refactor some of the code so it can be reused in
> stage-2 page table walk.
>
> Remove AA64 check from PTW as decode_cd already ensures that AA64 is
> used, otherwise it faults with C_BAD_CD.
>
> A stage member is added to SMMUPTWEventInfo to differentiate
> between stage-1 and stage-2 ptw faults.
>
> Add stage argument to trace_smmu_ptw_level be consistent with other
> trace events.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in v2:
> - Refactor common functions to be use in stage-2.
> - Add stage to SMMUPTWEventInfo.
> - Remove AA64 check.
> ---
> hw/arm/smmu-common.c | 27 ++++++++++-----------------
> hw/arm/smmuv3.c | 2 ++
> hw/arm/trace-events | 2 +-
> include/hw/arm/smmu-common.h | 15 ++++++++++++---
> 4 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 0a5a60ca1e..b49c1affdb 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -264,7 +264,7 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
> }
>
> /**
> - * smmu_ptw_64 - VMSAv8-64 Walk of the page tables for a given IOVA
> + * smmu_ptw_64_s1 - VMSAv8-64 Walk of the page tables for a given IOVA
> * @cfg: translation config
> * @iova: iova to translate
> * @perm: access type
> @@ -276,9 +276,9 @@ SMMUTransTableInfo *select_tt(SMMUTransCfg *cfg, dma_addr_t iova)
> * Upon success, @tlbe is filled with translated_addr and entry
> * permission rights.
> */
> -static int smmu_ptw_64(SMMUTransCfg *cfg,
> - dma_addr_t iova, IOMMUAccessFlags perm,
> - SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> + dma_addr_t iova, IOMMUAccessFlags perm,
> + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> {
> dma_addr_t baseaddr, indexmask;
> int stage = cfg->stage;
> @@ -291,14 +291,14 @@ static int smmu_ptw_64(SMMUTransCfg *cfg,
> }
>
> granule_sz = tt->granule_sz;
> - stride = granule_sz - 3;
> + stride = SMMU_STRIDE(granule_sz);
> inputsize = 64 - tt->tsz;
> level = 4 - (inputsize - 4) / stride;
> - indexmask = (1ULL << (inputsize - (stride * (4 - level)))) - 1;
> + indexmask = SMMU_IDXMSK(inputsize, stride, level);
> baseaddr = extract64(tt->ttb, 0, 48);
> baseaddr &= ~indexmask;
>
> - while (level <= 3) {
> + while (level < SMMU_LEVELS) {
> uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
> uint64_t mask = subpage_size - 1;
> uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
> @@ -309,7 +309,7 @@ static int smmu_ptw_64(SMMUTransCfg *cfg,
> if (get_pte(baseaddr, offset, &pte, info)) {
> goto error;
> }
> - trace_smmu_ptw_level(level, iova, subpage_size,
> + trace_smmu_ptw_level(stage, level, iova, subpage_size,
> baseaddr, offset, pte);
>
> if (is_invalid_pte(pte) || is_reserved_pte(pte, level)) {
> @@ -358,6 +358,7 @@ static int smmu_ptw_64(SMMUTransCfg *cfg,
> info->type = SMMU_PTW_ERR_TRANSLATION;
>
> error:
> + info->stage = 1;
> tlbe->entry.perm = IOMMU_NONE;
> return -EINVAL;
> }
> @@ -376,15 +377,7 @@ error:
> int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> {
> - if (!cfg->aa64) {
> - /*
> - * This code path is not entered as we check this while decoding
> - * the configuration data in the derived SMMU model.
> - */
> - g_assert_not_reached();
> - }
> -
> - return smmu_ptw_64(cfg, iova, perm, tlbe, info);
> + return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> }
>
> /**
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 270c80b665..4e90343996 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -716,6 +716,8 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> cached_entry = g_new0(SMMUTLBEntry, 1);
>
> if (smmu_ptw(cfg, aligned_addr, flag, cached_entry, &ptw_info)) {
> + /* All faults from PTW has S2 field. */
> + event.u.f_walk_eabt.s2 = (ptw_info.stage == 2);
> g_free(cached_entry);
> switch (ptw_info.type) {
> case SMMU_PTW_ERR_WALK_EABT:
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 2dee296c8f..205ac04573 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -5,7 +5,7 @@ virt_acpi_setup(void) "No fw cfg or ACPI disabled. Bailing out."
>
> # smmu-common.c
> smmu_add_mr(const char *name) "%s"
> -smmu_ptw_level(int level, uint64_t iova, size_t subpage_size, uint64_t baseaddr, uint32_t offset, uint64_t pte) "level=%d iova=0x%"PRIx64" subpage_sz=0x%zx baseaddr=0x%"PRIx64" offset=%d => pte=0x%"PRIx64
> +smmu_ptw_level(int stage, int level, uint64_t iova, size_t subpage_size, uint64_t baseaddr, uint32_t offset, uint64_t pte) "stage=%d level=%d iova=0x%"PRIx64" subpage_sz=0x%zx baseaddr=0x%"PRIx64" offset=%d => pte=0x%"PRIx64
> smmu_ptw_invalid_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint32_t offset, uint64_t pte) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" offset=%d pte=0x%"PRIx64
> smmu_ptw_page_pte(int stage, int level, uint64_t iova, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t address) "stage=%d level=%d iova=0x%"PRIx64" base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" page address = 0x%"PRIx64
> smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, uint64_t pte, uint64_t iova, uint64_t gpa, int bsize_mb) "stage=%d level=%d base@=0x%"PRIx64" pte@=0x%"PRIx64" pte=0x%"PRIx64" iova=0x%"PRIx64" block address = 0x%"PRIx64" block size = %d MiB"
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 2deead08d6..1ada792122 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -23,9 +23,17 @@
> #include "hw/pci/pci.h"
> #include "qom/object.h"
>
> -#define SMMU_PCI_BUS_MAX 256
> -#define SMMU_PCI_DEVFN_MAX 256
> -#define SMMU_PCI_DEVFN(sid) (sid & 0xFF)
> +#define SMMU_PCI_BUS_MAX 256
> +#define SMMU_PCI_DEVFN_MAX 256
> +#define SMMU_PCI_DEVFN(sid) (sid & 0xFF)
> +
> +#define SMMU_LEVELS 4
> +
> +#define SMMU_STRIDE(gran) ((gran) - SMMU_LEVELS + 1)
> +#define SMMU_BIT_LVL(isz, strd, lvl) ((isz) - (strd) * \
> + (SMMU_LEVELS - (lvl)))
> +#define SMMU_IDXMSK(isz, strd, lvl) ((1ULL << \
> + SMMU_BIT_LVL(isz, strd, lvl)) - 1)
This looks good to me. Just a question about the BIT_LVL and IDXMSK
defines. Do they correspond to any documented pseudocode functions
documented somewhere in the ARM ARM?
Besides
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
>
> /*
> * Page table walk error types
> @@ -40,6 +48,7 @@ typedef enum {
> } SMMUPTWEventType;
>
> typedef struct SMMUPTWEventInfo {
> + int stage;
> SMMUPTWEventType type;
> dma_addr_t addr; /* fetched address that induced an abort, if any */
> } SMMUPTWEventInfo;
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 03/11] hw/arm/smmuv3: Refactor stage-1 PTW
2023-03-17 18:31 ` Eric Auger
@ 2023-03-19 8:38 ` Mostafa Saleh
0 siblings, 0 replies; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-19 8:38 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Eric,
On Fri, Mar 17, 2023 at 07:31:06PM +0100, Eric Auger wrote:
> > +#define SMMU_LEVELS 4
> > +
> > +#define SMMU_STRIDE(gran) ((gran) - SMMU_LEVELS + 1)
> > +#define SMMU_BIT_LVL(isz, strd, lvl) ((isz) - (strd) * \
> > + (SMMU_LEVELS - (lvl)))
> > +#define SMMU_IDXMSK(isz, strd, lvl) ((1ULL << \
> > + SMMU_BIT_LVL(isz, strd, lvl)) - 1)
> This looks good to me. Just a question about the BIT_LVL and IDXMSK
> defines. Do they correspond to any documented pseudocode functions
> documented somewhere in the ARM ARM?
I see they are not implemented as functions in ARM ARM, but as part of
aarch64/translation/vmsa_addrcalc/AArch64.TTBaseAddress:
constant integer FINAL_LEVEL = 3;
levels = FINAL_LEVEL - startlevel;
tsize = (iasize - (levels*stride + granulebits)) + 3;
tablebase = Align(tablebase, 1 << tsize);
This gives the same result, however the equations are a bit different as
they use final level(3), while we use number of levels(4).
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
` (2 preceding siblings ...)
2023-02-26 22:06 ` [RFC PATCH v2 03/11] hw/arm/smmuv3: Refactor stage-1 PTW Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-20 14:56 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 05/11] hw/arm/smmuv3: Parse STE config " Mostafa Saleh
` (6 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
In preparation for adding stage-2 support, add Stage-2 PTW code.
Only Aarch64 format is supported as stage-1.
Nesting stage-1 and stage-2 is not supported right now.
HTTU is not supported, SW is expected to maintain the Access flag.
This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
"[181] S2AFFD".
This flag determines the behavior on access of a stage-2 page whose
descriptor has AF == 0:
- 0b0: An Access flag fault occurs (stall not supported).
- 0b1: An Access flag fault never occurs.
An Access fault takes priority over a Permission fault.
Checks for IPA and output PA are done according to the user manual
"3.4 Address sizes".
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v2:
- Squash S2AFF PTW code.
- Use common functions between stage-1 and stage-2.
- Add checks for IPA and out PA.
---
hw/arm/smmu-common.c | 132 ++++++++++++++++++++++++++++++++++++++++-
hw/arm/smmu-internal.h | 39 ++++++++++++
2 files changed, 170 insertions(+), 1 deletion(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index b49c1affdb..3f448bc82e 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -363,6 +363,130 @@ error:
return -EINVAL;
}
+/**
+ * smmu_ptw_64_s2 - VMSAv8-64 Walk of the page tables for a given IOVA
+ * for stage-2.
+ * @cfg: translation config
+ * @iova: iova to translate
+ * @perm: access type
+ * @tlbe: SMMUTLBEntry (out)
+ * @info: handle to an error info
+ *
+ * Return 0 on success, < 0 on error. In case of error, @info is filled
+ * and tlbe->perm is set to IOMMU_NONE.
+ * Upon success, @tlbe is filled with translated_addr and entry
+ * permission rights.
+ */
+static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
+ dma_addr_t iova, IOMMUAccessFlags perm,
+ SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
+{
+ const int stage = 2;
+ int granule_sz = cfg->s2cfg.granule_sz;
+ /* ARM ARM: Table D8-7. */
+ int inputsize = 64 - cfg->s2cfg.tsz;
+ int level = get_start_level(cfg->s2cfg.sl0, granule_sz);
+ int stride = SMMU_STRIDE(granule_sz);
+ int idx = pgd_idx(level, granule_sz, iova);
+ /*
+ * Get the ttb from concatenated structure.
+ * The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
+ */
+ uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
+ idx * sizeof(uint64_t);
+ dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
+
+ baseaddr &= ~indexmask;
+
+ /*
+ * If the input address of a transaction exceeds the size of the IAS, a
+ * stage 1 Address Size fault occurs.
+ * For AA64, IAS = OAS
+ */
+ if (iova >= (1ULL << cfg->s2cfg.oas)) {
+ info->type = SMMU_PTW_ERR_ADDR_SIZE;
+ info->stage = 1;
+ goto error_no_stage;
+ }
+
+ while (level < SMMU_LEVELS) {
+ uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
+ uint64_t mask = subpage_size - 1;
+ uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
+ uint64_t pte, gpa;
+ dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
+ uint8_t ap;
+
+ if (get_pte(baseaddr, offset, &pte, info)) {
+ goto error;
+ }
+ trace_smmu_ptw_level(stage, level, iova, subpage_size,
+ baseaddr, offset, pte);
+ if (is_invalid_pte(pte) || is_reserved_pte(pte, level)) {
+ trace_smmu_ptw_invalid_pte(stage, level, baseaddr,
+ pte_addr, offset, pte);
+ break;
+ }
+
+ if (is_table_pte(pte, level)) {
+ baseaddr = get_table_pte_address(pte, granule_sz);
+ level++;
+ continue;
+ } else if (is_page_pte(pte, level)) {
+ gpa = get_page_pte_address(pte, granule_sz);
+ trace_smmu_ptw_page_pte(stage, level, iova,
+ baseaddr, pte_addr, pte, gpa);
+ } else {
+ uint64_t block_size;
+
+ gpa = get_block_pte_address(pte, level, granule_sz,
+ &block_size);
+ trace_smmu_ptw_block_pte(stage, level, baseaddr,
+ pte_addr, pte, iova, gpa,
+ block_size >> 20);
+ }
+
+ /*
+ * If S2AFFD and PTE.AF are 0 => fault. (5.2. Stream Table Entry)
+ * An Access fault takes priority over a Permission fault.
+ */
+ if (!PTE_AF(pte) && !cfg->s2cfg.affd) {
+ info->type = SMMU_PTW_ERR_ACCESS;
+ goto error;
+ }
+
+ ap = PTE_AP(pte);
+ if (is_permission_fault_s2(ap, perm)) {
+ info->type = SMMU_PTW_ERR_PERMISSION;
+ goto error;
+ }
+
+ /*
+ * The address output from the translation causes a stage 2 Address
+ * Size fault if it exceeds the effective PA output range.
+ */
+ if (gpa >= (1ULL << cfg->s2cfg.oas)) {
+ info->type = SMMU_PTW_ERR_ADDR_SIZE;
+ goto error;
+ }
+
+ tlbe->entry.translated_addr = gpa;
+ tlbe->entry.iova = iova & ~mask;
+ tlbe->entry.addr_mask = mask;
+ tlbe->entry.perm = ap;
+ tlbe->level = level;
+ tlbe->granule = granule_sz;
+ return 0;
+ }
+ info->type = SMMU_PTW_ERR_TRANSLATION;
+
+error:
+ info->stage = 2;
+error_no_stage:
+ tlbe->entry.perm = IOMMU_NONE;
+ return -EINVAL;
+}
+
/**
* smmu_ptw - Walk the page tables for an IOVA, according to @cfg
*
@@ -377,7 +501,13 @@ error:
int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
{
- return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
+ if (cfg->stage == 1) {
+ return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
+ } else if (cfg->stage == 2) {
+ return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
+ }
+
+ g_assert_not_reached();
}
/**
diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index 2d75b31953..f79c389cd3 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -66,6 +66,8 @@
#define PTE_APTABLE(pte) \
(extract64(pte, 61, 2))
+#define PTE_AF(pte) \
+ (extract64(pte, 10, 1))
/*
* TODO: At the moment all transactions are considered as privileged (EL1)
* as IOMMU translation callback does not pass user/priv attributes.
@@ -73,6 +75,9 @@
#define is_permission_fault(ap, perm) \
(((perm) & IOMMU_WO) && ((ap) & 0x2))
+#define is_permission_fault_s2(ap, perm) \
+ (!((ap & perm) == perm))
+
#define PTE_AP_TO_PERM(ap) \
(IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
@@ -96,6 +101,40 @@ uint64_t iova_level_offset(uint64_t iova, int inputsize,
MAKE_64BIT_MASK(0, gsz - 3);
}
+#define SMMU_MAX_S2_CONCAT 16
+
+/*
+ * Relies on correctness of gran and sl0 from caller.
+ * FEAT_LPA2 and FEAT_TTST are not implemented.
+ */
+static inline int get_start_level(int sl0 , int gran)
+{
+ /* ARM ARM: Table D8-12. */
+ if (gran == 12) {
+ return 2 - sl0;
+ }
+ /* ARM ARM: Table D8-22 and Table D8-31. */
+ return 3 - sl0;
+}
+
+/*
+ * Index in a concatenated first level stage-2 page table.
+ * ARM ARM: D8.2.2 Concatenated translation tables.
+ */
+static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
+{
+ uint64_t ret;
+ /*
+ * Get the number of bits handled by next levels, then any extra bits in
+ * the address should index the concatenated tables. This relation can
+ * deduced from tables in ARM ARM: D8.2.7-9
+ */
+ int shift = (SMMU_LEVELS - start_level) * (granule - 3) + granule;
+
+ ret = iova >> shift;
+ return ret;
+}
+
#define SMMU_IOTLB_ASID(key) ((key).asid)
typedef struct SMMUIOTLBPageInvInfo {
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2
2023-02-26 22:06 ` [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2 Mostafa Saleh
@ 2023-03-20 14:56 ` Eric Auger
2023-03-20 18:52 ` Mostafa Saleh
0 siblings, 1 reply; 37+ messages in thread
From: Eric Auger @ 2023-03-20 14:56 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi Mostafa,
On 2/26/23 23:06, Mostafa Saleh wrote:
> In preparation for adding stage-2 support, add Stage-2 PTW code.
> Only Aarch64 format is supported as stage-1.
>
> Nesting stage-1 and stage-2 is not supported right now.
>
> HTTU is not supported, SW is expected to maintain the Access flag.
> This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
> "[181] S2AFFD".
> This flag determines the behavior on access of a stage-2 page whose
> descriptor has AF == 0:
> - 0b0: An Access flag fault occurs (stall not supported).
> - 0b1: An Access flag fault never occurs.
> An Access fault takes priority over a Permission fault.
>
> Checks for IPA and output PA are done according to the user manual
> "3.4 Address sizes".
replace user manual by the exact ref of the doc. I guess this is IHI0070E
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in v2:
> - Squash S2AFF PTW code.
> - Use common functions between stage-1 and stage-2.
> - Add checks for IPA and out PA.
> ---
> hw/arm/smmu-common.c | 132 ++++++++++++++++++++++++++++++++++++++++-
> hw/arm/smmu-internal.h | 39 ++++++++++++
> 2 files changed, 170 insertions(+), 1 deletion(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index b49c1affdb..3f448bc82e 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -363,6 +363,130 @@ error:
> return -EINVAL;
> }
>
> +/**
> + * smmu_ptw_64_s2 - VMSAv8-64 Walk of the page tables for a given IOVA
> + * for stage-2.
> + * @cfg: translation config
> + * @iova: iova to translate
> + * @perm: access type
> + * @tlbe: SMMUTLBEntry (out)
> + * @info: handle to an error info
> + *
> + * Return 0 on success, < 0 on error. In case of error, @info is filled
> + * and tlbe->perm is set to IOMMU_NONE.
> + * Upon success, @tlbe is filled with translated_addr and entry
> + * permission rights.
> + */
> +static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> + dma_addr_t iova, IOMMUAccessFlags perm,
Rename iova into ipa?
> + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> +{
> + const int stage = 2;
> + int granule_sz = cfg->s2cfg.granule_sz;
> + /* ARM ARM: Table D8-7. */
You may refer the full reference
in DDI0487I.a as the chapter/table may vary. For instance in
ARM DDI 0487F.c D8 corresponds to the activity monitor extensions
> + int inputsize = 64 - cfg->s2cfg.tsz;
> + int level = get_start_level(cfg->s2cfg.sl0, granule_sz);
> + int stride = SMMU_STRIDE(granule_sz);
> + int idx = pgd_idx(level, granule_sz, iova);
> + /*
> + * Get the ttb from concatenated structure.
> + * The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
> + */
what I don't get is the spec that concatenated tables are used if the
initial lookup level would require less or equal than 16 entries. I
don't see such kind of check or is done implicitly somewhere else?
> + uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> + idx * sizeof(uint64_t);
> + dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
> +
> + baseaddr &= ~indexmask;
> +
> + /*
> + * If the input address of a transaction exceeds the size of the IAS, a
> + * stage 1 Address Size fault occurs.
> + * For AA64, IAS = OAS
then you may use a local variable max_as = cfg->s2cfg.oas used below and
in
if (gpa >= (1ULL << cfg->s2cfg.oas)) {
this is not obvious though that the ias = oas. Where does it come from?
In implementations of SMMUv3.1 and later, this value is Reserved and S2PS behaves as 0b110 (52 bits).
Effective_S2PS = MIN(STE.S2PS, SMMU_IDR5.OAS);
OAS is a maximum of 52 bits in SMMUv3.1 and later
> + */
> + if (iova >= (1ULL << cfg->s2cfg.oas)) {
> + info->type = SMMU_PTW_ERR_ADDR_SIZE;
> + info->stage = 1;
> + goto error_no_stage;
> + }
> +
> + while (level < SMMU_LEVELS) {
I would rename SMMU_LEVELs
> + uint64_t subpage_size = 1ULL << level_shift(level, granule_sz);
> + uint64_t mask = subpage_size - 1;
> + uint32_t offset = iova_level_offset(iova, inputsize, level, granule_sz);
> + uint64_t pte, gpa;
> + dma_addr_t pte_addr = baseaddr + offset * sizeof(pte);
> + uint8_t ap;
> +
> + if (get_pte(baseaddr, offset, &pte, info)) {
> + goto error;
> + }
> + trace_smmu_ptw_level(stage, level, iova, subpage_size,
> + baseaddr, offset, pte);
> + if (is_invalid_pte(pte) || is_reserved_pte(pte, level)) {
> + trace_smmu_ptw_invalid_pte(stage, level, baseaddr,
> + pte_addr, offset, pte);
> + break;
> + }
> +
> + if (is_table_pte(pte, level)) {
> + baseaddr = get_table_pte_address(pte, granule_sz);
> + level++;
> + continue;
> + } else if (is_page_pte(pte, level)) {
> + gpa = get_page_pte_address(pte, granule_sz);
> + trace_smmu_ptw_page_pte(stage, level, iova,
> + baseaddr, pte_addr, pte, gpa);
> + } else {
> + uint64_t block_size;
> +
> + gpa = get_block_pte_address(pte, level, granule_sz,
> + &block_size);
> + trace_smmu_ptw_block_pte(stage, level, baseaddr,
> + pte_addr, pte, iova, gpa,
> + block_size >> 20);
> + }
> +
> + /*
> + * If S2AFFD and PTE.AF are 0 => fault. (5.2. Stream Table Entry)
> + * An Access fault takes priority over a Permission fault.
> + */
> + if (!PTE_AF(pte) && !cfg->s2cfg.affd) {
> + info->type = SMMU_PTW_ERR_ACCESS;
> + goto error;
> + }
> +
> + ap = PTE_AP(pte);
> + if (is_permission_fault_s2(ap, perm)) {
> + info->type = SMMU_PTW_ERR_PERMISSION;
> + goto error;
> + }
> +
> + /*
> + * The address output from the translation causes a stage 2 Address
> + * Size fault if it exceeds the effective PA output range.
> + */
> + if (gpa >= (1ULL << cfg->s2cfg.oas)) {
> + info->type = SMMU_PTW_ERR_ADDR_SIZE;
> + goto error;
> + }
> +
> + tlbe->entry.translated_addr = gpa;
> + tlbe->entry.iova = iova & ~mask;
> + tlbe->entry.addr_mask = mask;
> + tlbe->entry.perm = ap;
> + tlbe->level = level;
> + tlbe->granule = granule_sz;
> + return 0;
> + }
> + info->type = SMMU_PTW_ERR_TRANSLATION;
> +
> +error:
> + info->stage = 2;
> +error_no_stage:
> + tlbe->entry.perm = IOMMU_NONE;
> + return -EINVAL;
> +}
> +
> /**
> * smmu_ptw - Walk the page tables for an IOVA, according to @cfg
> *
> @@ -377,7 +501,13 @@ error:
> int smmu_ptw(SMMUTransCfg *cfg, dma_addr_t iova, IOMMUAccessFlags perm,
> SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> {
> - return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> + if (cfg->stage == 1) {
> + return smmu_ptw_64_s1(cfg, iova, perm, tlbe, info);
> + } else if (cfg->stage == 2) {
> + return smmu_ptw_64_s2(cfg, iova, perm, tlbe, info);
> + }
> +
> + g_assert_not_reached();
> }
>
> /**
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index 2d75b31953..f79c389cd3 100644
> --- a/hw/arm/smmu-internal.h
> +++ b/hw/arm/smmu-internal.h
> @@ -66,6 +66,8 @@
> #define PTE_APTABLE(pte) \
> (extract64(pte, 61, 2))
>
> +#define PTE_AF(pte) \
> + (extract64(pte, 10, 1))
> /*
> * TODO: At the moment all transactions are considered as privileged (EL1)
> * as IOMMU translation callback does not pass user/priv attributes.
> @@ -73,6 +75,9 @@
> #define is_permission_fault(ap, perm) \
> (((perm) & IOMMU_WO) && ((ap) & 0x2))
>
> +#define is_permission_fault_s2(ap, perm) \
I would rename ap param into s2ap. This is the name of the field in the spec
> + (!((ap & perm) == perm))
> +
> #define PTE_AP_TO_PERM(ap) \
> (IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
>
> @@ -96,6 +101,40 @@ uint64_t iova_level_offset(uint64_t iova, int inputsize,
> MAKE_64BIT_MASK(0, gsz - 3);
> }
>
> +#define SMMU_MAX_S2_CONCAT 16
it is not really an SMMU property (same as SMMU_LEVELS by the way). This
is rather something related to VMSA spec, no?.
> +
> +/*
> + * Relies on correctness of gran and sl0 from caller.
I would remove the above line as we generally expect the caller to
behave properly or do you mean we do not handle any sanity check?
I refer to some of them in target/arm/ptw.c:check_s2_mmu_setup()
> + * FEAT_LPA2 and FEAT_TTST are not implemented.
> + */
> +static inline int get_start_level(int sl0 , int gran)
> +{
> + /* ARM ARM: Table D8-12. */
> + if (gran == 12) {
> + return 2 - sl0;
> + }
> + /* ARM ARM: Table D8-22 and Table D8-31. */
> + return 3 - sl0;
> +}
> +
> +/*
> + * Index in a concatenated first level stage-2 page table.
> + * ARM ARM: D8.2.2 Concatenated translation tables.
> + */
> +static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
then the name of the function may better reflect what is does?
> +{
> + uint64_t ret;
> + /*
> + * Get the number of bits handled by next levels, then any extra bits in
> + * the address should index the concatenated tables. This relation can
> + * deduced from tables in ARM ARM: D8.2.7-9
> + */
> + int shift = (SMMU_LEVELS - start_level) * (granule - 3) + granule;
this looks like level_shift() with level = start_level - 1, no.
s/granule_sz/gsz or granule_sz to match the rest of the code
> +
> + ret = iova >> shift;
> + return ret;
> +}
> +
> #define SMMU_IOTLB_ASID(key) ((key).asid)
>
> typedef struct SMMUIOTLBPageInvInfo {
Thanks
Eric
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2
2023-03-20 14:56 ` Eric Auger
@ 2023-03-20 18:52 ` Mostafa Saleh
0 siblings, 0 replies; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-20 18:52 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Eric,
On Mon, Mar 20, 2023 at 03:56:26PM +0100, Eric Auger wrote:
> Hi Mostafa,
>
> On 2/26/23 23:06, Mostafa Saleh wrote:
> > In preparation for adding stage-2 support, add Stage-2 PTW code.
> > Only Aarch64 format is supported as stage-1.
> >
> > Nesting stage-1 and stage-2 is not supported right now.
> >
> > HTTU is not supported, SW is expected to maintain the Access flag.
> > This is described in the SMMUv3 manual "5.2. Stream Table Entry" in
> > "[181] S2AFFD".
> > This flag determines the behavior on access of a stage-2 page whose
> > descriptor has AF == 0:
> > - 0b0: An Access flag fault occurs (stall not supported).
> > - 0b1: An Access flag fault never occurs.
> > An Access fault takes priority over a Permission fault.
> >
> > Checks for IPA and output PA are done according to the user manual
> > "3.4 Address sizes".
> replace user manual by the exact ref of the doc. I guess this is IHI0070E
Will do.
> > + * Return 0 on success, < 0 on error. In case of error, @info is filled
> > + * and tlbe->perm is set to IOMMU_NONE.
> > + * Upon success, @tlbe is filled with translated_addr and entry
> > + * permission rights.
> > + */
> > +static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> > + dma_addr_t iova, IOMMUAccessFlags perm,
> Rename iova into ipa?
Will do.
> > + SMMUTLBEntry *tlbe, SMMUPTWEventInfo *info)
> > +{
> > + const int stage = 2;
> > + int granule_sz = cfg->s2cfg.granule_sz;
> > + /* ARM ARM: Table D8-7. */
> You may refer the full reference
> in DDI0487I.a as the chapter/table may vary. For instance in
> ARM DDI 0487F.c D8 corresponds to the activity monitor extensions
Will do.
> > + int inputsize = 64 - cfg->s2cfg.tsz;
> > + int level = get_start_level(cfg->s2cfg.sl0, granule_sz);
> > + int stride = SMMU_STRIDE(granule_sz);
> > + int idx = pgd_idx(level, granule_sz, iova);
> > + /*
> > + * Get the ttb from concatenated structure.
> > + * The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
> > + */
> what I don't get is the spec that concatenated tables are used if the
> initial lookup level would require less or equal than 16 entries. I
> don't see such kind of check or is done implicitly somewhere else?
Yes, this is checked in the STE patch in function
s2_pgtable_config_valid, where it checks that the max input will not
need more than 16 tables which means that the pagetable config is
inconsistent, which means the STE is ILLEGAL.
> > + uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> > + idx * sizeof(uint64_t);
> > + dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
> > +
> > + baseaddr &= ~indexmask;
> > +
> > + /*
> > + * If the input address of a transaction exceeds the size of the IAS, a
> > + * stage 1 Address Size fault occurs.
> > + * For AA64, IAS = OAS
> then you may use a local variable max_as = cfg->s2cfg.oas used below and
> in
>
> if (gpa >= (1ULL << cfg->s2cfg.oas)) {
> this is not obvious though that the ias = oas. Where does it come from?
>
> In implementations of SMMUv3.1 and later, this value is Reserved and S2PS behaves as 0b110 (52 bits).
> Effective_S2PS = MIN(STE.S2PS, SMMU_IDR5.OAS);
> OAS is a maximum of 52 bits in SMMUv3.1 and later
IAS = OAS for AA64. Described in "3.4 Address sizes".
The first check is actually not correct, as input address should be
compared to IAS(or OAS) while s2cfg.oas is effective PS.
I will rename s2cfg.oas to s2cfg.eff_ps to avoid confusion.
I will change the check here to be against s2t0sz and in this case it
causes stage-2 addr size fault.
I think the check for the IAS shouldn't be done here.
> > + */
> > + if (iova >= (1ULL << cfg->s2cfg.oas)) {
> > + info->type = SMMU_PTW_ERR_ADDR_SIZE;
> > + info->stage = 1;
> > + goto error_no_stage;
> > + }
> > +
> > + while (level < SMMU_LEVELS) {
> I would rename SMMU_LEVELs
You mean replace SMMU_LEVELS with SMMU_LEVELs?
> >
> > +#define PTE_AF(pte) \
> > + (extract64(pte, 10, 1))
> > /*
> > * TODO: At the moment all transactions are considered as privileged (EL1)
> > * as IOMMU translation callback does not pass user/priv attributes.
> > @@ -73,6 +75,9 @@
> > #define is_permission_fault(ap, perm) \
> > (((perm) & IOMMU_WO) && ((ap) & 0x2))
> >
> > +#define is_permission_fault_s2(ap, perm) \
> I would rename ap param into s2ap. This is the name of the field in the spec
Will do.
> > + (!((ap & perm) == perm))
> > +
> > #define PTE_AP_TO_PERM(ap) \
> > (IOMMU_ACCESS_FLAG(true, !((ap) & 0x2)))
> >
> > @@ -96,6 +101,40 @@ uint64_t iova_level_offset(uint64_t iova, int inputsize,
> > MAKE_64BIT_MASK(0, gsz - 3);
> > }
> >
> > +#define SMMU_MAX_S2_CONCAT 16
> it is not really an SMMU property (same as SMMU_LEVELS by the way). This
> is rather something related to VMSA spec, no?.
Yes, they are part of VMSA, however they are named this way as they are
part of SMMU headers, should we rename them to something else?
> > +
> > +/*
> > + * Relies on correctness of gran and sl0 from caller.
> I would remove the above line as we generally expect the caller to
> behave properly or do you mean we do not handle any sanity check?
> I refer to some of them in target/arm/ptw.c:check_s2_mmu_setup()
We do sanity check in STE parsing, I added this line as this function
is in a header file and anyone can use it, so to make it clear.
However, it is very trivial, I will remove the comment.
> > + * FEAT_LPA2 and FEAT_TTST are not implemented.
> > + */
> > +static inline int get_start_level(int sl0 , int gran)
> > +{
> > + /* ARM ARM: Table D8-12. */
> > + if (gran == 12) {
> > + return 2 - sl0;
> > + }
> > + /* ARM ARM: Table D8-22 and Table D8-31. */
> > + return 3 - sl0;
> > +}
> > +
> > +/*
> > + * Index in a concatenated first level stage-2 page table.
> > + * ARM ARM: D8.2.2 Concatenated translation tables.
> > + */
> > +static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
> then the name of the function may better reflect what is does?
This returns the index of the page table descriptor in a concatenated
structure, this is actually close to what kvm calls it
“kvm_pgd_page_idx()”, however, I can call it something more clear as
pgd_concatenated_idx()?
> > +{
> > + uint64_t ret;
> > + /*
> > + * Get the number of bits handled by next levels, then any extra bits in
> > + * the address should index the concatenated tables. This relation can
> > + * deduced from tables in ARM ARM: D8.2.7-9
> > + */
> > + int shift = (SMMU_LEVELS - start_level) * (granule - 3) + granule;
> this looks like level_shift() with level = start_level - 1, no.
Yes, I will use level_shift() instead.
> s/granule_sz/gsz or granule_sz to match the rest of the code
Will do.
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 05/11] hw/arm/smmuv3: Parse STE config for stage-2
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
` (3 preceding siblings ...)
2023-02-26 22:06 ` [RFC PATCH v2 04/11] hw/arm/smmuv3: Add page table walk for stage-2 Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-20 15:14 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 06/11] hw/arm/smmuv3: Make TLB lookup work " Mostafa Saleh
` (5 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
Parse stage-2 configuration from STE and populate it in SMMUS2Cfg.
Validity of these value are checked when possible.
Only AA64 tables are supported and STT is not supported.
According to SMMUv3 user manual "5.2 Stream Table Entry": All fields
with an S2 prefix (with the exception of S2VMID) are IGNORED when
stage-2 bypasses translation (Config[1] == 0).
Which means that VMID can be used(for TLB tagging) even if stage-2 is
bypassed, so we parse it unconditionally when S2P exists. Otherwise
it is set to -1.(only S1P)
As stall is not supported, if S2S is set the translation would abort.
For S2R, we reuse the same code used for stage-1 with flag
record_faults. However when nested translation is supported we would
need to separate stage-1 and stage-2 faults.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in V2:
- Parse S2PS and S2ENDI
- Squash with S2VMID parsing patch
- Squash with S2AFF parsing
- Squash with fault reporting patch
- Add check for S2T0SZ
- Renaming and refactoring code
---
hw/arm/smmuv3-internal.h | 4 ++
hw/arm/smmuv3.c | 138 +++++++++++++++++++++++++++++++++++++++
include/hw/arm/smmuv3.h | 3 +
3 files changed, 145 insertions(+)
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 183d5ac8dc..3388e1a5f8 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -526,9 +526,13 @@ typedef struct CD {
#define STE_S2TG(x) extract32((x)->word[5], 14, 2)
#define STE_S2PS(x) extract32((x)->word[5], 16, 3)
#define STE_S2AA64(x) extract32((x)->word[5], 19, 1)
+#define STE_S2ENDI(x) extract32((x)->word[5], 20, 1)
+#define STE_S2AFFD(x) extract32((x)->word[5], 21, 1)
#define STE_S2HD(x) extract32((x)->word[5], 24, 1)
#define STE_S2HA(x) extract32((x)->word[5], 25, 1)
#define STE_S2S(x) extract32((x)->word[5], 26, 1)
+#define STE_S2R(x) extract32((x)->word[5], 27, 1)
+
#define STE_CTXPTR(x) \
({ \
unsigned long addr; \
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 4e90343996..dc74a5442d 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -329,6 +329,46 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
return 0;
}
+/*
+ * Max valid value is 39 when SMMU_IDR3.STT == 0.
+ * In architectures after SMMUv3.0:
+ * - If STE.S2TG selects a 4KB or 16KB granule, the minimum valid value for this
+ * field is MAX(16, 64-IAS)
+ * - If STE.S2TG selects a 64KB granule, the minimum valid value for this field
+ * is (64-IAS).
+ * As we only support AA64, IAS = OAS.
+ */
+static bool t0sz_valid(SMMUTransCfg *cfg)
+{
+ if (cfg->s2cfg.tsz > 39) {
+ return false;
+ }
+
+ if (cfg->s2cfg.granule_sz == 16) {
+ return (cfg->s2cfg.tsz >= 64 - cfg->s2cfg.oas);
+ }
+
+ return (cfg->s2cfg.tsz >= MAX(64 - cfg->s2cfg.oas, 16));
+}
+
+/*
+ * Return true if s2 page table config is valid.
+ * This checks with the configured start level, ias_bits and granularity we can
+ * have a valid page table as described in ARM ARM D8.2 Translation process.
+ * The idea here is to see for the highest possible number of IPA bits, how
+ * many concatenated tables we would need, if it is more than 16, then this is
+ * not possible.
+ */
+static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
+{
+ int level = get_start_level(sl0, gran);
+ uint64_t ipa_bits = 64 - t0sz;
+ uint64_t max_ipa = (1ULL << ipa_bits) - 1;
+ int nr_concat = pgd_idx(level, gran, max_ipa) + 1;
+
+ return nr_concat <= SMMU_MAX_S2_CONCAT;
+}
+
/* Returns < 0 in case of invalid STE, 0 otherwise */
static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
STE *ste, SMMUEventInfo *event)
@@ -354,7 +394,105 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
return 0;
}
+ /*
+ * If a stage is enabled in SW while not advertised, throw bad ste
+ * according to SMMU manual 5.2 Stream Table Entry - [3:1] Config.
+ */
+ if (!STAGE1_SUPPORTED(s) && STE_CFG_S1_ENABLED(config)) {
+ qemu_log_mask(LOG_GUEST_ERROR, "SMMUv3 S1 used but not supported.\n");
+ goto bad_ste;
+ }
+ if (!STAGE2_SUPPORTED(s) && STE_CFG_S2_ENABLED(config)) {
+ qemu_log_mask(LOG_GUEST_ERROR, "SMMUv3 S2 used but not supported.\n");
+ goto bad_ste;
+ }
+
+ if (STAGE2_SUPPORTED(s)) {
+ /* VMID is considered even if s2 is disabled. */
+ cfg->s2cfg.vmid = STE_S2VMID(ste);
+ } else {
+ /* Default to -1 */
+ cfg->s2cfg.vmid = -1;
+ }
+
if (STE_CFG_S2_ENABLED(config)) {
+ cfg->stage = 2;
+
+ if (STE_S2AA64(ste) == 0x0) {
+ qemu_log_mask(LOG_UNIMP,
+ "SMMUv3 AArch32 tables not supported\n");
+ g_assert_not_reached();
+ }
+
+ switch (STE_S2TG(ste)) {
+ case 0x0: /* 4KB */
+ cfg->s2cfg.granule_sz = 12;
+ break;
+ case 0x1: /* 64KB */
+ cfg->s2cfg.granule_sz = 16;
+ break;
+ case 0x2: /* 16KB */
+ cfg->s2cfg.granule_sz = 14;
+ break;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "SMMUv3 bad STE S2TG: %x\n", STE_S2TG(ste));
+ goto bad_ste;
+ }
+
+ cfg->s2cfg.vttb = STE_S2TTB(ste);
+
+ cfg->s2cfg.sl0 = STE_S2SL0(ste);
+ /* FEAT_TTST not supported. */
+ if (cfg->s2cfg.sl0 == 0x3) {
+ qemu_log_mask(LOG_UNIMP, "SMMUv3 S2SL0 = 0x3 has no meaning!\n");
+ goto bad_ste;
+ }
+
+
+ cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
+ /*
+ * It is ILLEGAL for the address in S2TTB to be outside the range
+ * described by the effective S2PS value.
+ */
+ if (cfg->s2cfg.vttb & ~(MAKE_64BIT_MASK(0, cfg->s2cfg.oas))) {
+ goto bad_ste;
+ }
+
+ cfg->s2cfg.tsz = STE_S2T0SZ(ste);
+
+ if (!t0sz_valid(cfg)) {
+ qemu_log_mask(LOG_GUEST_ERROR, "SMMUv3 bad STE S2T0SZ = %d\n",
+ cfg->s2cfg.tsz);
+ goto bad_ste;
+ }
+
+ if (!s2_pgtable_config_valid(cfg->s2cfg.sl0, cfg->s2cfg.tsz,
+ cfg->s2cfg.granule_sz)) {
+ qemu_log_mask(LOG_GUEST_ERROR,
+ "SMMUv3 STE stage 2 config not valid!\n");
+ goto bad_ste;
+ }
+
+ /* Only LE supported(IDR0.TTENDIAN). */
+ if (STE_S2ENDI(ste)) {
+ goto bad_ste;
+ }
+
+ cfg->s2cfg.affd = STE_S2AFFD(ste);
+ /*
+ * We reuse the same code used for stage-1 with flag record_faults.
+ * However when nested translation is supported we would
+ * need to separate stage-1 and stage-2 faults.
+ */
+ cfg->record_faults = STE_S2R(ste);
+ /* As stall is not supported. */
+ if (STE_S2S(ste)) {
+ qemu_log_mask(LOG_UNIMP, "SMMUv3 Stall not implemented!\n");
+ goto bad_ste;
+ }
+
+ /* This is still here as stage 2 has not been fully enabled yet. */
qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
goto bad_ste;
}
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index a0c026402e..6031d7d325 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -83,4 +83,7 @@ struct SMMUv3Class {
#define TYPE_ARM_SMMUV3 "arm-smmuv3"
OBJECT_DECLARE_TYPE(SMMUv3State, SMMUv3Class, ARM_SMMUV3)
+#define STAGE1_SUPPORTED(s) FIELD_EX32(s->idr[0], IDR0, S1P)
+#define STAGE2_SUPPORTED(s) FIELD_EX32(s->idr[0], IDR0, S2P)
+
#endif
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 05/11] hw/arm/smmuv3: Parse STE config for stage-2
2023-02-26 22:06 ` [RFC PATCH v2 05/11] hw/arm/smmuv3: Parse STE config " Mostafa Saleh
@ 2023-03-20 15:14 ` Eric Auger
2023-03-20 19:11 ` Mostafa Saleh
0 siblings, 1 reply; 37+ messages in thread
From: Eric Auger @ 2023-03-20 15:14 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi Mostafa,
On 2/26/23 23:06, Mostafa Saleh wrote:
> Parse stage-2 configuration from STE and populate it in SMMUS2Cfg.
> Validity of these value are checked when possible.
s/these value/field values
>
> Only AA64 tables are supported and STT is not supported.
Small Translation Tables (STT)
>
> According to SMMUv3 user manual "5.2 Stream Table Entry": All fields
it is not a user manual but rather an IP architecture spec. put the full
ref?
> with an S2 prefix (with the exception of S2VMID) are IGNORED when
> stage-2 bypasses translation (Config[1] == 0).
>
> Which means that VMID can be used(for TLB tagging) even if stage-2 is
> bypassed, so we parse it unconditionally when S2P exists. Otherwise
> it is set to -1.(only S1P)
>
> As stall is not supported, if S2S is set the translation would abort.
> For S2R, we reuse the same code used for stage-1 with flag
> record_faults. However when nested translation is supported we would
> need to separate stage-1 and stage-2 faults.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in V2:
> - Parse S2PS and S2ENDI
> - Squash with S2VMID parsing patch
> - Squash with S2AFF parsing
> - Squash with fault reporting patch
> - Add check for S2T0SZ
> - Renaming and refactoring code
> ---
> hw/arm/smmuv3-internal.h | 4 ++
> hw/arm/smmuv3.c | 138 +++++++++++++++++++++++++++++++++++++++
> include/hw/arm/smmuv3.h | 3 +
> 3 files changed, 145 insertions(+)
>
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 183d5ac8dc..3388e1a5f8 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -526,9 +526,13 @@ typedef struct CD {
> #define STE_S2TG(x) extract32((x)->word[5], 14, 2)
> #define STE_S2PS(x) extract32((x)->word[5], 16, 3)
> #define STE_S2AA64(x) extract32((x)->word[5], 19, 1)
> +#define STE_S2ENDI(x) extract32((x)->word[5], 20, 1)
> +#define STE_S2AFFD(x) extract32((x)->word[5], 21, 1)
I am afraid there is a shift in the fields below. S2HD should be 23
(problem in the original code) and everything is shifted.
> #define STE_S2HD(x) extract32((x)->word[5], 24, 1)
> #define STE_S2HA(x) extract32((x)->word[5], 25, 1)
> #define STE_S2S(x) extract32((x)->word[5], 26, 1)
> +#define STE_S2R(x) extract32((x)->word[5], 27, 1)
> +
> #define STE_CTXPTR(x) \
> ({ \
> unsigned long addr; \
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 4e90343996..dc74a5442d 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -329,6 +329,46 @@ static int smmu_get_cd(SMMUv3State *s, STE *ste, uint32_t ssid,
> return 0;
> }
>
> +/*
> + * Max valid value is 39 when SMMU_IDR3.STT == 0.
> + * In architectures after SMMUv3.0:
> + * - If STE.S2TG selects a 4KB or 16KB granule, the minimum valid value for this
> + * field is MAX(16, 64-IAS)
> + * - If STE.S2TG selects a 64KB granule, the minimum valid value for this field
> + * is (64-IAS).
> + * As we only support AA64, IAS = OAS.
OK this comes from STE.S2T0SZ description on the SMMU arch spec. You can
add this in previous patch too.
> + */
> +static bool t0sz_valid(SMMUTransCfg *cfg)
use S2t0sz to avoid confusion with S1 stuff
> +{
> + if (cfg->s2cfg.tsz > 39) {
> + return false;
> + }
> +
> + if (cfg->s2cfg.granule_sz == 16) {
> + return (cfg->s2cfg.tsz >= 64 - cfg->s2cfg.oas);
> + }
> +
> + return (cfg->s2cfg.tsz >= MAX(64 - cfg->s2cfg.oas, 16));
> +}
this chapter also states:
"The usable range of values is further constrained by a function of the
starting level set by S2SL0 and, if S2AA64 == 1, granule size set by
S2TG as described by the Armv8 translation system. Use of a value of
S2T0SZ that is inconsistent with the permitted range (given S2SL0 and
S2TG) is ILLEGAL."
> +
> +/*
> + * Return true if s2 page table config is valid.
> + * This checks with the configured start level, ias_bits and granularity we can
> + * have a valid page table as described in ARM ARM D8.2 Translation process.
> + * The idea here is to see for the highest possible number of IPA bits, how
> + * many concatenated tables we would need, if it is more than 16, then this is
> + * not possible.
why? in that case doesn't it mean that we can't use concatanated tables?
> + */
> +static bool s2_pgtable_config_valid(uint8_t sl0, uint8_t t0sz, uint8_t gran)
> +{
> + int level = get_start_level(sl0, gran);
> + uint64_t ipa_bits = 64 - t0sz;
> + uint64_t max_ipa = (1ULL << ipa_bits) - 1;
> + int nr_concat = pgd_idx(level, gran, max_ipa) + 1;
> +
> + return nr_concat <= SMMU_MAX_S2_CONCAT;
> +}
> +
> /* Returns < 0 in case of invalid STE, 0 otherwise */
> static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> STE *ste, SMMUEventInfo *event)
> @@ -354,7 +394,105 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> return 0;
> }
>
> + /*
> + * If a stage is enabled in SW while not advertised, throw bad ste
> + * according to SMMU manual 5.2 Stream Table Entry - [3:1] Config.
> + */
> + if (!STAGE1_SUPPORTED(s) && STE_CFG_S1_ENABLED(config)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "SMMUv3 S1 used but not supported.\n");
> + goto bad_ste;
> + }
> + if (!STAGE2_SUPPORTED(s) && STE_CFG_S2_ENABLED(config)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "SMMUv3 S2 used but not supported.\n");
> + goto bad_ste;
> + }
> +
> + if (STAGE2_SUPPORTED(s)) {
> + /* VMID is considered even if s2 is disabled. */
> + cfg->s2cfg.vmid = STE_S2VMID(ste);
> + } else {
> + /* Default to -1 */
> + cfg->s2cfg.vmid = -1;
> + }
> +
> if (STE_CFG_S2_ENABLED(config)) {
I think it would improve the readability to introduce a separate
function decode_ste_s2_cftg() taking the s2cfg to be populated
> + cfg->stage = 2;
> +
> + if (STE_S2AA64(ste) == 0x0) {
> + qemu_log_mask(LOG_UNIMP,
> + "SMMUv3 AArch32 tables not supported\n");
> + g_assert_not_reached();
> + }
> +
> + switch (STE_S2TG(ste)) {
> + case 0x0: /* 4KB */
> + cfg->s2cfg.granule_sz = 12;
> + break;
> + case 0x1: /* 64KB */
> + cfg->s2cfg.granule_sz = 16;
> + break;
> + case 0x2: /* 16KB */
> + cfg->s2cfg.granule_sz = 14;
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "SMMUv3 bad STE S2TG: %x\n", STE_S2TG(ste));
> + goto bad_ste;
> + }
> +
> + cfg->s2cfg.vttb = STE_S2TTB(ste);
> +
> + cfg->s2cfg.sl0 = STE_S2SL0(ste);
> + /* FEAT_TTST not supported. */
> + if (cfg->s2cfg.sl0 == 0x3) {
> + qemu_log_mask(LOG_UNIMP, "SMMUv3 S2SL0 = 0x3 has no meaning!\n");
> + goto bad_ste;
> + }
> +
> +
> + cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
> + /*
> + * It is ILLEGAL for the address in S2TTB to be outside the range
> + * described by the effective S2PS value.
> + */
> + if (cfg->s2cfg.vttb & ~(MAKE_64BIT_MASK(0, cfg->s2cfg.oas))) {
> + goto bad_ste;
> + }
> +
> + cfg->s2cfg.tsz = STE_S2T0SZ(ste);
> +
> + if (!t0sz_valid(cfg)) {
> + qemu_log_mask(LOG_GUEST_ERROR, "SMMUv3 bad STE S2T0SZ = %d\n",
> + cfg->s2cfg.tsz);
> + goto bad_ste;
> + }
> +
> + if (!s2_pgtable_config_valid(cfg->s2cfg.sl0, cfg->s2cfg.tsz,
> + cfg->s2cfg.granule_sz)) {
> + qemu_log_mask(LOG_GUEST_ERROR,
> + "SMMUv3 STE stage 2 config not valid!\n");
> + goto bad_ste;
> + }
> +
> + /* Only LE supported(IDR0.TTENDIAN). */
> + if (STE_S2ENDI(ste)) {
qemu_log
> + goto bad_ste;
> + }
> +
> + cfg->s2cfg.affd = STE_S2AFFD(ste);
> + /*
> + * We reuse the same code used for stage-1 with flag record_faults.
> + * However when nested translation is supported we would
> + * need to separate stage-1 and stage-2 faults.
> + */
> + cfg->record_faults = STE_S2R(ste);
> + /* As stall is not supported. */
> + if (STE_S2S(ste)) {
> + qemu_log_mask(LOG_UNIMP, "SMMUv3 Stall not implemented!\n");
> + goto bad_ste;
> + }
> +
> + /* This is still here as stage 2 has not been fully enabled yet. */
> qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
> goto bad_ste;
> }
> diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
> index a0c026402e..6031d7d325 100644
> --- a/include/hw/arm/smmuv3.h
> +++ b/include/hw/arm/smmuv3.h
> @@ -83,4 +83,7 @@ struct SMMUv3Class {
> #define TYPE_ARM_SMMUV3 "arm-smmuv3"
> OBJECT_DECLARE_TYPE(SMMUv3State, SMMUv3Class, ARM_SMMUV3)
>
> +#define STAGE1_SUPPORTED(s) FIELD_EX32(s->idr[0], IDR0, S1P)
> +#define STAGE2_SUPPORTED(s) FIELD_EX32(s->idr[0], IDR0, S2P)
> +
> #endif
Thanks
Eric
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 05/11] hw/arm/smmuv3: Parse STE config for stage-2
2023-03-20 15:14 ` Eric Auger
@ 2023-03-20 19:11 ` Mostafa Saleh
0 siblings, 0 replies; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-20 19:11 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Eric,
On Mon, Mar 20, 2023 at 04:14:48PM +0100, Eric Auger wrote:
> Hi Mostafa,
>
> On 2/26/23 23:06, Mostafa Saleh wrote:
> > Parse stage-2 configuration from STE and populate it in SMMUS2Cfg.
> > Validity of these value are checked when possible.
> s/these value/field values
Will do.
> >
> > Only AA64 tables are supported and STT is not supported.
> Small Translation Tables (STT)
Will do.
> >
> > According to SMMUv3 user manual "5.2 Stream Table Entry": All fields
> it is not a user manual but rather an IP architecture spec. put the full
> ref?
This is mentioned in the SMMU manual with the same wording “All fields
with an S2 prefix (with the exception of S2VMID) are IGNORED when stage
2 bypasses translation (Config[1] == 0)” in “5.2 Stream Table Entry”
in page 179, ARM IHI0070.E.a
> > 3 files changed, 145 insertions(+)
> >
> > diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> > index 183d5ac8dc..3388e1a5f8 100644
> > --- a/hw/arm/smmuv3-internal.h
> > +++ b/hw/arm/smmuv3-internal.h
> > @@ -526,9 +526,13 @@ typedef struct CD {
> > #define STE_S2TG(x) extract32((x)->word[5], 14, 2)
> > #define STE_S2PS(x) extract32((x)->word[5], 16, 3)
> > #define STE_S2AA64(x) extract32((x)->word[5], 19, 1)
> > +#define STE_S2ENDI(x) extract32((x)->word[5], 20, 1)
> > +#define STE_S2AFFD(x) extract32((x)->word[5], 21, 1)
> I am afraid there is a shift in the fields below. S2HD should be 23
> (problem in the original code) and everything is shifted.
Oh, yes, I should have checked that, I just relied they are next to it.
I will fix them.
> > +/*
> > + * Max valid value is 39 when SMMU_IDR3.STT == 0.
> > + * In architectures after SMMUv3.0:
> > + * - If STE.S2TG selects a 4KB or 16KB granule, the minimum valid value for this
> > + * field is MAX(16, 64-IAS)
> > + * - If STE.S2TG selects a 64KB granule, the minimum valid value for this field
> > + * is (64-IAS).
> > + * As we only support AA64, IAS = OAS.
> OK this comes from STE.S2T0SZ description on the SMMU arch spec. You can
> add this in previous patch too.
> > + */
> > +static bool t0sz_valid(SMMUTransCfg *cfg)
> use S2t0sz to avoid confusion with S1 stuff
Will do.
> > +{
> > + if (cfg->s2cfg.tsz > 39) {
> > + return false;
> > + }
> > +
> > + if (cfg->s2cfg.granule_sz == 16) {
> > + return (cfg->s2cfg.tsz >= 64 - cfg->s2cfg.oas);
> > + }
> > +
> > + return (cfg->s2cfg.tsz >= MAX(64 - cfg->s2cfg.oas, 16));
> > +}
>
> this chapter also states:
> "The usable range of values is further constrained by a function of the
> starting level set by S2SL0 and, if S2AA64 == 1, granule size set by
> S2TG as described by the Armv8 translation system. Use of a value of
> S2T0SZ that is inconsistent with the permitted range (given S2SL0 and
> S2TG) is ILLEGAL."
Yes, my understanding is that with some configurations the values of
S2SL0 and S2T0SZ are correct but the final configuration will lead to
input address that needs more than 16 concatenated tables which is
invalid, this is checked in s2_pgtable_config_valid()
For example:
A configuration with granularity 4K (granule_sz = 12)
SL0 = 1 (start level = 1)
S2T0SZ = 16 (48 bits)
This means that the 3 levels would cover 3*9 = 27 + 12 = 39 bits
So there are extra 48-39 = 9 bits which requires 512 concatenated
tables. This is invalid.
> > +
> > +/*
> > + * Return true if s2 page table config is valid.
> > + * This checks with the configured start level, ias_bits and granularity we can
> > + * have a valid page table as described in ARM ARM D8.2 Translation process.
> > + * The idea here is to see for the highest possible number of IPA bits, how
> > + * many concatenated tables we would need, if it is more than 16, then this is
> > + * not possible.
> why? in that case doesn't it mean that we can't use concatanated tables?
Yes, that means that we would need more than 16 tables, as mentioned
above this is illegal.
> > + goto bad_ste;
> > + }
> > +
> > + if (STAGE2_SUPPORTED(s)) {
> > + /* VMID is considered even if s2 is disabled. */
> > + cfg->s2cfg.vmid = STE_S2VMID(ste);
> > + } else {
> > + /* Default to -1 */
> > + cfg->s2cfg.vmid = -1;
> > + }
> > +
> > if (STE_CFG_S2_ENABLED(config)) {
> I think it would improve the readability to introduce a separate
> function decode_ste_s2_cftg() taking the s2cfg to be populated
Yes, will do.
> > +
> > + if (!s2_pgtable_config_valid(cfg->s2cfg.sl0, cfg->s2cfg.tsz,
> > + cfg->s2cfg.granule_sz)) {
> > + qemu_log_mask(LOG_GUEST_ERROR,
> > + "SMMUv3 STE stage 2 config not valid!\n");
> > + goto bad_ste;
> > + }
> > +
> > + /* Only LE supported(IDR0.TTENDIAN). */
> > + if (STE_S2ENDI(ste)) {
> qemu_log
Will do.
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 06/11] hw/arm/smmuv3: Make TLB lookup work for stage-2
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
` (4 preceding siblings ...)
2023-02-26 22:06 ` [RFC PATCH v2 05/11] hw/arm/smmuv3: Parse STE config " Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-20 16:05 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 07/11] hw/arm/smmuv3: Add VMID to tlb tagging Mostafa Saleh
` (4 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
Right now, either stage-1 or stage-2 are supported, this simplifies
how we can deal with TLBs.
This patch makes TLB lookup work if stage-2 is enabled instead of
stage-1.
TLB lookup is done before a PTW, if a valid entry is found we won't
do the PTW.
To be able to do TLB lookup, we need the correct tagging info, as
granularity and input size, so we get this based on the supported
translation stage. The TLB entries are added correctly from each
stage PTW.
When nested translation is supported, this would need to change, for
example if we go with a combined TLB implementation, we would need to
use the min of the granularities in TLB.
As stage-2 shouldn't be tagged by ASID, it will be set to -1 if S1P
is not enabled.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v2:
- check if S1 is enabled(not supported) when reading S1 TT.
---
hw/arm/smmuv3.c | 45 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index dc74a5442d..ce193e9598 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -697,6 +697,9 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
STE ste;
CD cd;
+ /* ASID defaults to -1 (if s1 is not supported). */
+ cfg->asid = -1;
+
ret = smmu_find_ste(s, sid, &ste, event);
if (ret) {
return ret;
@@ -787,6 +790,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
SMMUTLBEntry *cached_entry = NULL;
SMMUTransTableInfo *tt;
SMMUTransCfg *cfg = NULL;
+ uint8_t granule_sz, tsz;
IOMMUTLBEntry entry = {
.target_as = &address_space_memory,
.iova = addr,
@@ -822,21 +826,40 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
goto epilogue;
}
- tt = select_tt(cfg, addr);
- if (!tt) {
- if (cfg->record_faults) {
- event.type = SMMU_EVT_F_TRANSLATION;
- event.u.f_translation.addr = addr;
- event.u.f_translation.rnw = flag & 0x1;
+ if (cfg->stage == 1) {
+ /* Select stage1 translation table. */
+ tt = select_tt(cfg, addr);
+ if (!tt) {
+ if (cfg->record_faults) {
+ event.type = SMMU_EVT_F_TRANSLATION;
+ event.u.f_translation.addr = addr;
+ event.u.f_translation.rnw = flag & 0x1;
+ }
+ status = SMMU_TRANS_ERROR;
+ goto epilogue;
}
- status = SMMU_TRANS_ERROR;
- goto epilogue;
- }
+ granule_sz = tt->granule_sz;
+ tsz = tt->tsz;
- page_mask = (1ULL << (tt->granule_sz)) - 1;
+ } else {
+ /* Stage2. */
+ granule_sz = cfg->s2cfg.granule_sz;
+ tsz = cfg->s2cfg.tsz;
+ }
+ /*
+ * TLB lookup looks for granule and input size for a translation stage,
+ * as only one stage is supported right now, choose the right values
+ * from the configuration.
+ */
+ page_mask = (1ULL << granule_sz) - 1;
aligned_addr = addr & ~page_mask;
- cached_entry = smmu_iotlb_lookup(bs, cfg, tt, aligned_addr);
+ SMMUTransTableInfo temp = {
+ .granule_sz = granule_sz,
+ .tsz = tsz,
+ };
+
+ cached_entry = smmu_iotlb_lookup(bs, cfg, &temp, aligned_addr);
if (cached_entry) {
if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
status = SMMU_TRANS_ERROR;
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 06/11] hw/arm/smmuv3: Make TLB lookup work for stage-2
2023-02-26 22:06 ` [RFC PATCH v2 06/11] hw/arm/smmuv3: Make TLB lookup work " Mostafa Saleh
@ 2023-03-20 16:05 ` Eric Auger
2023-03-20 19:14 ` Mostafa Saleh
0 siblings, 1 reply; 37+ messages in thread
From: Eric Auger @ 2023-03-20 16:05 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi Mostafa,
On 2/26/23 23:06, Mostafa Saleh wrote:
> Right now, either stage-1 or stage-2 are supported, this simplifies
> how we can deal with TLBs.
> This patch makes TLB lookup work if stage-2 is enabled instead of
> stage-1.
> TLB lookup is done before a PTW, if a valid entry is found we won't
> do the PTW.
> To be able to do TLB lookup, we need the correct tagging info, as
> granularity and input size, so we get this based on the supported
> translation stage. The TLB entries are added correctly from each
> stage PTW.
>
> When nested translation is supported, this would need to change, for
> example if we go with a combined TLB implementation, we would need to
> use the min of the granularities in TLB.
>
> As stage-2 shouldn't be tagged by ASID, it will be set to -1 if S1P
> is not enabled.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in v2:
> - check if S1 is enabled(not supported) when reading S1 TT.
> ---
> hw/arm/smmuv3.c | 45 ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index dc74a5442d..ce193e9598 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -697,6 +697,9 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
> STE ste;
> CD cd;
>
> + /* ASID defaults to -1 (if s1 is not supported). */
> + cfg->asid = -1;
> +
> ret = smmu_find_ste(s, sid, &ste, event);
> if (ret) {
> return ret;
> @@ -787,6 +790,7 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> SMMUTLBEntry *cached_entry = NULL;
> SMMUTransTableInfo *tt;
> SMMUTransCfg *cfg = NULL;
> + uint8_t granule_sz, tsz;
> IOMMUTLBEntry entry = {
> .target_as = &address_space_memory,
> .iova = addr,
> @@ -822,21 +826,40 @@ static IOMMUTLBEntry smmuv3_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> goto epilogue;
> }
>
> - tt = select_tt(cfg, addr);
> - if (!tt) {
> - if (cfg->record_faults) {
> - event.type = SMMU_EVT_F_TRANSLATION;
> - event.u.f_translation.addr = addr;
> - event.u.f_translation.rnw = flag & 0x1;
> + if (cfg->stage == 1) {
> + /* Select stage1 translation table. */
> + tt = select_tt(cfg, addr);
> + if (!tt) {
> + if (cfg->record_faults) {
> + event.type = SMMU_EVT_F_TRANSLATION;
> + event.u.f_translation.addr = addr;
> + event.u.f_translation.rnw = flag & 0x1;
> + }
> + status = SMMU_TRANS_ERROR;
> + goto epilogue;
> }
> - status = SMMU_TRANS_ERROR;
> - goto epilogue;
> - }
> + granule_sz = tt->granule_sz;
> + tsz = tt->tsz;
>
> - page_mask = (1ULL << (tt->granule_sz)) - 1;
> + } else {
> + /* Stage2. */
> + granule_sz = cfg->s2cfg.granule_sz;
> + tsz = cfg->s2cfg.tsz;
> + }
> + /*
> + * TLB lookup looks for granule and input size for a translation stage,
> + * as only one stage is supported right now, choose the right values
> + * from the configuration.
> + */
> + page_mask = (1ULL << granule_sz) - 1;
> aligned_addr = addr & ~page_mask;
>
> - cached_entry = smmu_iotlb_lookup(bs, cfg, tt, aligned_addr);
> + SMMUTransTableInfo temp = {
Move the declaration at the top. Also rename temp into tt to be more
explicit about what it is?
> + .granule_sz = granule_sz,
> + .tsz = tsz,
> + };
> +
> + cached_entry = smmu_iotlb_lookup(bs, cfg, &temp, aligned_addr);
> if (cached_entry) {
> if ((flag & IOMMU_WO) && !(cached_entry->entry.perm & IOMMU_WO)) {
> status = SMMU_TRANS_ERROR;
Besides, looks good to me
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 06/11] hw/arm/smmuv3: Make TLB lookup work for stage-2
2023-03-20 16:05 ` Eric Auger
@ 2023-03-20 19:14 ` Mostafa Saleh
0 siblings, 0 replies; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-20 19:14 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Eric,
On Mon, Mar 20, 2023 at 05:05:31PM +0100, Eric Auger wrote:
> > + /*
> > + * TLB lookup looks for granule and input size for a translation stage,
> > + * as only one stage is supported right now, choose the right values
> > + * from the configuration.
> > + */
> > + page_mask = (1ULL << granule_sz) - 1;
> > aligned_addr = addr & ~page_mask;
> >
> > - cached_entry = smmu_iotlb_lookup(bs, cfg, tt, aligned_addr);
> > + SMMUTransTableInfo temp = {
> Move the declaration at the top. Also rename temp into tt to be more
> explicit about what it is?
I will move it to the top and remove granule_sz and tsz and just assign
values to this struct.
There is a pointer already called tt, I can call it tt_combined as
ideally this will hold the combined attributes for the TLB lookup.
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 07/11] hw/arm/smmuv3: Add VMID to tlb tagging
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
` (5 preceding siblings ...)
2023-02-26 22:06 ` [RFC PATCH v2 06/11] hw/arm/smmuv3: Make TLB lookup work " Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-20 16:16 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 08/11] hw/arm/smmuv3: Add CMDs related to stage-2 Mostafa Saleh
` (3 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
Allow TLB to be tagged with VMID.
If stage-1 is only supported, VMID is set to -1 and ignored from STE
and CMD_TLBI_NH* cmds.
Update smmu_iotlb_insert trace event to have vmid.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v2:
-Fix TLB aliasing issue from missing check in smmu_iotlb_key_equal.
-Add vmid to traces smmu_iotlb_insert and smmu_iotlb_lookup_hit/miss.
-Add vmid to hash function.
---
hw/arm/smmu-common.c | 36 ++++++++++++++++++++++--------------
hw/arm/smmu-internal.h | 2 ++
hw/arm/smmuv3.c | 12 +++++++++---
hw/arm/trace-events | 6 +++---
include/hw/arm/smmu-common.h | 5 +++--
5 files changed, 39 insertions(+), 22 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 3f448bc82e..3191a008c6 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -38,7 +38,7 @@ static guint smmu_iotlb_key_hash(gconstpointer v)
/* Jenkins hash */
a = b = c = JHASH_INITVAL + sizeof(*key);
- a += key->asid + key->level + key->tg;
+ a += key->asid + key->vmid + key->level + key->tg;
b += extract64(key->iova, 0, 32);
c += extract64(key->iova, 32, 32);
@@ -53,13 +53,15 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
SMMUIOTLBKey *k1 = (SMMUIOTLBKey *)v1, *k2 = (SMMUIOTLBKey *)v2;
return (k1->asid == k2->asid) && (k1->iova == k2->iova) &&
- (k1->level == k2->level) && (k1->tg == k2->tg);
+ (k1->level == k2->level) && (k1->tg == k2->tg) &&
+ (k1->vmid == k2->vmid);
}
-SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
+SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
uint8_t tg, uint8_t level)
{
- SMMUIOTLBKey key = {.asid = asid, .iova = iova, .tg = tg, .level = level};
+ SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova,
+ .tg = tg, .level = level};
return key;
}
@@ -78,7 +80,8 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
uint64_t mask = subpage_size - 1;
SMMUIOTLBKey key;
- key = smmu_get_iotlb_key(cfg->asid, iova & ~mask, tg, level);
+ key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid,
+ iova & ~mask, tg, level);
entry = g_hash_table_lookup(bs->iotlb, &key);
if (entry) {
break;
@@ -88,13 +91,13 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
if (entry) {
cfg->iotlb_hits++;
- trace_smmu_iotlb_lookup_hit(cfg->asid, iova,
+ trace_smmu_iotlb_lookup_hit(cfg->asid, cfg->s2cfg.vmid, iova,
cfg->iotlb_hits, cfg->iotlb_misses,
100 * cfg->iotlb_hits /
(cfg->iotlb_hits + cfg->iotlb_misses));
} else {
cfg->iotlb_misses++;
- trace_smmu_iotlb_lookup_miss(cfg->asid, iova,
+ trace_smmu_iotlb_lookup_miss(cfg->asid, cfg->s2cfg.vmid, iova,
cfg->iotlb_hits, cfg->iotlb_misses,
100 * cfg->iotlb_hits /
(cfg->iotlb_hits + cfg->iotlb_misses));
@@ -111,8 +114,10 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new)
smmu_iotlb_inv_all(bs);
}
- *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level);
- trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level);
+ *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
+ tg, new->level);
+ trace_smmu_iotlb_insert(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
+ tg, new->level);
g_hash_table_insert(bs->iotlb, key, new);
}
@@ -130,8 +135,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
return SMMU_IOTLB_ASID(*iotlb_key) == asid;
}
-
-static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
+static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
gpointer user_data)
{
SMMUTLBEntry *iter = (SMMUTLBEntry *)value;
@@ -142,18 +146,21 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
if (info->asid >= 0 && info->asid != SMMU_IOTLB_ASID(iotlb_key)) {
return false;
}
+ if (info->vmid >= 0 && info->vmid != SMMU_IOTLB_VMID(iotlb_key)) {
+ return false;
+ }
return ((info->iova & ~entry->addr_mask) == entry->iova) ||
((entry->iova & ~info->mask) == info->iova);
}
-void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
+void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl)
{
/* if tg is not set we use 4KB range invalidation */
uint8_t granule = tg ? tg * 2 + 10 : 12;
if (ttl && (num_pages == 1) && (asid >= 0)) {
- SMMUIOTLBKey key = smmu_get_iotlb_key(asid, iova, tg, ttl);
+ SMMUIOTLBKey key = smmu_get_iotlb_key(asid, vmid, iova, tg, ttl);
if (g_hash_table_remove(s->iotlb, &key)) {
return;
@@ -166,10 +173,11 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
SMMUIOTLBPageInvInfo info = {
.asid = asid, .iova = iova,
+ .vmid = vmid,
.mask = (num_pages * 1 << granule) - 1};
g_hash_table_foreach_remove(s->iotlb,
- smmu_hash_remove_by_asid_iova,
+ smmu_hash_remove_by_asid_vmid_iova,
&info);
}
diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
index f79c389cd3..4beba76302 100644
--- a/hw/arm/smmu-internal.h
+++ b/hw/arm/smmu-internal.h
@@ -136,9 +136,11 @@ static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
}
#define SMMU_IOTLB_ASID(key) ((key).asid)
+#define SMMU_IOTLB_VMID(key) ((key).vmid)
typedef struct SMMUIOTLBPageInvInfo {
int asid;
+ int vmid;
uint64_t iova;
uint64_t mask;
} SMMUIOTLBPageInvInfo;
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index ce193e9598..f9c06723f9 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1038,7 +1038,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
{
dma_addr_t end, addr = CMD_ADDR(cmd);
uint8_t type = CMD_TYPE(cmd);
- uint16_t vmid = CMD_VMID(cmd);
+ int vmid = -1;
uint8_t scale = CMD_SCALE(cmd);
uint8_t num = CMD_NUM(cmd);
uint8_t ttl = CMD_TTL(cmd);
@@ -1047,6 +1047,12 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
uint64_t num_pages;
uint8_t granule;
int asid = -1;
+ SMMUv3State *smmuv3 = ARM_SMMUV3(s);
+
+ /* Only consider VMID if stage-2 is supported. */
+ if (STAGE2_SUPPORTED(smmuv3)) {
+ vmid = CMD_VMID(cmd);
+ }
if (type == SMMU_CMD_TLBI_NH_VA) {
asid = CMD_ASID(cmd);
@@ -1055,7 +1061,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
if (!tg) {
trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
- smmu_iotlb_inv_iova(s, asid, addr, tg, 1, ttl);
+ smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
return;
}
@@ -1073,7 +1079,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
num_pages = (mask + 1) >> granule;
trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
- smmu_iotlb_inv_iova(s, asid, addr, tg, num_pages, ttl);
+ smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
addr += mask + 1;
}
}
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 205ac04573..705104e58b 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -14,9 +14,9 @@ smmu_iotlb_inv_all(void) "IOTLB invalidate all"
smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
smmu_iotlb_inv_iova(uint16_t asid, uint64_t addr) "IOTLB invalidate asid=%d addr=0x%"PRIx64
smmu_inv_notifiers_mr(const char *name) "iommu mr=%s"
-smmu_iotlb_lookup_hit(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
-smmu_iotlb_lookup_miss(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
-smmu_iotlb_insert(uint16_t asid, uint64_t addr, uint8_t tg, uint8_t level) "IOTLB ++ asid=%d addr=0x%"PRIx64" tg=%d level=%d"
+smmu_iotlb_lookup_hit(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
+smmu_iotlb_lookup_miss(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
+smmu_iotlb_insert(uint16_t asid, uint16_t vmid, uint64_t addr, uint8_t tg, uint8_t level) "IOTLB ++ asid=%d vmid=%d addr=0x%"PRIx64" tg=%d level=%d"
# smmuv3.c
smmuv3_read_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index 1ada792122..c415e8d853 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -118,6 +118,7 @@ typedef struct SMMUPciBus {
typedef struct SMMUIOTLBKey {
uint64_t iova;
uint16_t asid;
+ uint16_t vmid;
uint8_t tg;
uint8_t level;
} SMMUIOTLBKey;
@@ -181,11 +182,11 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
SMMUTransTableInfo *tt, hwaddr iova);
void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
-SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
+SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
uint8_t tg, uint8_t level);
void smmu_iotlb_inv_all(SMMUState *s);
void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
-void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
+void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl);
/* Unmap the range of all the notifiers registered to any IOMMU mr */
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 07/11] hw/arm/smmuv3: Add VMID to tlb tagging
2023-02-26 22:06 ` [RFC PATCH v2 07/11] hw/arm/smmuv3: Add VMID to tlb tagging Mostafa Saleh
@ 2023-03-20 16:16 ` Eric Auger
0 siblings, 0 replies; 37+ messages in thread
From: Eric Auger @ 2023-03-20 16:16 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi Mostafa,
On 2/26/23 23:06, Mostafa Saleh wrote:
> Allow TLB to be tagged with VMID.
s/tlb/TLB in the commit msg.
>
> If stage-1 is only supported, VMID is set to -1 and ignored from STE
> and CMD_TLBI_NH* cmds.
>
> Update smmu_iotlb_insert trace event to have vmid.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in v2:
> -Fix TLB aliasing issue from missing check in smmu_iotlb_key_equal.
> -Add vmid to traces smmu_iotlb_insert and smmu_iotlb_lookup_hit/miss.
> -Add vmid to hash function.
> ---
> hw/arm/smmu-common.c | 36 ++++++++++++++++++++++--------------
> hw/arm/smmu-internal.h | 2 ++
> hw/arm/smmuv3.c | 12 +++++++++---
> hw/arm/trace-events | 6 +++---
> include/hw/arm/smmu-common.h | 5 +++--
> 5 files changed, 39 insertions(+), 22 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 3f448bc82e..3191a008c6 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -38,7 +38,7 @@ static guint smmu_iotlb_key_hash(gconstpointer v)
>
> /* Jenkins hash */
> a = b = c = JHASH_INITVAL + sizeof(*key);
> - a += key->asid + key->level + key->tg;
> + a += key->asid + key->vmid + key->level + key->tg;
> b += extract64(key->iova, 0, 32);
> c += extract64(key->iova, 32, 32);
>
> @@ -53,13 +53,15 @@ static gboolean smmu_iotlb_key_equal(gconstpointer v1, gconstpointer v2)
> SMMUIOTLBKey *k1 = (SMMUIOTLBKey *)v1, *k2 = (SMMUIOTLBKey *)v2;
>
> return (k1->asid == k2->asid) && (k1->iova == k2->iova) &&
> - (k1->level == k2->level) && (k1->tg == k2->tg);
> + (k1->level == k2->level) && (k1->tg == k2->tg) &&
> + (k1->vmid == k2->vmid);
> }
>
> -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
> +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
> uint8_t tg, uint8_t level)
> {
> - SMMUIOTLBKey key = {.asid = asid, .iova = iova, .tg = tg, .level = level};
> + SMMUIOTLBKey key = {.asid = asid, .vmid = vmid, .iova = iova,
> + .tg = tg, .level = level};
>
> return key;
> }
> @@ -78,7 +80,8 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> uint64_t mask = subpage_size - 1;
> SMMUIOTLBKey key;
>
> - key = smmu_get_iotlb_key(cfg->asid, iova & ~mask, tg, level);
> + key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid,
> + iova & ~mask, tg, level);
> entry = g_hash_table_lookup(bs->iotlb, &key);
> if (entry) {
> break;
> @@ -88,13 +91,13 @@ SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
>
> if (entry) {
> cfg->iotlb_hits++;
> - trace_smmu_iotlb_lookup_hit(cfg->asid, iova,
> + trace_smmu_iotlb_lookup_hit(cfg->asid, cfg->s2cfg.vmid, iova,
> cfg->iotlb_hits, cfg->iotlb_misses,
> 100 * cfg->iotlb_hits /
> (cfg->iotlb_hits + cfg->iotlb_misses));
> } else {
> cfg->iotlb_misses++;
> - trace_smmu_iotlb_lookup_miss(cfg->asid, iova,
> + trace_smmu_iotlb_lookup_miss(cfg->asid, cfg->s2cfg.vmid, iova,
> cfg->iotlb_hits, cfg->iotlb_misses,
> 100 * cfg->iotlb_hits /
> (cfg->iotlb_hits + cfg->iotlb_misses));
> @@ -111,8 +114,10 @@ void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *new)
> smmu_iotlb_inv_all(bs);
> }
>
> - *key = smmu_get_iotlb_key(cfg->asid, new->entry.iova, tg, new->level);
> - trace_smmu_iotlb_insert(cfg->asid, new->entry.iova, tg, new->level);
> + *key = smmu_get_iotlb_key(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> + tg, new->level);
> + trace_smmu_iotlb_insert(cfg->asid, cfg->s2cfg.vmid, new->entry.iova,
> + tg, new->level);
> g_hash_table_insert(bs->iotlb, key, new);
> }
>
> @@ -130,8 +135,7 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
>
> return SMMU_IOTLB_ASID(*iotlb_key) == asid;
> }
> -
> -static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
> +static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
> gpointer user_data)
> {
> SMMUTLBEntry *iter = (SMMUTLBEntry *)value;
> @@ -142,18 +146,21 @@ static gboolean smmu_hash_remove_by_asid_iova(gpointer key, gpointer value,
> if (info->asid >= 0 && info->asid != SMMU_IOTLB_ASID(iotlb_key)) {
> return false;
> }
> + if (info->vmid >= 0 && info->vmid != SMMU_IOTLB_VMID(iotlb_key)) {
> + return false;
> + }
> return ((info->iova & ~entry->addr_mask) == entry->iova) ||
> ((entry->iova & ~info->mask) == info->iova);
> }
>
> -void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
> +void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> uint8_t tg, uint64_t num_pages, uint8_t ttl)
> {
> /* if tg is not set we use 4KB range invalidation */
> uint8_t granule = tg ? tg * 2 + 10 : 12;
>
> if (ttl && (num_pages == 1) && (asid >= 0)) {
> - SMMUIOTLBKey key = smmu_get_iotlb_key(asid, iova, tg, ttl);
> + SMMUIOTLBKey key = smmu_get_iotlb_key(asid, vmid, iova, tg, ttl);
>
> if (g_hash_table_remove(s->iotlb, &key)) {
> return;
> @@ -166,10 +173,11 @@ void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
>
> SMMUIOTLBPageInvInfo info = {
> .asid = asid, .iova = iova,
> + .vmid = vmid,
> .mask = (num_pages * 1 << granule) - 1};
>
> g_hash_table_foreach_remove(s->iotlb,
> - smmu_hash_remove_by_asid_iova,
> + smmu_hash_remove_by_asid_vmid_iova,
> &info);
> }
>
> diff --git a/hw/arm/smmu-internal.h b/hw/arm/smmu-internal.h
> index f79c389cd3..4beba76302 100644
> --- a/hw/arm/smmu-internal.h
> +++ b/hw/arm/smmu-internal.h
> @@ -136,9 +136,11 @@ static inline int pgd_idx(int start_level, int granule, dma_addr_t iova)
> }
>
> #define SMMU_IOTLB_ASID(key) ((key).asid)
> +#define SMMU_IOTLB_VMID(key) ((key).vmid)
>
> typedef struct SMMUIOTLBPageInvInfo {
> int asid;
> + int vmid;
> uint64_t iova;
> uint64_t mask;
> } SMMUIOTLBPageInvInfo;
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index ce193e9598..f9c06723f9 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1038,7 +1038,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
> {
> dma_addr_t end, addr = CMD_ADDR(cmd);
> uint8_t type = CMD_TYPE(cmd);
> - uint16_t vmid = CMD_VMID(cmd);
> + int vmid = -1;
> uint8_t scale = CMD_SCALE(cmd);
> uint8_t num = CMD_NUM(cmd);
> uint8_t ttl = CMD_TTL(cmd);
> @@ -1047,6 +1047,12 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
> uint64_t num_pages;
> uint8_t granule;
> int asid = -1;
> + SMMUv3State *smmuv3 = ARM_SMMUV3(s);
> +
> + /* Only consider VMID if stage-2 is supported. */
> + if (STAGE2_SUPPORTED(smmuv3)) {
> + vmid = CMD_VMID(cmd);
> + }
>
> if (type == SMMU_CMD_TLBI_NH_VA) {
> asid = CMD_ASID(cmd);
> @@ -1055,7 +1061,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
> if (!tg) {
> trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
> smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
> - smmu_iotlb_inv_iova(s, asid, addr, tg, 1, ttl);
> + smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
> return;
> }
>
> @@ -1073,7 +1079,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
> num_pages = (mask + 1) >> granule;
> trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
> smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
> - smmu_iotlb_inv_iova(s, asid, addr, tg, num_pages, ttl);
> + smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
> addr += mask + 1;
> }
> }
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 205ac04573..705104e58b 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -14,9 +14,9 @@ smmu_iotlb_inv_all(void) "IOTLB invalidate all"
> smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
> smmu_iotlb_inv_iova(uint16_t asid, uint64_t addr) "IOTLB invalidate asid=%d addr=0x%"PRIx64
> smmu_inv_notifiers_mr(const char *name) "iommu mr=%s"
> -smmu_iotlb_lookup_hit(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> -smmu_iotlb_lookup_miss(uint16_t asid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> -smmu_iotlb_insert(uint16_t asid, uint64_t addr, uint8_t tg, uint8_t level) "IOTLB ++ asid=%d addr=0x%"PRIx64" tg=%d level=%d"
> +smmu_iotlb_lookup_hit(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> +smmu_iotlb_lookup_miss(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache MISS asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> +smmu_iotlb_insert(uint16_t asid, uint16_t vmid, uint64_t addr, uint8_t tg, uint8_t level) "IOTLB ++ asid=%d vmid=%d addr=0x%"PRIx64" tg=%d level=%d"
>
> # smmuv3.c
> smmuv3_read_mmio(uint64_t addr, uint64_t val, unsigned size, uint32_t r) "addr: 0x%"PRIx64" val:0x%"PRIx64" size: 0x%x(%d)"
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index 1ada792122..c415e8d853 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -118,6 +118,7 @@ typedef struct SMMUPciBus {
> typedef struct SMMUIOTLBKey {
> uint64_t iova;
> uint16_t asid;
> + uint16_t vmid;
> uint8_t tg;
> uint8_t level;
> } SMMUIOTLBKey;
> @@ -181,11 +182,11 @@ IOMMUMemoryRegion *smmu_iommu_mr(SMMUState *s, uint32_t sid);
> SMMUTLBEntry *smmu_iotlb_lookup(SMMUState *bs, SMMUTransCfg *cfg,
> SMMUTransTableInfo *tt, hwaddr iova);
> void smmu_iotlb_insert(SMMUState *bs, SMMUTransCfg *cfg, SMMUTLBEntry *entry);
> -SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint64_t iova,
> +SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
> uint8_t tg, uint8_t level);
> void smmu_iotlb_inv_all(SMMUState *s);
> void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
> -void smmu_iotlb_inv_iova(SMMUState *s, int asid, dma_addr_t iova,
> +void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> uint8_t tg, uint64_t num_pages, uint8_t ttl);
>
> /* Unmap the range of all the notifiers registered to any IOMMU mr */
Looks good to me:
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 08/11] hw/arm/smmuv3: Add CMDs related to stage-2
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
` (6 preceding siblings ...)
2023-02-26 22:06 ` [RFC PATCH v2 07/11] hw/arm/smmuv3: Add VMID to tlb tagging Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-20 16:51 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 09/11] hw/arm/smmuv3: Add stage-2 support in iova notifier Mostafa Saleh
` (2 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
CMD_TLBI_S2_IPA: As S1+S2 is not enabled, for now this can be the
same as CMD_TLBI_NH_VAA.
CMD_TLBI_S12_VMALL: Added new function to invalidate TLB by VMID.
For stage-1 only commands, add a check to to throw CERROR_ILL if used
when stage-1 is not supported.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v2:
- Add checks for stage-1 only commands
- Rename smmuv3_s1_range_inval to smmuv3_range_inval
---
hw/arm/smmu-common.c | 16 ++++++++++++
hw/arm/smmuv3.c | 47 +++++++++++++++++++++++++++++++-----
hw/arm/trace-events | 4 ++-
include/hw/arm/smmu-common.h | 1 +
4 files changed, 61 insertions(+), 7 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index 3191a008c6..e4b477af10 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -135,6 +135,16 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
return SMMU_IOTLB_ASID(*iotlb_key) == asid;
}
+
+static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
+ gpointer user_data)
+{
+ uint16_t vmid = *(uint16_t *)user_data;
+ SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
+
+ return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
+}
+
static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
gpointer user_data)
{
@@ -187,6 +197,12 @@ void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
}
+inline void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
+{
+ trace_smmu_iotlb_inv_vmid(vmid);
+ g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
+}
+
/* VMSAv8-64 Translation */
/**
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index f9c06723f9..8c76a48c8d 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -1034,7 +1034,7 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova,
}
}
-static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
+static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
{
dma_addr_t end, addr = CMD_ADDR(cmd);
uint8_t type = CMD_TYPE(cmd);
@@ -1059,7 +1059,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
}
if (!tg) {
- trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
+ trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
return;
@@ -1077,7 +1077,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
uint64_t mask = dma_aligned_pow2_mask(addr, end, 64);
num_pages = (mask + 1) >> granule;
- trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
+ trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
addr += mask + 1;
@@ -1211,12 +1211,22 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
{
uint16_t asid = CMD_ASID(&cmd);
+ if (!STAGE1_SUPPORTED(s)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+
trace_smmuv3_cmdq_tlbi_nh_asid(asid);
smmu_inv_notifiers_all(&s->smmu_state);
smmu_iotlb_inv_asid(bs, asid);
break;
}
case SMMU_CMD_TLBI_NH_ALL:
+ if (!STAGE1_SUPPORTED(s)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+ QEMU_FALLTHROUGH;
case SMMU_CMD_TLBI_NSNH_ALL:
trace_smmuv3_cmdq_tlbi_nh();
smmu_inv_notifiers_all(&s->smmu_state);
@@ -1224,7 +1234,34 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
break;
case SMMU_CMD_TLBI_NH_VAA:
case SMMU_CMD_TLBI_NH_VA:
- smmuv3_s1_range_inval(bs, &cmd);
+ if (!STAGE1_SUPPORTED(s)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+ smmuv3_range_inval(bs, &cmd);
+ break;
+ case SMMU_CMD_TLBI_S12_VMALL:
+ uint16_t vmid = CMD_VMID(&cmd);
+
+ if (!STAGE2_SUPPORTED(s)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+
+ trace_smmuv3_cmdq_tlbi_s12_vmid(vmid);
+ smmu_inv_notifiers_all(&s->smmu_state);
+ smmu_iotlb_inv_vmid(bs, vmid);
+ break;
+ case SMMU_CMD_TLBI_S2_IPA:
+ if (!STAGE2_SUPPORTED(s)) {
+ cmd_error = SMMU_CERROR_ILL;
+ break;
+ }
+ /*
+ * As currently only either s1 or s2 are supported
+ * we can reuse same function for s2.
+ */
+ smmuv3_range_inval(bs, &cmd);
break;
case SMMU_CMD_TLBI_EL3_ALL:
case SMMU_CMD_TLBI_EL3_VA:
@@ -1232,8 +1269,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
case SMMU_CMD_TLBI_EL2_ASID:
case SMMU_CMD_TLBI_EL2_VA:
case SMMU_CMD_TLBI_EL2_VAA:
- case SMMU_CMD_TLBI_S12_VMALL:
- case SMMU_CMD_TLBI_S2_IPA:
case SMMU_CMD_ATC_INV:
case SMMU_CMD_PRI_RESP:
case SMMU_CMD_RESUME:
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index 705104e58b..f8fdf1ca9f 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -12,6 +12,7 @@ smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, ui
smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64
smmu_iotlb_inv_all(void) "IOTLB invalidate all"
smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
+smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%d"
smmu_iotlb_inv_iova(uint16_t asid, uint64_t addr) "IOTLB invalidate asid=%d addr=0x%"PRIx64
smmu_inv_notifiers_mr(const char *name) "iommu mr=%s"
smmu_iotlb_lookup_hit(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
@@ -45,9 +46,10 @@ smmuv3_cmdq_cfgi_ste_range(int start, int end) "start=0x%x - end=0x%x"
smmuv3_cmdq_cfgi_cd(uint32_t sid) "sid=0x%x"
smmuv3_config_cache_hit(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache HIT for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache MISS for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
-smmuv3_s1_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d"
+smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d"
smmuv3_cmdq_tlbi_nh(void) ""
smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
+smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s"
smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
index c415e8d853..d8b458379e 100644
--- a/include/hw/arm/smmu-common.h
+++ b/include/hw/arm/smmu-common.h
@@ -186,6 +186,7 @@ SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
uint8_t tg, uint8_t level);
void smmu_iotlb_inv_all(SMMUState *s);
void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
+void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid);
void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
uint8_t tg, uint64_t num_pages, uint8_t ttl);
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 08/11] hw/arm/smmuv3: Add CMDs related to stage-2
2023-02-26 22:06 ` [RFC PATCH v2 08/11] hw/arm/smmuv3: Add CMDs related to stage-2 Mostafa Saleh
@ 2023-03-20 16:51 ` Eric Auger
2023-03-20 19:29 ` Mostafa Saleh
0 siblings, 1 reply; 37+ messages in thread
From: Eric Auger @ 2023-03-20 16:51 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi Mostafa,
On 2/26/23 23:06, Mostafa Saleh wrote:
> CMD_TLBI_S2_IPA: As S1+S2 is not enabled, for now this can be the
> same as CMD_TLBI_NH_VAA.
>
> CMD_TLBI_S12_VMALL: Added new function to invalidate TLB by VMID.
>
> For stage-1 only commands, add a check to to throw CERROR_ILL if used
s/to to/to
> when stage-1 is not supported.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> Changes in v2:
> - Add checks for stage-1 only commands
> - Rename smmuv3_s1_range_inval to smmuv3_range_inval
> ---
> hw/arm/smmu-common.c | 16 ++++++++++++
> hw/arm/smmuv3.c | 47 +++++++++++++++++++++++++++++++-----
> hw/arm/trace-events | 4 ++-
> include/hw/arm/smmu-common.h | 1 +
> 4 files changed, 61 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index 3191a008c6..e4b477af10 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -135,6 +135,16 @@ static gboolean smmu_hash_remove_by_asid(gpointer key, gpointer value,
>
> return SMMU_IOTLB_ASID(*iotlb_key) == asid;
> }
> +
> +static gboolean smmu_hash_remove_by_vmid(gpointer key, gpointer value,
> + gpointer user_data)
> +{
> + uint16_t vmid = *(uint16_t *)user_data;
> + SMMUIOTLBKey *iotlb_key = (SMMUIOTLBKey *)key;
> +
> + return SMMU_IOTLB_VMID(*iotlb_key) == vmid;
> +}
> +
> static gboolean smmu_hash_remove_by_asid_vmid_iova(gpointer key, gpointer value,
> gpointer user_data)
> {
> @@ -187,6 +197,12 @@ void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid)
> g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_asid, &asid);
> }
>
> +inline void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid)
> +{
> + trace_smmu_iotlb_inv_vmid(vmid);
> + g_hash_table_foreach_remove(s->iotlb, smmu_hash_remove_by_vmid, &vmid);
> +}
> +
> /* VMSAv8-64 Translation */
>
> /**
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index f9c06723f9..8c76a48c8d 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -1034,7 +1034,7 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova,
> }
> }
>
> -static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
> +static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
> {
> dma_addr_t end, addr = CMD_ADDR(cmd);
> uint8_t type = CMD_TYPE(cmd);
> @@ -1059,7 +1059,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
> }
>
> if (!tg) {
> - trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
> + trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
> smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
> smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
> return;
> @@ -1077,7 +1077,7 @@ static void smmuv3_s1_range_inval(SMMUState *s, Cmd *cmd)
> uint64_t mask = dma_aligned_pow2_mask(addr, end, 64);
>
> num_pages = (mask + 1) >> granule;
> - trace_smmuv3_s1_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
> + trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
> smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
> smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
> addr += mask + 1;
> @@ -1211,12 +1211,22 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> {
> uint16_t asid = CMD_ASID(&cmd);
>
> + if (!STAGE1_SUPPORTED(s)) {
> + cmd_error = SMMU_CERROR_ILL;
Well looking further this is not said explicitly this should return
SMMU_CERROR_ILL. Maybe you should mark it as a guest error because we do not expect a guest to send such command when S1 is not supported?
> + break;
> + }
> +
> trace_smmuv3_cmdq_tlbi_nh_asid(asid);
> smmu_inv_notifiers_all(&s->smmu_state);
> smmu_iotlb_inv_asid(bs, asid);
> break;
> }
> case SMMU_CMD_TLBI_NH_ALL:
> + if (!STAGE1_SUPPORTED(s)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
there is a VMID field. Can't this be used in S2 mode as well?
> + }
> + QEMU_FALLTHROUGH;
> case SMMU_CMD_TLBI_NSNH_ALL:
> trace_smmuv3_cmdq_tlbi_nh();
> smmu_inv_notifiers_all(&s->smmu_state);
> @@ -1224,7 +1234,34 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> break;
> case SMMU_CMD_TLBI_NH_VAA:
> case SMMU_CMD_TLBI_NH_VA:
> - smmuv3_s1_range_inval(bs, &cmd);
> + if (!STAGE1_SUPPORTED(s)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> + smmuv3_range_inval(bs, &cmd);
> + break;
> + case SMMU_CMD_TLBI_S12_VMALL:
> + uint16_t vmid = CMD_VMID(&cmd);
> +
> + if (!STAGE2_SUPPORTED(s)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> +
> + trace_smmuv3_cmdq_tlbi_s12_vmid(vmid);
> + smmu_inv_notifiers_all(&s->smmu_state);
> + smmu_iotlb_inv_vmid(bs, vmid);
> + break;
> + case SMMU_CMD_TLBI_S2_IPA:
> + if (!STAGE2_SUPPORTED(s)) {
> + cmd_error = SMMU_CERROR_ILL;
> + break;
> + }
> + /*
> + * As currently only either s1 or s2 are supported
> + * we can reuse same function for s2.
> + */
> + smmuv3_range_inval(bs, &cmd);
> break;
> case SMMU_CMD_TLBI_EL3_ALL:
> case SMMU_CMD_TLBI_EL3_VA:
> @@ -1232,8 +1269,6 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> case SMMU_CMD_TLBI_EL2_ASID:
> case SMMU_CMD_TLBI_EL2_VA:
> case SMMU_CMD_TLBI_EL2_VAA:
> - case SMMU_CMD_TLBI_S12_VMALL:
> - case SMMU_CMD_TLBI_S2_IPA:
> case SMMU_CMD_ATC_INV:
> case SMMU_CMD_PRI_RESP:
> case SMMU_CMD_RESUME:
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index 705104e58b..f8fdf1ca9f 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -12,6 +12,7 @@ smmu_ptw_block_pte(int stage, int level, uint64_t baseaddr, uint64_t pteaddr, ui
> smmu_get_pte(uint64_t baseaddr, int index, uint64_t pteaddr, uint64_t pte) "baseaddr=0x%"PRIx64" index=0x%x, pteaddr=0x%"PRIx64", pte=0x%"PRIx64
> smmu_iotlb_inv_all(void) "IOTLB invalidate all"
> smmu_iotlb_inv_asid(uint16_t asid) "IOTLB invalidate asid=%d"
> +smmu_iotlb_inv_vmid(uint16_t vmid) "IOTLB invalidate vmid=%d"
> smmu_iotlb_inv_iova(uint16_t asid, uint64_t addr) "IOTLB invalidate asid=%d addr=0x%"PRIx64
> smmu_inv_notifiers_mr(const char *name) "iommu mr=%s"
> smmu_iotlb_lookup_hit(uint16_t asid, uint16_t vmid, uint64_t addr, uint32_t hit, uint32_t miss, uint32_t p) "IOTLB cache HIT asid=%d vmid=%d addr=0x%"PRIx64" hit=%d miss=%d hit rate=%d"
> @@ -45,9 +46,10 @@ smmuv3_cmdq_cfgi_ste_range(int start, int end) "start=0x%x - end=0x%x"
> smmuv3_cmdq_cfgi_cd(uint32_t sid) "sid=0x%x"
> smmuv3_config_cache_hit(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache HIT for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
> smmuv3_config_cache_miss(uint32_t sid, uint32_t hits, uint32_t misses, uint32_t perc) "Config cache MISS for sid=0x%x (hits=%d, misses=%d, hit rate=%d)"
> -smmuv3_s1_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d"
> +smmuv3_range_inval(int vmid, int asid, uint64_t addr, uint8_t tg, uint64_t num_pages, uint8_t ttl, bool leaf) "vmid=%d asid=%d addr=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64" ttl=%d leaf=%d"
> smmuv3_cmdq_tlbi_nh(void) ""
> smmuv3_cmdq_tlbi_nh_asid(uint16_t asid) "asid=%d"
> +smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
> smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
> smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s"
> smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
> diff --git a/include/hw/arm/smmu-common.h b/include/hw/arm/smmu-common.h
> index c415e8d853..d8b458379e 100644
> --- a/include/hw/arm/smmu-common.h
> +++ b/include/hw/arm/smmu-common.h
> @@ -186,6 +186,7 @@ SMMUIOTLBKey smmu_get_iotlb_key(uint16_t asid, uint16_t vmid, uint64_t iova,
> uint8_t tg, uint8_t level);
> void smmu_iotlb_inv_all(SMMUState *s);
> void smmu_iotlb_inv_asid(SMMUState *s, uint16_t asid);
> +void smmu_iotlb_inv_vmid(SMMUState *s, uint16_t vmid);
> void smmu_iotlb_inv_iova(SMMUState *s, int asid, int vmid, dma_addr_t iova,
> uint8_t tg, uint64_t num_pages, uint8_t ttl);
>
Thanks
Eric
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 08/11] hw/arm/smmuv3: Add CMDs related to stage-2
2023-03-20 16:51 ` Eric Auger
@ 2023-03-20 19:29 ` Mostafa Saleh
0 siblings, 0 replies; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-20 19:29 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Eric,
On Mon, Mar 20, 2023 at 05:51:07PM +0100, Eric Auger wrote:
> Hi Mostafa,
>
> On 2/26/23 23:06, Mostafa Saleh wrote:
> > CMD_TLBI_S2_IPA: As S1+S2 is not enabled, for now this can be the
> > same as CMD_TLBI_NH_VAA.
> >
> > CMD_TLBI_S12_VMALL: Added new function to invalidate TLB by VMID.
> >
> > For stage-1 only commands, add a check to to throw CERROR_ILL if used
> s/to to/to
Will do.
> > @@ -1211,12 +1211,22 @@ static int smmuv3_cmdq_consume(SMMUv3State *s)
> > {
> > uint16_t asid = CMD_ASID(&cmd);
> >
> > + if (!STAGE1_SUPPORTED(s)) {
> > + cmd_error = SMMU_CERROR_ILL;
> Well looking further this is not said explicitly this should return
>
> SMMU_CERROR_ILL. Maybe you should mark it as a guest error because we do not expect a guest to send such command when S1 is not supported?
>
I can add a check after the switch for SMMU_CERROR_ILL to log a guest
error.
> > + break;
> > + }
> > +
> > trace_smmuv3_cmdq_tlbi_nh_asid(asid);
> > smmu_inv_notifiers_all(&s->smmu_state);
> > smmu_iotlb_inv_asid(bs, asid);
> > break;
> > }
> > case SMMU_CMD_TLBI_NH_ALL:
> > + if (!STAGE1_SUPPORTED(s)) {
> > + cmd_error = SMMU_CERROR_ILL;
> > + break;
>
> there is a VMID field. Can't this be used in S2 mode as well?
According to the user manual "4.4.2 TLB invalidation of stage 1"
CMD_TLBI_NH_ALL causes CERROR_ILL if stage-1 is not supported.
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 09/11] hw/arm/smmuv3: Add stage-2 support in iova notifier
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
` (7 preceding siblings ...)
2023-02-26 22:06 ` [RFC PATCH v2 08/11] hw/arm/smmuv3: Add CMDs related to stage-2 Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-20 16:57 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE Mostafa Saleh
2023-02-26 22:06 ` [RFC PATCH v2 11/11] hw/arm/smmuv3: Add knob to choose translation stage and enable stage-2 Mostafa Saleh
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
In smmuv3_notify_iova, read the granule based on translation stage
and use VMID if valid value is sent.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmuv3.c | 39 ++++++++++++++++++++++++++-------------
hw/arm/trace-events | 2 +-
2 files changed, 27 insertions(+), 14 deletions(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 8c76a48c8d..7297f6adc1 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -971,18 +971,21 @@ epilogue:
* @mr: IOMMU mr region handle
* @n: notifier to be called
* @asid: address space ID or negative value if we don't care
+ * @vmid: virtual machine ID or negative value if we don't care
* @iova: iova
* @tg: translation granule (if communicated through range invalidation)
* @num_pages: number of @granule sized pages (if tg != 0), otherwise 1
*/
static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
IOMMUNotifier *n,
- int asid, dma_addr_t iova,
- uint8_t tg, uint64_t num_pages)
+ int asid, int vmid,
+ dma_addr_t iova, uint8_t tg,
+ uint64_t num_pages)
{
SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
IOMMUTLBEvent event;
uint8_t granule;
+ SMMUv3State *s = sdev->smmu;
if (!tg) {
SMMUEventInfo event = {.inval_ste_allowed = true};
@@ -997,11 +1000,20 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
return;
}
- tt = select_tt(cfg, iova);
- if (!tt) {
+ if (vmid >= 0 && cfg->s2cfg.vmid != vmid) {
return;
}
- granule = tt->granule_sz;
+
+ if (STAGE1_SUPPORTED(s)) {
+ tt = select_tt(cfg, iova);
+ if (!tt) {
+ return;
+ }
+ granule = tt->granule_sz;
+ } else {
+ granule = cfg->s2cfg.granule_sz;
+ }
+
} else {
granule = tg * 2 + 10;
}
@@ -1015,9 +1027,10 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
memory_region_notify_iommu_one(n, &event);
}
-/* invalidate an asid/iova range tuple in all mr's */
-static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova,
- uint8_t tg, uint64_t num_pages)
+/* invalidate an asid/vmid/iova range tuple in all mr's */
+static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid,
+ dma_addr_t iova, uint8_t tg,
+ uint64_t num_pages)
{
SMMUDevice *sdev;
@@ -1025,11 +1038,11 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova,
IOMMUMemoryRegion *mr = &sdev->iommu;
IOMMUNotifier *n;
- trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, iova,
- tg, num_pages);
+ trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid,
+ iova, tg, num_pages);
IOMMU_NOTIFIER_FOREACH(n, mr) {
- smmuv3_notify_iova(mr, n, asid, iova, tg, num_pages);
+ smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages);
}
}
}
@@ -1060,7 +1073,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
if (!tg) {
trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
- smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
+ smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1);
smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
return;
}
@@ -1078,7 +1091,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
num_pages = (mask + 1) >> granule;
trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
- smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
+ smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages);
smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
addr += mask + 1;
}
diff --git a/hw/arm/trace-events b/hw/arm/trace-events
index f8fdf1ca9f..cdc1ea06a8 100644
--- a/hw/arm/trace-events
+++ b/hw/arm/trace-events
@@ -53,5 +53,5 @@ smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s"
smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
-smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
+smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint16_t vmid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 09/11] hw/arm/smmuv3: Add stage-2 support in iova notifier
2023-02-26 22:06 ` [RFC PATCH v2 09/11] hw/arm/smmuv3: Add stage-2 support in iova notifier Mostafa Saleh
@ 2023-03-20 16:57 ` Eric Auger
0 siblings, 0 replies; 37+ messages in thread
From: Eric Auger @ 2023-03-20 16:57 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi,
On 2/26/23 23:06, Mostafa Saleh wrote:
> In smmuv3_notify_iova, read the granule based on translation stage
> and use VMID if valid value is sent.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmuv3.c | 39 ++++++++++++++++++++++++++-------------
> hw/arm/trace-events | 2 +-
> 2 files changed, 27 insertions(+), 14 deletions(-)
>
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 8c76a48c8d..7297f6adc1 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -971,18 +971,21 @@ epilogue:
> * @mr: IOMMU mr region handle
> * @n: notifier to be called
> * @asid: address space ID or negative value if we don't care
> + * @vmid: virtual machine ID or negative value if we don't care
> * @iova: iova
> * @tg: translation granule (if communicated through range invalidation)
> * @num_pages: number of @granule sized pages (if tg != 0), otherwise 1
> */
> static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
> IOMMUNotifier *n,
> - int asid, dma_addr_t iova,
> - uint8_t tg, uint64_t num_pages)
> + int asid, int vmid,
> + dma_addr_t iova, uint8_t tg,
> + uint64_t num_pages)
> {
> SMMUDevice *sdev = container_of(mr, SMMUDevice, iommu);
> IOMMUTLBEvent event;
> uint8_t granule;
> + SMMUv3State *s = sdev->smmu;
I am not sure notifiers are likely to be registered in the S2 case
because it is mostly meant to integrate with vhost or VFIO. The code in
this patch would rather prepare for nested stage support I guess, I
don't think it can harm.
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Eric
>
> if (!tg) {
> SMMUEventInfo event = {.inval_ste_allowed = true};
> @@ -997,11 +1000,20 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
> return;
> }
>
> - tt = select_tt(cfg, iova);
> - if (!tt) {
> + if (vmid >= 0 && cfg->s2cfg.vmid != vmid) {
> return;
> }
> - granule = tt->granule_sz;
> +
> + if (STAGE1_SUPPORTED(s)) {
> + tt = select_tt(cfg, iova);
> + if (!tt) {
> + return;
> + }
> + granule = tt->granule_sz;
> + } else {
> + granule = cfg->s2cfg.granule_sz;
> + }
> +
> } else {
> granule = tg * 2 + 10;
> }
> @@ -1015,9 +1027,10 @@ static void smmuv3_notify_iova(IOMMUMemoryRegion *mr,
> memory_region_notify_iommu_one(n, &event);
> }
>
> -/* invalidate an asid/iova range tuple in all mr's */
> -static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova,
> - uint8_t tg, uint64_t num_pages)
> +/* invalidate an asid/vmid/iova range tuple in all mr's */
> +static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, int vmid,
> + dma_addr_t iova, uint8_t tg,
> + uint64_t num_pages)
> {
> SMMUDevice *sdev;
>
> @@ -1025,11 +1038,11 @@ static void smmuv3_inv_notifiers_iova(SMMUState *s, int asid, dma_addr_t iova,
> IOMMUMemoryRegion *mr = &sdev->iommu;
> IOMMUNotifier *n;
>
> - trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, iova,
> - tg, num_pages);
> + trace_smmuv3_inv_notifiers_iova(mr->parent_obj.name, asid, vmid,
> + iova, tg, num_pages);
>
> IOMMU_NOTIFIER_FOREACH(n, mr) {
> - smmuv3_notify_iova(mr, n, asid, iova, tg, num_pages);
> + smmuv3_notify_iova(mr, n, asid, vmid, iova, tg, num_pages);
> }
> }
> }
> @@ -1060,7 +1073,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
>
> if (!tg) {
> trace_smmuv3_range_inval(vmid, asid, addr, tg, 1, ttl, leaf);
> - smmuv3_inv_notifiers_iova(s, asid, addr, tg, 1);
> + smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, 1);
> smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, 1, ttl);
> return;
> }
> @@ -1078,7 +1091,7 @@ static void smmuv3_range_inval(SMMUState *s, Cmd *cmd)
>
> num_pages = (mask + 1) >> granule;
> trace_smmuv3_range_inval(vmid, asid, addr, tg, num_pages, ttl, leaf);
> - smmuv3_inv_notifiers_iova(s, asid, addr, tg, num_pages);
> + smmuv3_inv_notifiers_iova(s, asid, vmid, addr, tg, num_pages);
> smmu_iotlb_inv_iova(s, asid, vmid, addr, tg, num_pages, ttl);
> addr += mask + 1;
> }
> diff --git a/hw/arm/trace-events b/hw/arm/trace-events
> index f8fdf1ca9f..cdc1ea06a8 100644
> --- a/hw/arm/trace-events
> +++ b/hw/arm/trace-events
> @@ -53,5 +53,5 @@ smmuv3_cmdq_tlbi_s12_vmid(uint16_t vmid) "vmid=%d"
> smmuv3_config_cache_inv(uint32_t sid) "Config cache INV for sid=0x%x"
> smmuv3_notify_flag_add(const char *iommu) "ADD SMMUNotifier node for iommu mr=%s"
> smmuv3_notify_flag_del(const char *iommu) "DEL SMMUNotifier node for iommu mr=%s"
> -smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
> +smmuv3_inv_notifiers_iova(const char *name, uint16_t asid, uint16_t vmid, uint64_t iova, uint8_t tg, uint64_t num_pages) "iommu mr=%s asid=%d vmid=%d iova=0x%"PRIx64" tg=%d num_pages=0x%"PRIx64
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
` (8 preceding siblings ...)
2023-02-26 22:06 ` [RFC PATCH v2 09/11] hw/arm/smmuv3: Add stage-2 support in iova notifier Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
2023-03-20 17:12 ` Eric Auger
2023-02-26 22:06 ` [RFC PATCH v2 11/11] hw/arm/smmuv3: Add knob to choose translation stage and enable stage-2 Mostafa Saleh
10 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
OAS used to be hardcoded to 44 bits, however according to SMMU manual
6.3.6 SMMU_IDR5, OAS must match the system physical address size, so
we read it from CPU PARANGE.
Remove PA_MAX and pa_range as they were not used.
Add SMMUv3State as an argument to decode_cd, so it can read the SMMU
OAS.
As CPU can use PARANGE with 52 bits, add 52 bits check to oas2bits,
and cap OAS to 48 bits for stage-1 and stage-2 if granule is not 64KB
as specified for SMMUv3.1 and later.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
hw/arm/smmu-common.c | 13 +++++++++----
hw/arm/smmuv3-internal.h | 15 ++-------------
hw/arm/smmuv3.c | 41 ++++++++++++++++++++++++++++++++++------
3 files changed, 46 insertions(+), 23 deletions(-)
diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
index e4b477af10..3a2b06fd7f 100644
--- a/hw/arm/smmu-common.c
+++ b/hw/arm/smmu-common.c
@@ -307,7 +307,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
dma_addr_t baseaddr, indexmask;
int stage = cfg->stage;
SMMUTransTableInfo *tt = select_tt(cfg, iova);
- uint8_t level, granule_sz, inputsize, stride;
+ uint8_t level, granule_sz, inputsize, stride, oas;
if (!tt || tt->disabled) {
info->type = SMMU_PTW_ERR_TRANSLATION;
@@ -319,7 +319,12 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
inputsize = 64 - tt->tsz;
level = 4 - (inputsize - 4) / stride;
indexmask = SMMU_IDXMSK(inputsize, stride, level);
- baseaddr = extract64(tt->ttb, 0, 48);
+ oas = cfg->oas;
+ if (tt->granule_sz != 16) {
+ oas = MIN(oas, 48);
+ }
+
+ baseaddr = extract64(tt->ttb, 0, oas);
baseaddr &= ~indexmask;
while (level < SMMU_LEVELS) {
@@ -416,8 +421,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
* Get the ttb from concatenated structure.
* The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
*/
- uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
- idx * sizeof(uint64_t);
+ uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, cfg->s2cfg.oas) +
+ (1 << stride) * idx * sizeof(uint64_t);
dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
baseaddr &= ~indexmask;
diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
index 3388e1a5f8..25ae12fb5c 100644
--- a/hw/arm/smmuv3-internal.h
+++ b/hw/arm/smmuv3-internal.h
@@ -564,23 +564,12 @@ static inline int oas2bits(int oas_field)
return 44;
case 5:
return 48;
+ case 6:
+ return 52;
}
return -1;
}
-static inline int pa_range(STE *ste)
-{
- int oas_field = MIN(STE_S2PS(ste), SMMU_IDR5_OAS);
-
- if (!STE_S2AA64(ste)) {
- return 40;
- }
-
- return oas2bits(oas_field);
-}
-
-#define MAX_PA(ste) ((1 << pa_range(ste)) - 1)
-
/* CD fields */
#define CD_VALID(x) extract32((x)->word[0], 31, 1)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index 7297f6adc1..bc4ec202f4 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -238,6 +238,13 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
static void smmuv3_init_regs(SMMUv3State *s)
{
+ /*
+ * According to 6.3.6 SMMU_IDR5, OAS must match the system physical address
+ * size.
+ */
+ ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
+ uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
+
/**
* IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
* multi-level stream table
@@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
- s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
+ s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
s->cmdq.prod = 0;
@@ -374,6 +381,7 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
STE *ste, SMMUEventInfo *event)
{
uint32_t config;
+ uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
if (!STE_VALID(ste)) {
if (!event->inval_ste_allowed) {
@@ -450,7 +458,16 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
}
- cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
+ cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), oas));
+ /*
+ * For SMMUv3.1 and later, when OAS == IAS == 52, the stage 2 input
+ * range is further limited to 48 bits unless STE.S2TG indicates a
+ * 64KB granule.
+ */
+ if (cfg->s2cfg.granule_sz != 16) {
+ cfg->s2cfg.oas = MIN(cfg->s2cfg.oas, 48);
+ }
+
/*
* It is ILLEGAL for the address in S2TTB to be outside the range
* described by the effective S2PS value.
@@ -607,10 +624,12 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
return 0;
}
-static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
+static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg, CD *cd,
+ SMMUEventInfo *event)
{
int ret = -EINVAL;
int i;
+ uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
goto bad_cd;
@@ -630,7 +649,8 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
cfg->stage = 1;
cfg->oas = oas2bits(CD_IPS(cd));
- cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
+ cfg->oas = MIN(oas2bits(oas), cfg->oas);
+
cfg->tbi = CD_TBI(cd);
cfg->asid = CD_ASID(cd);
@@ -658,9 +678,18 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
goto bad_cd;
}
+ /*
+ * An address greater than 48 bits in size can only be output from a
+ * TTD when, in SMMUv3.1 and later, the effective IPS is 52 and a 64KB
+ * granule is in use for that translation table
+ */
+ if (tt->granule_sz != 16) {
+ oas = MIN(cfg->oas, 48);
+ }
+
tt->tsz = tsz;
tt->ttb = CD_TTB(cd, i);
- if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) {
+ if (tt->ttb & ~(MAKE_64BIT_MASK(0, oas))) {
goto bad_cd;
}
tt->had = CD_HAD(cd, i);
@@ -719,7 +748,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
return ret;
}
- return decode_cd(cfg, &cd, event);
+ return decode_cd(s, cfg, &cd, event);
}
/**
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-02-26 22:06 ` [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE Mostafa Saleh
@ 2023-03-20 17:12 ` Eric Auger
2023-03-21 13:06 ` Mostafa Saleh
0 siblings, 1 reply; 37+ messages in thread
From: Eric Auger @ 2023-03-20 17:12 UTC (permalink / raw)
To: Mostafa Saleh, qemu-devel
Cc: jean-philippe, peter.maydell, qemu-arm, richard.henderson
Hi Mostafa,
On 2/26/23 23:06, Mostafa Saleh wrote:
> OAS used to be hardcoded to 44 bits, however according to SMMU manual
> 6.3.6 SMMU_IDR5, OAS must match the system physical address size, so
> we read it from CPU PARANGE.
>
> Remove PA_MAX and pa_range as they were not used.
>
> Add SMMUv3State as an argument to decode_cd, so it can read the SMMU
> OAS.
>
> As CPU can use PARANGE with 52 bits, add 52 bits check to oas2bits,
> and cap OAS to 48 bits for stage-1 and stage-2 if granule is not 64KB
> as specified for SMMUv3.1 and later.
>
> Signed-off-by: Mostafa Saleh <smostafa@google.com>
> ---
> hw/arm/smmu-common.c | 13 +++++++++----
> hw/arm/smmuv3-internal.h | 15 ++-------------
> hw/arm/smmuv3.c | 41 ++++++++++++++++++++++++++++++++++------
> 3 files changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/hw/arm/smmu-common.c b/hw/arm/smmu-common.c
> index e4b477af10..3a2b06fd7f 100644
> --- a/hw/arm/smmu-common.c
> +++ b/hw/arm/smmu-common.c
> @@ -307,7 +307,7 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> dma_addr_t baseaddr, indexmask;
> int stage = cfg->stage;
> SMMUTransTableInfo *tt = select_tt(cfg, iova);
> - uint8_t level, granule_sz, inputsize, stride;
> + uint8_t level, granule_sz, inputsize, stride, oas;
>
> if (!tt || tt->disabled) {
> info->type = SMMU_PTW_ERR_TRANSLATION;
> @@ -319,7 +319,12 @@ static int smmu_ptw_64_s1(SMMUTransCfg *cfg,
> inputsize = 64 - tt->tsz;
> level = 4 - (inputsize - 4) / stride;
> indexmask = SMMU_IDXMSK(inputsize, stride, level);
> - baseaddr = extract64(tt->ttb, 0, 48);
> + oas = cfg->oas;
> + if (tt->granule_sz != 16) {
> + oas = MIN(oas, 48);
> + }
> +
> + baseaddr = extract64(tt->ttb, 0, oas);
> baseaddr &= ~indexmask;
>
> while (level < SMMU_LEVELS) {
> @@ -416,8 +421,8 @@ static int smmu_ptw_64_s2(SMMUTransCfg *cfg,
> * Get the ttb from concatenated structure.
> * The offset is the idx * size of each ttb(number of ptes * (sizeof(pte))
> */
> - uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, 48) + (1 << stride) *
> - idx * sizeof(uint64_t);
> + uint64_t baseaddr = extract64(cfg->s2cfg.vttb, 0, cfg->s2cfg.oas) +
> + (1 << stride) * idx * sizeof(uint64_t);
> dma_addr_t indexmask = SMMU_IDXMSK(inputsize, stride, level);
>
> baseaddr &= ~indexmask;
> diff --git a/hw/arm/smmuv3-internal.h b/hw/arm/smmuv3-internal.h
> index 3388e1a5f8..25ae12fb5c 100644
> --- a/hw/arm/smmuv3-internal.h
> +++ b/hw/arm/smmuv3-internal.h
> @@ -564,23 +564,12 @@ static inline int oas2bits(int oas_field)
> return 44;
> case 5:
> return 48;
> + case 6:
> + return 52;
> }
> return -1;
> }
>
> -static inline int pa_range(STE *ste)
> -{
> - int oas_field = MIN(STE_S2PS(ste), SMMU_IDR5_OAS);
> -
> - if (!STE_S2AA64(ste)) {
> - return 40;
> - }
> -
> - return oas2bits(oas_field);
> -}
> -
> -#define MAX_PA(ste) ((1 << pa_range(ste)) - 1)
> -
> /* CD fields */
>
> #define CD_VALID(x) extract32((x)->word[0], 31, 1)
> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
> index 7297f6adc1..bc4ec202f4 100644
> --- a/hw/arm/smmuv3.c
> +++ b/hw/arm/smmuv3.c
> @@ -238,6 +238,13 @@ void smmuv3_record_event(SMMUv3State *s, SMMUEventInfo *info)
>
> static void smmuv3_init_regs(SMMUv3State *s)
> {
> + /*
> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical address
> + * size.
> + */
> + ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> + uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
is this working in accelerated mode?
> +
> /**
> * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
> * multi-level stream table
> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
> + s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
I am not sure you can change that easily. In case of migration this is
going to change the behavior of the device, no?
Thanks
Eric
>
> s->cmdq.base = deposit64(s->cmdq.base, 0, 5, SMMU_CMDQS);
> s->cmdq.prod = 0;
> @@ -374,6 +381,7 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> STE *ste, SMMUEventInfo *event)
> {
> uint32_t config;
> + uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
>
> if (!STE_VALID(ste)) {
> if (!event->inval_ste_allowed) {
> @@ -450,7 +458,16 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
> }
>
>
> - cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), SMMU_IDR5_OAS));
> + cfg->s2cfg.oas = oas2bits(MIN(STE_S2PS(ste), oas));
> + /*
> + * For SMMUv3.1 and later, when OAS == IAS == 52, the stage 2 input
> + * range is further limited to 48 bits unless STE.S2TG indicates a
> + * 64KB granule.
> + */
> + if (cfg->s2cfg.granule_sz != 16) {
> + cfg->s2cfg.oas = MIN(cfg->s2cfg.oas, 48);
> + }
> +
> /*
> * It is ILLEGAL for the address in S2TTB to be outside the range
> * described by the effective S2PS value.
> @@ -607,10 +624,12 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste,
> return 0;
> }
>
> -static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> +static int decode_cd(SMMUv3State *s, SMMUTransCfg *cfg, CD *cd,
> + SMMUEventInfo *event)
> {
> int ret = -EINVAL;
> int i;
> + uint8_t oas = FIELD_EX32(s->idr[5], IDR5, OAS);
>
> if (!CD_VALID(cd) || !CD_AARCH64(cd)) {
> goto bad_cd;
> @@ -630,7 +649,8 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> cfg->stage = 1;
>
> cfg->oas = oas2bits(CD_IPS(cd));
> - cfg->oas = MIN(oas2bits(SMMU_IDR5_OAS), cfg->oas);
> + cfg->oas = MIN(oas2bits(oas), cfg->oas);
> +
> cfg->tbi = CD_TBI(cd);
> cfg->asid = CD_ASID(cd);
>
> @@ -658,9 +678,18 @@ static int decode_cd(SMMUTransCfg *cfg, CD *cd, SMMUEventInfo *event)
> goto bad_cd;
> }
>
> + /*
> + * An address greater than 48 bits in size can only be output from a
> + * TTD when, in SMMUv3.1 and later, the effective IPS is 52 and a 64KB
> + * granule is in use for that translation table
> + */
> + if (tt->granule_sz != 16) {
> + oas = MIN(cfg->oas, 48);
> + }
> +
> tt->tsz = tsz;
> tt->ttb = CD_TTB(cd, i);
> - if (tt->ttb & ~(MAKE_64BIT_MASK(0, cfg->oas))) {
> + if (tt->ttb & ~(MAKE_64BIT_MASK(0, oas))) {
> goto bad_cd;
> }
> tt->had = CD_HAD(cd, i);
> @@ -719,7 +748,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
> return ret;
> }
>
> - return decode_cd(cfg, &cd, event);
> + return decode_cd(s, cfg, &cd, event);
> }
>
> /**
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-20 17:12 ` Eric Auger
@ 2023-03-21 13:06 ` Mostafa Saleh
2023-03-21 13:23 ` Eric Auger
0 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-21 13:06 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Eric,
> > + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical address
> > + * size.
> > + */
> > + ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> > + uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
> is this working in accelerated mode?
I didn't try with accel, I will give it a try, but from what I see, that
ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
accel/kvm-all.c, so it seems this would work for accelerated mode.
> > +
> > /**
> > * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
> > * multi-level stream table
> > @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> > s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
> > s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
> > s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> > - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
> > + s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> I am not sure you can change that easily. In case of migration this is
> going to change the behavior of the device, no?
I see IDR registers are not migrated. I guess we can add them in a
subsection and if they were not passed (old instances) we set OAS to
44.
Maybe this should be another change outside of this series.
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-21 13:06 ` Mostafa Saleh
@ 2023-03-21 13:23 ` Eric Auger
2023-03-21 13:29 ` Mostafa Saleh
2023-03-21 13:34 ` Peter Maydell
0 siblings, 2 replies; 37+ messages in thread
From: Eric Auger @ 2023-03-21 13:23 UTC (permalink / raw)
To: Mostafa Saleh
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
Hi Mostafa,
On 3/21/23 14:06, Mostafa Saleh wrote:
> Hi Eric,
>
>>> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical address
>>> + * size.
>>> + */
>>> + ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
>>> + uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
>> is this working in accelerated mode?
> I didn't try with accel, I will give it a try, but from what I see, that
> ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> accel/kvm-all.c, so it seems this would work for accelerated mode.
yeah I ma not familiar enough with that code but it is worth to be checked.
>
>>> +
>>> /**
>>> * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
>>> * multi-level stream table
>>> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
>>> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
>>> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
>>> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
>>> - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
>>> + s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
>> I am not sure you can change that easily. In case of migration this is
>> going to change the behavior of the device, no?
> I see IDR registers are not migrated. I guess we can add them in a
> subsection and if they were not passed (old instances) we set OAS to
> 44.
> Maybe this should be another change outside of this series.
Indeed tehy are not migrated so it can lead to inconsistent behavior in
both source and dest. This deserves more analysis to me. In case you
would decide to migrate IDR regs this would need to be done in that
series I think. Migration must not be broken by this series.
Thanks
Eric
>
> Thanks,
> Mostafa
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-21 13:23 ` Eric Auger
@ 2023-03-21 13:29 ` Mostafa Saleh
2023-03-21 13:34 ` Eric Auger
2023-03-21 13:34 ` Peter Maydell
1 sibling, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-21 13:29 UTC (permalink / raw)
To: Eric Auger
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
On Tue, Mar 21, 2023 at 02:23:03PM +0100, Eric Auger wrote:
> >>> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> >>> - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
> >>> + s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> >> I am not sure you can change that easily. In case of migration this is
> >> going to change the behavior of the device, no?
> > I see IDR registers are not migrated. I guess we can add them in a
> > subsection and if they were not passed (old instances) we set OAS to
> > 44.
> > Maybe this should be another change outside of this series.
> Indeed tehy are not migrated so it can lead to inconsistent behavior in
> both source and dest. This deserves more analysis to me. In case you
> would decide to migrate IDR regs this would need to be done in that
> series I think. Migration must not be broken by this series
I agree, I meant to drop this patch from the series as it is not
really related to stage-2, and we can have another patch for this +
migration for IDR if needed after doing proper analysis.
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-21 13:29 ` Mostafa Saleh
@ 2023-03-21 13:34 ` Eric Auger
0 siblings, 0 replies; 37+ messages in thread
From: Eric Auger @ 2023-03-21 13:34 UTC (permalink / raw)
To: Mostafa Saleh
Cc: qemu-devel, jean-philippe, peter.maydell, qemu-arm,
richard.henderson
On 3/21/23 14:29, Mostafa Saleh wrote:
> On Tue, Mar 21, 2023 at 02:23:03PM +0100, Eric Auger wrote:
>>>>> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
>>>>> - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
>>>>> + s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
>>>> I am not sure you can change that easily. In case of migration this is
>>>> going to change the behavior of the device, no?
>>> I see IDR registers are not migrated. I guess we can add them in a
>>> subsection and if they were not passed (old instances) we set OAS to
>>> 44.
>>> Maybe this should be another change outside of this series.
>> Indeed tehy are not migrated so it can lead to inconsistent behavior in
>> both source and dest. This deserves more analysis to me. In case you
>> would decide to migrate IDR regs this would need to be done in that
>> series I think. Migration must not be broken by this series
> I agree, I meant to drop this patch from the series as it is not
> really related to stage-2, and we can have another patch for this +
> migration for IDR if needed after doing proper analysis.
Ah OK. I get it now. Yes this looks sensible
Thanks
Eric
>
> Thanks,
> Mostafa
>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-21 13:23 ` Eric Auger
2023-03-21 13:29 ` Mostafa Saleh
@ 2023-03-21 13:34 ` Peter Maydell
2023-03-21 13:42 ` Mostafa Saleh
` (2 more replies)
1 sibling, 3 replies; 37+ messages in thread
From: Peter Maydell @ 2023-03-21 13:34 UTC (permalink / raw)
To: eric.auger
Cc: Mostafa Saleh, qemu-devel, jean-philippe, qemu-arm,
richard.henderson
On Tue, 21 Mar 2023 at 13:23, Eric Auger <eric.auger@redhat.com> wrote:
>
> Hi Mostafa,
>
> On 3/21/23 14:06, Mostafa Saleh wrote:
> > Hi Eric,
> >
> >>> + * According to 6.3.6 SMMU_IDR5, OAS must match the system physical address
> >>> + * size.
> >>> + */
> >>> + ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> >>> + uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
> >> is this working in accelerated mode?
> > I didn't try with accel, I will give it a try, but from what I see, that
> > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > accel/kvm-all.c, so it seems this would work for accelerated mode.
>
> yeah I ma not familiar enough with that code but it is worth to be checked.
I'm a bit unsure about fishing around in the CPU ID registers for this.
That's not what you would do in real hardware, you'd just say "the
system is supposed to configure the CPU and the SMMU correctly".
Also, there is no guarantee that CPU 0 is related to this SMMU in
particular.
> >
> >>> +
> >>> /**
> >>> * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
> >>> * multi-level stream table
> >>> @@ -265,7 +272,7 @@ static void smmuv3_init_regs(SMMUv3State *s)
> >>> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN4K, 1);
> >>> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN16K, 1);
> >>> s->idr[5] = FIELD_DP32(s->idr[5], IDR5, GRAN64K, 1);
> >>> - s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, SMMU_IDR5_OAS); /* 44 bits */
> >>> + s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> >> I am not sure you can change that easily. In case of migration this is
> >> going to change the behavior of the device, no?
> > I see IDR registers are not migrated. I guess we can add them in a
> > subsection and if they were not passed (old instances) we set OAS to
> > 44.
> > Maybe this should be another change outside of this series.
> Indeed tehy are not migrated so it can lead to inconsistent behavior in
> both source and dest. This deserves more analysis to me. In case you
> would decide to migrate IDR regs this would need to be done in that
> series I think. Migration must not be broken by this series.
Jumping in here without having read much of the context, but why
would we need to migrate the ID registers? They are constant, read-only,
so they will be the same value on both source and destination.
thanks
-- PMM
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-21 13:34 ` Peter Maydell
@ 2023-03-21 13:42 ` Mostafa Saleh
2023-03-21 13:45 ` Eric Auger
2023-03-21 13:54 ` Mostafa Saleh
2 siblings, 0 replies; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-21 13:42 UTC (permalink / raw)
To: Peter Maydell
Cc: eric.auger, qemu-devel, jean-philippe, qemu-arm,
richard.henderson
Hi Peter,
On Tue, Mar 21, 2023 at 01:34:55PM +0000, Peter Maydell wrote:
> > >>> + s->idr[5] = FIELD_DP32(s->idr[5], IDR5, OAS, oas);
> > >> I am not sure you can change that easily. In case of migration this is
> > >> going to change the behavior of the device, no?
> > > I see IDR registers are not migrated. I guess we can add them in a
> > > subsection and if they were not passed (old instances) we set OAS to
> > > 44.
> > > Maybe this should be another change outside of this series.
> > Indeed tehy are not migrated so it can lead to inconsistent behavior in
> > both source and dest. This deserves more analysis to me. In case you
> > would decide to migrate IDR regs this would need to be done in that
> > series I think. Migration must not be broken by this series.
>
> Jumping in here without having read much of the context, but why
> would we need to migrate the ID registers? They are constant, read-only,
> so they will be the same value on both source and destination.
Currently OAS for SMMU is hardcoded to 44 bits, and the SMMU manual says
"OAS reflects the maximum usable PA output from the last stage of
AArch64 translations, and must match the system physical address size.
The OAS is discoverable from SMMU_IDR5.OAS."
This patch implements OAS based on CPU PARANGE, but this would break
migration from old instances that ran with 44 bits OAS to new code that
configures it based on the current CPU.
So one idea is to migrate the IDRs (or atleast IDR5).
Maybe that is not the best solution, we just need a way to know if the
old instance needs to be 44 bits or not.
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-21 13:34 ` Peter Maydell
2023-03-21 13:42 ` Mostafa Saleh
@ 2023-03-21 13:45 ` Eric Auger
2023-03-21 13:54 ` Mostafa Saleh
2 siblings, 0 replies; 37+ messages in thread
From: Eric Auger @ 2023-03-21 13:45 UTC (permalink / raw)
To: Peter Maydell
Cc: Mostafa Saleh, qemu-devel, jean-philippe, qemu-arm,
richard.henderson
Hi Peter,
On 3/21/23 14:34, Peter Maydell wrote:
> thout having read much of the context, but why
> would we need to migrate the ID registers? They are constant, read-only,
> so they will be the same value on both source and destination.
this series modifies the values of IDR[5] (oas). So my understanding is
the guest is likely to behave differently on src and dst, depending on
the qemu version, no?
Thanks
Eric
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-21 13:34 ` Peter Maydell
2023-03-21 13:42 ` Mostafa Saleh
2023-03-21 13:45 ` Eric Auger
@ 2023-03-21 13:54 ` Mostafa Saleh
2023-03-21 14:08 ` Peter Maydell
2 siblings, 1 reply; 37+ messages in thread
From: Mostafa Saleh @ 2023-03-21 13:54 UTC (permalink / raw)
To: Peter Maydell
Cc: eric.auger, qemu-devel, jean-philippe, qemu-arm,
richard.henderson
Hi Peter,
On Tue, Mar 21, 2023 at 01:34:55PM +0000, Peter Maydell wrote:
> > >>> + */
> > >>> + ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> > >>> + uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
> > >> is this working in accelerated mode?
> > > I didn't try with accel, I will give it a try, but from what I see, that
> > > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > > accel/kvm-all.c, so it seems this would work for accelerated mode.
> >
> > yeah I ma not familiar enough with that code but it is worth to be checked.
>
> I'm a bit unsure about fishing around in the CPU ID registers for this.
> That's not what you would do in real hardware, you'd just say "the
> system is supposed to configure the CPU and the SMMU correctly".
>
> Also, there is no guarantee that CPU 0 is related to this SMMU in
> particular.
Sorry, missed this point in last email.
So, we leave it this way, or there is a better way to discover PARANGE?
Thanks,
Mostafa
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE
2023-03-21 13:54 ` Mostafa Saleh
@ 2023-03-21 14:08 ` Peter Maydell
0 siblings, 0 replies; 37+ messages in thread
From: Peter Maydell @ 2023-03-21 14:08 UTC (permalink / raw)
To: Mostafa Saleh
Cc: eric.auger, qemu-devel, jean-philippe, qemu-arm,
richard.henderson
On Tue, 21 Mar 2023 at 13:55, Mostafa Saleh <smostafa@google.com> wrote:
>
> Hi Peter,
>
> On Tue, Mar 21, 2023 at 01:34:55PM +0000, Peter Maydell wrote:
> > > >>> + */
> > > >>> + ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
> > > >>> + uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
> > > >> is this working in accelerated mode?
> > > > I didn't try with accel, I will give it a try, but from what I see, that
> > > > ARM_CPU() is used to get the CPU in traget/arm/kvm.c which is used from
> > > > accel/kvm-all.c, so it seems this would work for accelerated mode.
> > >
> > > yeah I ma not familiar enough with that code but it is worth to be checked.
> >
> > I'm a bit unsure about fishing around in the CPU ID registers for this.
> > That's not what you would do in real hardware, you'd just say "the
> > system is supposed to configure the CPU and the SMMU correctly".
> >
> > Also, there is no guarantee that CPU 0 is related to this SMMU in
> > particular.
> Sorry, missed this point in last email.
>
> So, we leave it this way, or there is a better way to discover PARANGE?
If you really need to know it, put a QOM property on the SMMU device
and have the board code set it. (This is analogous to how it works
in hardware: there are tie-off signals on the SMMU for the OAS value.)
-- PMM
^ permalink raw reply [flat|nested] 37+ messages in thread
* [RFC PATCH v2 11/11] hw/arm/smmuv3: Add knob to choose translation stage and enable stage-2
2023-02-26 22:06 [RFC PATCH v2 00/11] Add stage-2 translation for SMMUv3 Mostafa Saleh
` (9 preceding siblings ...)
2023-02-26 22:06 ` [RFC PATCH v2 10/11] hw/arm/smmuv3: Populate OAS based on CPU PARANGE Mostafa Saleh
@ 2023-02-26 22:06 ` Mostafa Saleh
10 siblings, 0 replies; 37+ messages in thread
From: Mostafa Saleh @ 2023-02-26 22:06 UTC (permalink / raw)
To: qemu-devel
Cc: jean-philippe, eric.auger, peter.maydell, qemu-arm,
richard.henderson, Mostafa Saleh
As everything is in place, we can use a new system property to
advertise which stage is supported and remove bad_ste from STE
stage2 config.
The property added arm-smmuv3.stage can have 3 values:
- "1": Stage-1 only is advertised.
- "2": Stage-2 only is advertised.
- "all": Stage-1 + Stage-2 are supported, which is not implemented in
this patch series.
If not passed or an unsupported value is passed, it will default to
stage-1.
Advertise VMID16.
Don't try to decode CD, if stage-2 is configured.
Signed-off-by: Mostafa Saleh <smostafa@google.com>
---
Changes in v2:
- Squash knob patch with stage-2 enable patch.
- Don't try to decode CD, if stage-2 is configured.
---
hw/arm/smmuv3.c | 34 +++++++++++++++++++++++++---------
include/hw/arm/smmuv3.h | 1 +
2 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c
index bc4ec202f4..ebffc7e5f5 100644
--- a/hw/arm/smmuv3.c
+++ b/hw/arm/smmuv3.c
@@ -21,6 +21,7 @@
#include "hw/irq.h"
#include "hw/sysbus.h"
#include "migration/vmstate.h"
+#include "hw/qdev-properties.h"
#include "hw/qdev-core.h"
#include "hw/pci/pci.h"
#include "cpu.h"
@@ -245,14 +246,20 @@ static void smmuv3_init_regs(SMMUv3State *s)
ARMCPU *armcpu = ARM_CPU(qemu_get_cpu(0));
uint8_t oas = FIELD_EX64(armcpu->isar.id_aa64mmfr0, ID_AA64MMFR0, PARANGE);
- /**
- * IDR0: stage1 only, AArch64 only, coherent access, 16b ASID,
- * multi-level stream table
+ /*
+ * Based on sys property, the stages supported in smmu will be advertised.
+ * At the moment "all" is not supported and default to stage-1.
*/
- s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1); /* stage 1 supported */
+ if (s->stage && !strcmp("2", s->stage)) {
+ s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S2P, 1);
+ } else {
+ s->idr[0] = FIELD_DP32(s->idr[0], IDR0, S1P, 1);
+ }
+
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTF, 2); /* AArch64 PTW only */
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, COHACC, 1); /* IO coherent */
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, ASID16, 1); /* 16-bit ASID */
+ s->idr[0] = FIELD_DP32(s->idr[0], IDR0, VMID16, 1); /* 16-bit VMID */
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, TTENDIAN, 2); /* little endian */
s->idr[0] = FIELD_DP32(s->idr[0], IDR0, STALL_MODEL, 1); /* No stall */
/* terminated transaction will always be aborted/error returned */
@@ -508,10 +515,6 @@ static int decode_ste(SMMUv3State *s, SMMUTransCfg *cfg,
qemu_log_mask(LOG_UNIMP, "SMMUv3 Stall not implemented!\n");
goto bad_ste;
}
-
- /* This is still here as stage 2 has not been fully enabled yet. */
- qemu_log_mask(LOG_UNIMP, "SMMUv3 does not support stage 2 yet\n");
- goto bad_ste;
}
if (STE_S1CDMAX(ste) != 0) {
@@ -739,7 +742,7 @@ static int smmuv3_decode_config(IOMMUMemoryRegion *mr, SMMUTransCfg *cfg,
return ret;
}
- if (cfg->aborted || cfg->bypassed) {
+ if (cfg->aborted || cfg->bypassed || (cfg->stage == 2)) {
return 0;
}
@@ -1801,6 +1804,18 @@ static const VMStateDescription vmstate_smmuv3 = {
}
};
+static Property smmuv3_properties[] = {
+ /*
+ * Stages of translation advertised.
+ * "1": Stage 1
+ * "2": Stage 2
+ * "all": Stage 1 + Stage 2
+ * Defaults to stage 1
+ */
+ DEFINE_PROP_STRING("stage", SMMUv3State, stage),
+ DEFINE_PROP_END_OF_LIST()
+};
+
static void smmuv3_instance_init(Object *obj)
{
/* Nothing much to do here as of now */
@@ -1817,6 +1832,7 @@ static void smmuv3_class_init(ObjectClass *klass, void *data)
&c->parent_phases);
c->parent_realize = dc->realize;
dc->realize = smmu_realize;
+ device_class_set_props(dc, smmuv3_properties);
}
static int smmuv3_notify_flag_changed(IOMMUMemoryRegion *iommu,
diff --git a/include/hw/arm/smmuv3.h b/include/hw/arm/smmuv3.h
index 6031d7d325..d183a62766 100644
--- a/include/hw/arm/smmuv3.h
+++ b/include/hw/arm/smmuv3.h
@@ -62,6 +62,7 @@ struct SMMUv3State {
qemu_irq irq[4];
QemuMutex mutex;
+ char *stage;
};
typedef enum {
--
2.39.2.637.g21b0678d19-goog
^ permalink raw reply related [flat|nested] 37+ messages in thread