| Message ID | 20190514121132.26732-2-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: TLB flush helpers rework | expand |
On Tue, 14 May 2019, Julien Grall wrote: > The AIVIVT is a type of instruction cache available on Armv7. This is > the only cache not implementing the IVIPT extension and therefore > requiring specific care. > > To simplify maintenance requirements, Xen will not boot on platform > using AIVIVT cache. > > This should not be an issue because Xen Arm32 can only boot on a small > number of processors (see arch/arm/arm32/proc-v7.S). All of them are > not using AIVIVT cache. > > Signed-off-by: Julien Grall <julien.grall@arm.com> As we have already discussed, I am OK with this and neither of us foresee any issues. Given that it could be considered as a drop in support for something, I think it would be nice to send an email outside of the series to say we won't support AIVIVT processors any longer, using words easier to understand to users (not necessarily developers). Would you be able to do that? I can help you with the text. > --- > > Changes in v3: > - Patch added > --- > xen/arch/arm/setup.c | 5 +++++ > xen/include/asm-arm/processor.h | 5 +++++ > 2 files changed, 10 insertions(+) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index ccb0f181ea..faaf029b99 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -526,10 +526,15 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size) > unsigned long boot_mfn_start, boot_mfn_end; > int i; > void *fdt; > + const uint32_t ctr = READ_CP32(CTR); > > if ( !bootinfo.mem.nr_banks ) > panic("No memory bank\n"); > > + /* We only supports instruction caches implementing the IVIPT extension. */ Please mention that IVIPT can only be implemented by PIPT and VIPT caches, not by AIVIVT caches. That should make it straightforward to understand the reason for the panic below. > + if ( ((ctr >> CTR_L1Ip_SHIFT) & CTR_L1Ip_MASK) == CTR_L1Ip_AIVIVT ) > + panic("AIVIVT instruction cache not supported\n"); > + > init_pdx(); > > ram_start = bootinfo.mem.bank[0].start; > diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h > index b5f515805d..04b05b3f39 100644 > --- a/xen/include/asm-arm/processor.h > +++ b/xen/include/asm-arm/processor.h > @@ -6,6 +6,11 @@ > #endif > #include <public/arch-arm.h> > > +/* CTR Cache Type Register */ > +#define CTR_L1Ip_MASK 0x3 > +#define CTR_L1Ip_SHIFT 14 > +#define CTR_L1Ip_AIVIVT 0x1 > + > /* MIDR Main ID Register */ > #define MIDR_REVISION_MASK 0xf > #define MIDR_RESIVION(midr) ((midr) & MIDR_REVISION_MASK) > -- > 2.11.0 >
Hi, On 20/05/2019 19:56, Stefano Stabellini wrote: > On Tue, 14 May 2019, Julien Grall wrote: >> The AIVIVT is a type of instruction cache available on Armv7. This is >> the only cache not implementing the IVIPT extension and therefore >> requiring specific care. >> >> To simplify maintenance requirements, Xen will not boot on platform >> using AIVIVT cache. >> >> This should not be an issue because Xen Arm32 can only boot on a small >> number of processors (see arch/arm/arm32/proc-v7.S). All of them are >> not using AIVIVT cache. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > As we have already discussed, I am OK with this and neither of us > foresee any issues. Given that it could be considered as a drop in > support for something, I think it would be nice to send an email outside > of the series to say we won't support AIVIVT processors any longer, > using words easier to understand to users (not necessarily developers). Users of what? Xen upstream will *panic* on every processor not listed in arch/arm/arm32/proc-v7.S even without this patch. > Would you be able to do that? I can help you with the text. While in theory this sounds sensible, for reaching the panic added in this patch, you would need out-of-tree patches. So in practice you are saying we should care about out-of-tree users. I have already enough to care about Xen upstream itself that out-of-tree is my last concern. If someone were using out-of-tree then then too bad they will see the panic. TBH, I am pretty sure we don't currently properly follow the maintenance requirements... So we are making them a favor to add a panic. Before they could just see random corruption... Anyway, feel free to send the message yourself. > > >> --- >> >> Changes in v3: >> - Patch added >> --- >> xen/arch/arm/setup.c | 5 +++++ >> xen/include/asm-arm/processor.h | 5 +++++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index ccb0f181ea..faaf029b99 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -526,10 +526,15 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size) >> unsigned long boot_mfn_start, boot_mfn_end; >> int i; >> void *fdt; >> + const uint32_t ctr = READ_CP32(CTR); >> >> if ( !bootinfo.mem.nr_banks ) >> panic("No memory bank\n"); >> >> + /* We only supports instruction caches implementing the IVIPT extension. */ > > Please mention that IVIPT can only be implemented by PIPT and VIPT > caches, not by AIVIVT caches. That should make it straightforward to > understand the reason for the panic below. I would prefer to add "This is not the case of AIVIVT" rather than spelling out the other caches. Cheers, -- Julien Grall
Gentle ping. On 20/05/2019 20:53, Julien Grall wrote: > Hi, > > On 20/05/2019 19:56, Stefano Stabellini wrote: >> On Tue, 14 May 2019, Julien Grall wrote: >>> The AIVIVT is a type of instruction cache available on Armv7. This is >>> the only cache not implementing the IVIPT extension and therefore >>> requiring specific care. >>> >>> To simplify maintenance requirements, Xen will not boot on platform >>> using AIVIVT cache. >>> >>> This should not be an issue because Xen Arm32 can only boot on a small >>> number of processors (see arch/arm/arm32/proc-v7.S). All of them are >>> not using AIVIVT cache. >>> >>> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> As we have already discussed, I am OK with this and neither of us >> foresee any issues. Given that it could be considered as a drop in >> support for something, I think it would be nice to send an email outside >> of the series to say we won't support AIVIVT processors any longer, >> using words easier to understand to users (not necessarily developers). > > Users of what? Xen upstream will *panic* on every processor not listed in > arch/arm/arm32/proc-v7.S even without this patch. > >> Would you be able to do that? I can help you with the text. > While in theory this sounds sensible, for reaching the panic added in this > patch, you would need out-of-tree patches. So in practice you are saying we > should care about out-of-tree users. > > I have already enough to care about Xen upstream itself that out-of-tree is my > last concern. If someone were using out-of-tree then then too bad they will see > the panic. > > TBH, I am pretty sure we don't currently properly follow the maintenance > requirements... So we are making them a favor to add a panic. Before they could > just see random corruption... > > Anyway, feel free to send the message yourself. > >> >> >>> --- >>> >>> Changes in v3: >>> - Patch added >>> --- >>> xen/arch/arm/setup.c | 5 +++++ >>> xen/include/asm-arm/processor.h | 5 +++++ >>> 2 files changed, 10 insertions(+) >>> >>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >>> index ccb0f181ea..faaf029b99 100644 >>> --- a/xen/arch/arm/setup.c >>> +++ b/xen/arch/arm/setup.c >>> @@ -526,10 +526,15 @@ static void __init setup_mm(unsigned long dtb_paddr, >>> size_t dtb_size) >>> unsigned long boot_mfn_start, boot_mfn_end; >>> int i; >>> void *fdt; >>> + const uint32_t ctr = READ_CP32(CTR); >>> if ( !bootinfo.mem.nr_banks ) >>> panic("No memory bank\n"); >>> + /* We only supports instruction caches implementing the IVIPT extension. */ >> >> Please mention that IVIPT can only be implemented by PIPT and VIPT >> caches, not by AIVIVT caches. That should make it straightforward to >> understand the reason for the panic below. > > I would prefer to add "This is not the case of AIVIVT" rather than spelling out > the other caches. > > Cheers, > >
(+ Committers) Ping again... I have quite a few patches blocked on this work. Cheers, On 29/05/2019 17:44, Julien Grall wrote: > Gentle ping. > > On 20/05/2019 20:53, Julien Grall wrote: >> Hi, >> >> On 20/05/2019 19:56, Stefano Stabellini wrote: >>> On Tue, 14 May 2019, Julien Grall wrote: >>>> The AIVIVT is a type of instruction cache available on Armv7. This is >>>> the only cache not implementing the IVIPT extension and therefore >>>> requiring specific care. >>>> >>>> To simplify maintenance requirements, Xen will not boot on platform >>>> using AIVIVT cache. >>>> >>>> This should not be an issue because Xen Arm32 can only boot on a small >>>> number of processors (see arch/arm/arm32/proc-v7.S). All of them are >>>> not using AIVIVT cache. >>>> >>>> Signed-off-by: Julien Grall <julien.grall@arm.com> >>> >>> As we have already discussed, I am OK with this and neither of us >>> foresee any issues. Given that it could be considered as a drop in >>> support for something, I think it would be nice to send an email outside >>> of the series to say we won't support AIVIVT processors any longer, >>> using words easier to understand to users (not necessarily developers). >> >> Users of what? Xen upstream will *panic* on every processor not listed in >> arch/arm/arm32/proc-v7.S even without this patch. >> >>> Would you be able to do that? I can help you with the text. >> While in theory this sounds sensible, for reaching the panic added in this >> patch, you would need out-of-tree patches. So in practice you are saying we >> should care about out-of-tree users. >> >> I have already enough to care about Xen upstream itself that out-of-tree is my >> last concern. If someone were using out-of-tree then then too bad they will >> see the panic. >> >> TBH, I am pretty sure we don't currently properly follow the maintenance >> requirements... So we are making them a favor to add a panic. Before they >> could just see random corruption... >> >> Anyway, feel free to send the message yourself. >> >>> >>> >>>> --- >>>> >>>> Changes in v3: >>>> - Patch added >>>> --- >>>> xen/arch/arm/setup.c | 5 +++++ >>>> xen/include/asm-arm/processor.h | 5 +++++ >>>> 2 files changed, 10 insertions(+) >>>> >>>> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >>>> index ccb0f181ea..faaf029b99 100644 >>>> --- a/xen/arch/arm/setup.c >>>> +++ b/xen/arch/arm/setup.c >>>> @@ -526,10 +526,15 @@ static void __init setup_mm(unsigned long dtb_paddr, >>>> size_t dtb_size) >>>> unsigned long boot_mfn_start, boot_mfn_end; >>>> int i; >>>> void *fdt; >>>> + const uint32_t ctr = READ_CP32(CTR); >>>> if ( !bootinfo.mem.nr_banks ) >>>> panic("No memory bank\n"); >>>> + /* We only supports instruction caches implementing the IVIPT >>>> extension. */ >>> >>> Please mention that IVIPT can only be implemented by PIPT and VIPT >>> caches, not by AIVIVT caches. That should make it straightforward to >>> understand the reason for the panic below. >> >> I would prefer to add "This is not the case of AIVIVT" rather than spelling >> out the other caches. >> >> Cheers, >> >> >
Hi Julien, I expressed my preference below. We don't agree. Is there anything else you would like me to add to this thread? Do you have a specific question? The only question I see below is "Users of what?" but I take it was just rhetorical. On Mon, 10 Jun 2019, Julien Grall wrote: > (+ Committers) > > Ping again... I have quite a few patches blocked on this work. > > Cheers, > > On 29/05/2019 17:44, Julien Grall wrote: > > Gentle ping. > > > > On 20/05/2019 20:53, Julien Grall wrote: > > > Hi, > > > > > > On 20/05/2019 19:56, Stefano Stabellini wrote: > > > > On Tue, 14 May 2019, Julien Grall wrote: > > > > > The AIVIVT is a type of instruction cache available on Armv7. This is > > > > > the only cache not implementing the IVIPT extension and therefore > > > > > requiring specific care. > > > > > > > > > > To simplify maintenance requirements, Xen will not boot on platform > > > > > using AIVIVT cache. > > > > > > > > > > This should not be an issue because Xen Arm32 can only boot on a small > > > > > number of processors (see arch/arm/arm32/proc-v7.S). All of them are > > > > > not using AIVIVT cache. > > > > > > > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > > > > > As we have already discussed, I am OK with this and neither of us > > > > foresee any issues. Given that it could be considered as a drop in > > > > support for something, I think it would be nice to send an email outside > > > > of the series to say we won't support AIVIVT processors any longer, > > > > using words easier to understand to users (not necessarily developers). > > > > > > Users of what? Xen upstream will *panic* on every processor not listed in > > > arch/arm/arm32/proc-v7.S even without this patch. > > > > > > > Would you be able to do that? I can help you with the text. > > > While in theory this sounds sensible, for reaching the panic added in this > > > patch, you would need out-of-tree patches. So in practice you are saying > > > we should care about out-of-tree users. > > > > > > I have already enough to care about Xen upstream itself that out-of-tree > > > is my last concern. If someone were using out-of-tree then then too bad > > > they will see the panic. > > > > > > TBH, I am pretty sure we don't currently properly follow the maintenance > > > requirements... So we are making them a favor to add a panic. Before they > > > could just see random corruption... > > > > > > Anyway, feel free to send the message yourself. > > > > > > > > > > > > > > > > --- > > > > > > > > > > Changes in v3: > > > > > - Patch added > > > > > --- > > > > > xen/arch/arm/setup.c | 5 +++++ > > > > > xen/include/asm-arm/processor.h | 5 +++++ > > > > > 2 files changed, 10 insertions(+) > > > > > > > > > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > > > > > index ccb0f181ea..faaf029b99 100644 > > > > > --- a/xen/arch/arm/setup.c > > > > > +++ b/xen/arch/arm/setup.c > > > > > @@ -526,10 +526,15 @@ static void __init setup_mm(unsigned long > > > > > dtb_paddr, size_t dtb_size) > > > > > unsigned long boot_mfn_start, boot_mfn_end; > > > > > int i; > > > > > void *fdt; > > > > > + const uint32_t ctr = READ_CP32(CTR); > > > > > if ( !bootinfo.mem.nr_banks ) > > > > > panic("No memory bank\n"); > > > > > + /* We only supports instruction caches implementing the IVIPT > > > > > extension. */ > > > > > > > > Please mention that IVIPT can only be implemented by PIPT and VIPT > > > > caches, not by AIVIVT caches. That should make it straightforward to > > > > understand the reason for the panic below. > > > > > > I would prefer to add "This is not the case of AIVIVT" rather than > > > spelling out the other caches. > > > > > > Cheers, > > > > > > > > > > -- > Julien Grall >
On 6/10/19 9:40 PM, Stefano Stabellini wrote: > Hi Julien, Hi Stefano, > > I expressed my preference below. We don't agree. Is there anything else > you would like me to add to this thread? Do you have a specific > question? The only question I see below is "Users of what?" but I take > it was just rhetorical. No it wasn't rhetorical. It was a genuine question, because you are implying that: 1) It is possible to have user that are using AIVIVT 2) We have to support out of tree users The latter is particularly critical as this implies that any change in Xen should be done with keeping in mind any patches that could be applied on top of Xen. So I am all hear of your arguments here... At the end, we need to come to an agreement here as at the moment my patch can't go without your ack. Cheers,
On Mon, 10 Jun 2019, Julien Grall wrote: > On 6/10/19 9:40 PM, Stefano Stabellini wrote: > > Hi Julien, > > Hi Stefano, > > > > > I expressed my preference below. We don't agree. Is there anything else > > you would like me to add to this thread? Do you have a specific > > question? The only question I see below is "Users of what?" but I take > > it was just rhetorical. > > No it wasn't rhetorical. It was a genuine question, because you are implying > that: > 1) It is possible to have user that are using AIVIVT > 2) We have to support out of tree users > > The latter is particularly critical as this implies that any change in Xen > should be done with keeping in mind any patches that could be applied on top > of Xen. > > So I am all hear of your arguments here... At the end, we need to come to an > agreement here as at the moment my patch can't go without your ack. No, we don't have to support out of tree users. I didn't mean to imply it. But it costs us very little to be courteous and polite in cases like this, sending a more obvious [ANNOUNCE] email saying "we are dropping AIVIVT as nobody should be using it". Can this patch go in regardless? I wouldn't be happy about it, but if this was a vote it would be a -1, not a -2. It is difficult to give an ack for a thing I don't like, but I wouldn't go as far as nacking it.
On Mon, 10 Jun 2019, Stefano Stabellini wrote: > On Mon, 10 Jun 2019, Julien Grall wrote: > > On 6/10/19 9:40 PM, Stefano Stabellini wrote: > > > Hi Julien, > > > > Hi Stefano, > > > > > > > > I expressed my preference below. We don't agree. Is there anything else > > > you would like me to add to this thread? Do you have a specific > > > question? The only question I see below is "Users of what?" but I take > > > it was just rhetorical. > > > > No it wasn't rhetorical. It was a genuine question, because you are implying > > that: > > 1) It is possible to have user that are using AIVIVT > > 2) We have to support out of tree users > > > > The latter is particularly critical as this implies that any change in Xen > > should be done with keeping in mind any patches that could be applied on top > > of Xen. > > > > So I am all hear of your arguments here... At the end, we need to come to an > > agreement here as at the moment my patch can't go without your ack. > > No, we don't have to support out of tree users. I didn't mean to imply > it. But it costs us very little to be courteous and polite in cases like > this, sending a more obvious [ANNOUNCE] email saying "we are dropping > AIVIVT as nobody should be using it". > > Can this patch go in regardless? I wouldn't be happy about it, but if > this was a vote it would be a -1, not a -2. It is difficult to give an > ack for a thing I don't like, but I wouldn't go as far as nacking it. On second thought, this patch should not be gated by an announce email, and given the scarcity of AIVIVT platforms, it is not worth the effort. Acked-by: Stefano Stabellini <sstabellini@kernel.org>
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index ccb0f181ea..faaf029b99 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -526,10 +526,15 @@ static void __init setup_mm(unsigned long dtb_paddr, size_t dtb_size) unsigned long boot_mfn_start, boot_mfn_end; int i; void *fdt; + const uint32_t ctr = READ_CP32(CTR); if ( !bootinfo.mem.nr_banks ) panic("No memory bank\n"); + /* We only supports instruction caches implementing the IVIPT extension. */ + if ( ((ctr >> CTR_L1Ip_SHIFT) & CTR_L1Ip_MASK) == CTR_L1Ip_AIVIVT ) + panic("AIVIVT instruction cache not supported\n"); + init_pdx(); ram_start = bootinfo.mem.bank[0].start; diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h index b5f515805d..04b05b3f39 100644 --- a/xen/include/asm-arm/processor.h +++ b/xen/include/asm-arm/processor.h @@ -6,6 +6,11 @@ #endif #include <public/arch-arm.h> +/* CTR Cache Type Register */ +#define CTR_L1Ip_MASK 0x3 +#define CTR_L1Ip_SHIFT 14 +#define CTR_L1Ip_AIVIVT 0x1 + /* MIDR Main ID Register */ #define MIDR_REVISION_MASK 0xf #define MIDR_RESIVION(midr) ((midr) & MIDR_REVISION_MASK)
The AIVIVT is a type of instruction cache available on Armv7. This is the only cache not implementing the IVIPT extension and therefore requiring specific care. To simplify maintenance requirements, Xen will not boot on platform using AIVIVT cache. This should not be an issue because Xen Arm32 can only boot on a small number of processors (see arch/arm/arm32/proc-v7.S). All of them are not using AIVIVT cache. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v3: - Patch added --- xen/arch/arm/setup.c | 5 +++++ xen/include/asm-arm/processor.h | 5 +++++ 2 files changed, 10 insertions(+)