| Message ID | 1575467748-28898-2-git-send-email-sveith@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/arm/smmuv3: Correct stream ID and event address handling | expand |
Hi Simon, On 12/4/19 2:55 PM, Simon Veith wrote: > In the SMMU_STRTAB_BASE register, the stream table base address only > occupies bits [51:6]. Other bits, such as RA (bit [62]), must be masked > out to obtain the base address. > > The branch for 2-level stream tables correctly applies this mask by way > of SMMU_BASE_ADDR_MASK, but the one for linear stream tables does not. > > Apply the missing mask in that case as well so that the correct stream > base address is used by guests which configure a linear stream table. > > Linux guests are unaffected by this change because they choose a 2-level > stream table layout for the QEMU SMMUv3, based on the size of its stream > ID space. > > ref. ARM IHI 0070C, section 6.3.23. > > Signed-off-by: Simon Veith <sveith@amazon.de> > Cc: Eric Auger <eric.auger@redhat.com> > Cc: qemu-devel@nongnu.org > Cc: qemu-arm@nongnu.org > --- > hw/arm/smmuv3.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index e2fbb83..eef9a18 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -429,7 +429,7 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, > } > addr = l2ptr + l2_ste_offset * sizeof(*ste); > } else { > - addr = s->strtab_base + sid * sizeof(*ste); > + addr = (s->strtab_base & SMMU_BASE_ADDR_MASK) + sid * sizeof(*ste); Not related to this patch but I noticed SMMU_BASE_ADDR_MASK should be 0xffffffffffc0 and not 0xffffffffffe0. I can fix it separately or if you respin, you may fix it as well? > } > > if (smmu_get_ste(s, addr, ste, event)) { > Besides Acked-by: Eric Auger <eric.auger@redhat.com> Thanks Eric
Hello Eric, On 05/12/2019 09:42, Auger Eric wrote: > Not related to this patch but I noticed SMMU_BASE_ADDR_MASK should be > 0xffffffffffc0 and not 0xffffffffffe0. I can fix it separately or if you > respin, you may fix it as well? Good catch, thank you. I'll fix it in the next version. Looking at the upper end of that mask, it seems that we are currently masking out bits 48 through 63, rather than just 51 through 63. The reference manual says that this should be done to match the system physical address size as given by SMMU_IDR5.OAS. We define SMMU_IDR5_OAS to be 4, which selects a physical address size of 44 bits (ref. section 6.3.6). I think the mask should therefore be 0xfffffffffc0 to clear bits 44 and above. Do you agree? Ideally, we would derive this mask from our definition of SMMU_IDR5_OAS, but I'm not sure it's okay to stuff a call to oas2bits() into the SMMU_BASE_ADDR_MASK macro. Regards Simon Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
Hi Simon, On 12/5/19 11:04 PM, Simon Veith wrote: > Hello Eric, > > On 05/12/2019 09:42, Auger Eric wrote: >> Not related to this patch but I noticed SMMU_BASE_ADDR_MASK should be >> 0xffffffffffc0 and not 0xffffffffffe0. I can fix it separately or if you >> respin, you may fix it as well? > > Good catch, thank you. I'll fix it in the next version. > > Looking at the upper end of that mask, it seems that we are currently masking out bits 48 through 63, rather than just 51 through 63. > The reference manual says that this should be done to match the system physical address size as given by SMMU_IDR5.OAS. Yes you're right. This should go up to 51 as per the field range definition. Spec says address bits and above this field range are treated as zero. > > We define SMMU_IDR5_OAS to be 4, which selects a physical address size of 44 bits (ref. section 6.3.6). I think the mask should therefore be 0xfffffffffc0 to clear bits 44 and above. Do you agree? bits beyond the OAS are RES0. The spec does not says those fields are treated as zero, as explicitly mentioned for bits > 51. Normally the guest should not set them to something != 0, this would be a programming error right? Guest is supposed to read the IDR5 and program accordingly? > > Ideally, we would derive this mask from our definition of SMMU_IDR5_OAS, but I'm not sure it's okay to stuff a call to oas2bits() into the SMMU_BASE_ADDR_MASK macro. Well I am not sure this is worth the candle. I am not sure we are obliged to enforce this. Thanks Eric > > Regards > Simon > > > > Amazon Development Center Germany GmbH > Krausenstr. 38 > 10117 Berlin > Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich > Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B > Sitz: Berlin > Ust-ID: DE 289 237 879 > >
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index e2fbb83..eef9a18 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -429,7 +429,7 @@ static int smmu_find_ste(SMMUv3State *s, uint32_t sid, STE *ste, } addr = l2ptr + l2_ste_offset * sizeof(*ste); } else { - addr = s->strtab_base + sid * sizeof(*ste); + addr = (s->strtab_base & SMMU_BASE_ADDR_MASK) + sid * sizeof(*ste); } if (smmu_get_ste(s, addr, ste, event)) {
In the SMMU_STRTAB_BASE register, the stream table base address only occupies bits [51:6]. Other bits, such as RA (bit [62]), must be masked out to obtain the base address. The branch for 2-level stream tables correctly applies this mask by way of SMMU_BASE_ADDR_MASK, but the one for linear stream tables does not. Apply the missing mask in that case as well so that the correct stream base address is used by guests which configure a linear stream table. Linux guests are unaffected by this change because they choose a 2-level stream table layout for the QEMU SMMUv3, based on the size of its stream ID space. ref. ARM IHI 0070C, section 6.3.23. Signed-off-by: Simon Veith <sveith@amazon.de> Cc: Eric Auger <eric.auger@redhat.com> Cc: qemu-devel@nongnu.org Cc: qemu-arm@nongnu.org --- hw/arm/smmuv3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)