| Message ID | 20240701110241.2005222-19-smostafa@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | SMMUv3 nested translation support | expand |
On Mon, Jul 01, 2024 at 11:02:40AM +0000, Mostafa Saleh wrote: > QEMU doesn's support memory attributes, so FWB is NOP, this > might change in the future if memory attributre would be supported. > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > --- > hw/arm/smmuv3.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > index 807f26f2da..88378e83dd 100644 > --- a/hw/arm/smmuv3.c > +++ b/hw/arm/smmuv3.c > @@ -287,6 +287,14 @@ static void smmuv3_init_regs(SMMUv3State *s) > if (FIELD_EX32(s->idr[0], IDR0, S2P)) { > /* XNX is a stage-2-specific feature */ > s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1); > + if (FIELD_EX32(s->idr[0], IDR0, S1P)) { Why is this check needed? > + /* > + * QEMU doesn's support memory attributes, so FWB is NOP, this doesn't Thanks, Jean > + * might change in the future if memory attributre would be > + * supported. > + */ > + s->idr[3] = FIELD_DP32(s->idr[3], IDR3, FWB, 1); > + } > } > s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1); > s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2); > -- > 2.45.2.803.g4e1b14247a-goog >
Hi Mostafa, On 7/4/24 20:36, Jean-Philippe Brucker wrote: > On Mon, Jul 01, 2024 at 11:02:40AM +0000, Mostafa Saleh wrote: >> QEMU doesn's support memory attributes, so FWB is NOP, this >> might change in the future if memory attributre would be supported. attributes here and below as reported along with v3 >> >> Signed-off-by: Mostafa Saleh <smostafa@google.com> >> --- >> hw/arm/smmuv3.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c >> index 807f26f2da..88378e83dd 100644 >> --- a/hw/arm/smmuv3.c >> +++ b/hw/arm/smmuv3.c >> @@ -287,6 +287,14 @@ static void smmuv3_init_regs(SMMUv3State *s) >> if (FIELD_EX32(s->idr[0], IDR0, S2P)) { >> /* XNX is a stage-2-specific feature */ >> s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1); >> + if (FIELD_EX32(s->idr[0], IDR0, S1P)) { > Why is this check needed? > >> + /* >> + * QEMU doesn's support memory attributes, so FWB is NOP, this > doesn't I have just seen your reply on my v3 comments. I still do not understand why we expose this bit at this stage. Thanks Eric > > Thanks, > Jean > >> + * might change in the future if memory attributre would be >> + * supported. >> + */ >> + s->idr[3] = FIELD_DP32(s->idr[3], IDR3, FWB, 1); >> + } >> } >> s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1); >> s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2); >> -- >> 2.45.2.803.g4e1b14247a-goog >>
Hi Jean, On Thu, Jul 04, 2024 at 07:36:58PM +0100, Jean-Philippe Brucker wrote: > On Mon, Jul 01, 2024 at 11:02:40AM +0000, Mostafa Saleh wrote: > > QEMU doesn's support memory attributes, so FWB is NOP, this > > might change in the future if memory attributre would be supported. > > > > Signed-off-by: Mostafa Saleh <smostafa@google.com> > > --- > > hw/arm/smmuv3.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > > index 807f26f2da..88378e83dd 100644 > > --- a/hw/arm/smmuv3.c > > +++ b/hw/arm/smmuv3.c > > @@ -287,6 +287,14 @@ static void smmuv3_init_regs(SMMUv3State *s) > > if (FIELD_EX32(s->idr[0], IDR0, S2P)) { > > /* XNX is a stage-2-specific feature */ > > s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1); > > + if (FIELD_EX32(s->idr[0], IDR0, S1P)) { > > Why is this check needed? > I thought that only made sense only for nested SMMUs, but I guess in practice it’s not important for qemu and Linux doesn’t use it, I can just drop this patch. Thanks, Mostafa > > > + /* > > + * QEMU doesn's support memory attributes, so FWB is NOP, this > > doesn't > > Thanks, > Jean > > > + * might change in the future if memory attributre would be > > + * supported. > > + */ > > + s->idr[3] = FIELD_DP32(s->idr[3], IDR3, FWB, 1); > > + } > > } > > s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1); > > s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2); > > -- > > 2.45.2.803.g4e1b14247a-goog > >
Hi Eric, On Mon, Jul 08, 2024 at 07:09:02PM +0200, Eric Auger wrote: > Hi Mostafa, > > On 7/4/24 20:36, Jean-Philippe Brucker wrote: > > On Mon, Jul 01, 2024 at 11:02:40AM +0000, Mostafa Saleh wrote: > >> QEMU doesn's support memory attributes, so FWB is NOP, this > >> might change in the future if memory attributre would be supported. > attributes here and below as reported along with v3 > >> > >> Signed-off-by: Mostafa Saleh <smostafa@google.com> > >> --- > >> hw/arm/smmuv3.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c > >> index 807f26f2da..88378e83dd 100644 > >> --- a/hw/arm/smmuv3.c > >> +++ b/hw/arm/smmuv3.c > >> @@ -287,6 +287,14 @@ static void smmuv3_init_regs(SMMUv3State *s) > >> if (FIELD_EX32(s->idr[0], IDR0, S2P)) { > >> /* XNX is a stage-2-specific feature */ > >> s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1); > >> + if (FIELD_EX32(s->idr[0], IDR0, S1P)) { > > Why is this check needed? > > > >> + /* > >> + * QEMU doesn's support memory attributes, so FWB is NOP, this > > doesn't > I have just seen your reply on my v3 comments. I still do not understand > why we expose this bit at this stage. As I replied to Jean, I will drop this patch for now, we can always add it later, as it doens't add much value. Thanks, Mostafa > > Thanks > > Eric > > > > Thanks, > > Jean > > > >> + * might change in the future if memory attributre would be > >> + * supported. > >> + */ > >> + s->idr[3] = FIELD_DP32(s->idr[3], IDR3, FWB, 1); > >> + } > >> } > >> s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1); > >> s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2); > >> -- > >> 2.45.2.803.g4e1b14247a-goog > >> >
diff --git a/hw/arm/smmuv3.c b/hw/arm/smmuv3.c index 807f26f2da..88378e83dd 100644 --- a/hw/arm/smmuv3.c +++ b/hw/arm/smmuv3.c @@ -287,6 +287,14 @@ static void smmuv3_init_regs(SMMUv3State *s) if (FIELD_EX32(s->idr[0], IDR0, S2P)) { /* XNX is a stage-2-specific feature */ s->idr[3] = FIELD_DP32(s->idr[3], IDR3, XNX, 1); + if (FIELD_EX32(s->idr[0], IDR0, S1P)) { + /* + * QEMU doesn's support memory attributes, so FWB is NOP, this + * might change in the future if memory attributre would be + * supported. + */ + s->idr[3] = FIELD_DP32(s->idr[3], IDR3, FWB, 1); + } } s->idr[3] = FIELD_DP32(s->idr[3], IDR3, RIL, 1); s->idr[3] = FIELD_DP32(s->idr[3], IDR3, BBML, 2);
QEMU doesn's support memory attributes, so FWB is NOP, this might change in the future if memory attributre would be supported. Signed-off-by: Mostafa Saleh <smostafa@google.com> --- hw/arm/smmuv3.c | 8 ++++++++ 1 file changed, 8 insertions(+)