| Message ID | 20190516000212.13468-2-alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] config.sub: Update config.sub to latest version | expand |
>>> On 16.05.19 at 02:02, <alistair.francis@wdc.com> wrote: > Make the asm/vpl011.h dependent on the ARM architecture. But we only have x86 and Arm right now. A word more about your motivation would help. Also I don't think your Cc list is wide enough for this change. Jan
On Thu, May 16, 2019 at 3:32 AM Jan Beulich <JBeulich@suse.com> wrote: > > >>> On 16.05.19 at 02:02, <alistair.francis@wdc.com> wrote: > > Make the asm/vpl011.h dependent on the ARM architecture. > > But we only have x86 and Arm right now. A word more about > your motivation would help. As the code currently is no one can add another architecture. This is just a general fixup as assuming Xen will only ever support two archs seems strange. > > Also I don't think your Cc list is wide enough for this change. I couldn't find anyother people in the MAINTAINERS file, who else should I CC? Alistair > > Jan > >
>>> On 16.05.19 at 21:30, <alistair23@gmail.com> wrote: > On Thu, May 16, 2019 at 3:32 AM Jan Beulich <JBeulich@suse.com> wrote: >> >> >>> On 16.05.19 at 02:02, <alistair.francis@wdc.com> wrote: >> > Make the asm/vpl011.h dependent on the ARM architecture. >> >> But we only have x86 and Arm right now. A word more about >> your motivation would help. > > As the code currently is no one can add another architecture. This is > just a general fixup as assuming Xen will only ever support two archs > seems strange. But that's then not the only place, is it? I'd not expect a single, random change of this kind to be sent all on its own in such a case. >> Also I don't think your Cc list is wide enough for this change. > > I couldn't find anyother people in the MAINTAINERS file, who else should I > CC? Well, how did you land at Cc-ing the tool stack maintainers on this hypervisor change? Anyway - there's a section THE REST at the bottom of ./MAINTAINERS, and we also have the two scripts/*_maintainers.pl scripts to aid you with determining who to Cc. Jan
On 16/05/2019 20:30, Alistair Francis wrote: > On Thu, May 16, 2019 at 3:32 AM Jan Beulich <JBeulich@suse.com> wrote: >> >>>>> On 16.05.19 at 02:02, <alistair.francis@wdc.com> wrote: >>> Make the asm/vpl011.h dependent on the ARM architecture. >> >> But we only have x86 and Arm right now. A word more about >> your motivation would help. > > As the code currently is no one can add another architecture. This is > just a general fixup as assuming Xen will only ever support two archs > seems strange. At which point, wouldn't it be better to avoid #ifdef ARCH in common code? Instead, we could provide arch helper and/or more meaning CONFIG name. Cheers,
On Fri, May 17, 2019 at 1:46 AM Julien Grall <julien.grall@arm.com> wrote: > > > > On 16/05/2019 20:30, Alistair Francis wrote: > > On Thu, May 16, 2019 at 3:32 AM Jan Beulich <JBeulich@suse.com> wrote: > >> > >>>>> On 16.05.19 at 02:02, <alistair.francis@wdc.com> wrote: > >>> Make the asm/vpl011.h dependent on the ARM architecture. > >> > >> But we only have x86 and Arm right now. A word more about > >> your motivation would help. > > > > As the code currently is no one can add another architecture. This is > > just a general fixup as assuming Xen will only ever support two archs > > seems strange. > > At which point, wouldn't it be better to avoid #ifdef ARCH in common code? > Instead, we could provide arch helper and/or more meaning CONFIG name. I'm not sure if the arch helpers are any easier to understand. Maybe that is worth looking into, at the moment though I still think it makes sense to fix this #ifdef. Alistair > > Cheers, > > -- > Julien Grall
On Thu, May 16, 2019 at 11:26 PM Jan Beulich <JBeulich@suse.com> wrote: > > >>> On 16.05.19 at 21:30, <alistair23@gmail.com> wrote: > > On Thu, May 16, 2019 at 3:32 AM Jan Beulich <JBeulich@suse.com> wrote: > >> > >> >>> On 16.05.19 at 02:02, <alistair.francis@wdc.com> wrote: > >> > Make the asm/vpl011.h dependent on the ARM architecture. > >> > >> But we only have x86 and Arm right now. A word more about > >> your motivation would help. > > > > As the code currently is no one can add another architecture. This is > > just a general fixup as assuming Xen will only ever support two archs > > seems strange. > > But that's then not the only place, is it? I'd not expect a single, > random change of this kind to be sent all on its own in such a > case. This is the only case that I have run into where there isn't a sane #else condition. Most other #ifdefs fall through to an "Unsupported.." message in the #else. > > >> Also I don't think your Cc list is wide enough for this change. > > > > I couldn't find anyother people in the MAINTAINERS file, who else should I > > CC? > > Well, how did you land at Cc-ing the tool stack maintainers on > this hypervisor change? Anyway - there's a section THE REST > at the bottom of ./MAINTAINERS, and we also have the two > scripts/*_maintainers.pl scripts to aid you with determining > who to Cc. Ah, sorry. I just grepped the file next time I'll make sure to turn the script. Alistair > > Jan > >
Hi Alistair, On 17/05/2019 23:01, Alistair Francis wrote: > On Fri, May 17, 2019 at 1:46 AM Julien Grall <julien.grall@arm.com> wrote: >> >> >> >> On 16/05/2019 20:30, Alistair Francis wrote: >>> On Thu, May 16, 2019 at 3:32 AM Jan Beulich <JBeulich@suse.com> wrote: >>>> >>>>>>> On 16.05.19 at 02:02, <alistair.francis@wdc.com> wrote: >>>>> Make the asm/vpl011.h dependent on the ARM architecture. >>>> >>>> But we only have x86 and Arm right now. A word more about >>>> your motivation would help. >>> >>> As the code currently is no one can add another architecture. This is >>> just a general fixup as assuming Xen will only ever support two archs >>> seems strange. >> >> At which point, wouldn't it be better to avoid #ifdef ARCH in common code? >> Instead, we could provide arch helper and/or more meaning CONFIG name. > > I'm not sure if the arch helpers are any easier to understand. Maybe > that is worth looking into, at the moment though I still think it > makes sense to fix this #ifdef. Well, I don't think we would want an #elseif defined(CONFIG_NEW_ARCH) for adding your new architecture specific include. This is defeating the purpose of common code. In this case, the code using the header is protected by CONFIG_SBSA_VUART_CONSOLE. So I would use that instead of CONFIG_ARM. Cheers,
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index 9bbcb0f57a..f840d999bc 100644 --- a/xen/drivers/char/console.c +++ b/xen/drivers/char/console.c @@ -36,7 +36,7 @@ #ifdef CONFIG_X86 #include <xen/consoled.h> #include <asm/guest.h> -#else +#elif CONFIG_ARM #include <asm/vpl011.h> #endif
Make the asm/vpl011.h dependent on the ARM architecture. Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- xen/drivers/char/console.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)