* [PATCH] xen/acpi: Don't fail if SPCR table is absent
@ 2020-10-21 22:12 Elliott Mitchell
2020-10-22 0:20 ` Stefano Stabellini
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Elliott Mitchell @ 2020-10-21 22:12 UTC (permalink / raw)
To: xen-devel; +Cc: Stefano Stabellini, Julien Grall, Volodymyr Babchuk
Absence of a SPCR table likely means the console is a framebuffer. In
such case acpi_iomem_deny_access() should NOT fail.
Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
xen/arch/arm/acpi/domain_build.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
index 1b1cfabb00..bbdc90f92c 100644
--- a/xen/arch/arm/acpi/domain_build.c
+++ b/xen/arch/arm/acpi/domain_build.c
@@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d)
status = acpi_get_table(ACPI_SIG_SPCR, 0,
(struct acpi_table_header **)&spcr);
- if ( ACPI_FAILURE(status) )
+ if ( ACPI_SUCCESS(status) )
{
- printk("Failed to get SPCR table\n");
- return -EINVAL;
+ mfn = spcr->serial_port.address >> PAGE_SHIFT;
+ /* Deny MMIO access for UART */
+ rc = iomem_deny_access(d, mfn, mfn + 1);
+ if ( rc )
+ return rc;
+ }
+ else
+ {
+ printk("Failed to get SPCR table, Xen console may be unavailable\n");
}
-
- mfn = spcr->serial_port.address >> PAGE_SHIFT;
- /* Deny MMIO access for UART */
- rc = iomem_deny_access(d, mfn, mfn + 1);
- if ( rc )
- return rc;
/* Deny MMIO access for GIC regions */
return gic_iomem_deny_access(d);
--
2.20.1
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | ehem+sigmsg@m5p.com PGP 87145445 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-21 22:12 [PATCH] xen/acpi: Don't fail if SPCR table is absent Elliott Mitchell
@ 2020-10-22 0:20 ` Stefano Stabellini
2020-10-22 7:42 ` Jan Beulich
2020-10-22 18:44 ` Julien Grall
2 siblings, 0 replies; 10+ messages in thread
From: Stefano Stabellini @ 2020-10-22 0:20 UTC (permalink / raw)
To: Elliott Mitchell
Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk
On Wed, 21 Oct 2020, Elliott Mitchell wrote:
> Absence of a SPCR table likely means the console is a framebuffer. In
> such case acpi_iomem_deny_access() should NOT fail.
>
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> ---
> xen/arch/arm/acpi/domain_build.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
> index 1b1cfabb00..bbdc90f92c 100644
> --- a/xen/arch/arm/acpi/domain_build.c
> +++ b/xen/arch/arm/acpi/domain_build.c
> @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d)
> status = acpi_get_table(ACPI_SIG_SPCR, 0,
> (struct acpi_table_header **)&spcr);
>
> - if ( ACPI_FAILURE(status) )
> + if ( ACPI_SUCCESS(status) )
> {
> - printk("Failed to get SPCR table\n");
> - return -EINVAL;
> + mfn = spcr->serial_port.address >> PAGE_SHIFT;
> + /* Deny MMIO access for UART */
> + rc = iomem_deny_access(d, mfn, mfn + 1);
> + if ( rc )
> + return rc;
> + }
> + else
> + {
> + printk("Failed to get SPCR table, Xen console may be unavailable\n");
> }
> -
> - mfn = spcr->serial_port.address >> PAGE_SHIFT;
> - /* Deny MMIO access for UART */
> - rc = iomem_deny_access(d, mfn, mfn + 1);
> - if ( rc )
> - return rc;
>
> /* Deny MMIO access for GIC regions */
> return gic_iomem_deny_access(d);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-21 22:12 [PATCH] xen/acpi: Don't fail if SPCR table is absent Elliott Mitchell
2020-10-22 0:20 ` Stefano Stabellini
@ 2020-10-22 7:42 ` Jan Beulich
2020-10-22 17:13 ` Elliott Mitchell
2020-10-22 18:44 ` Julien Grall
2 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2020-10-22 7:42 UTC (permalink / raw)
To: Elliott Mitchell
Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk
On 22.10.2020 00:12, Elliott Mitchell wrote:
> --- a/xen/arch/arm/acpi/domain_build.c
> +++ b/xen/arch/arm/acpi/domain_build.c
> @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d)
> status = acpi_get_table(ACPI_SIG_SPCR, 0,
> (struct acpi_table_header **)&spcr);
>
> - if ( ACPI_FAILURE(status) )
> + if ( ACPI_SUCCESS(status) )
> {
> - printk("Failed to get SPCR table\n");
> - return -EINVAL;
> + mfn = spcr->serial_port.address >> PAGE_SHIFT;
> + /* Deny MMIO access for UART */
> + rc = iomem_deny_access(d, mfn, mfn + 1);
> + if ( rc )
> + return rc;
> + }
> + else
> + {
> + printk("Failed to get SPCR table, Xen console may be unavailable\n");
> }
Nit: While I see you've got Stefano's R-b already, I Xen we typically
omit the braces here.
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-22 7:42 ` Jan Beulich
@ 2020-10-22 17:13 ` Elliott Mitchell
2020-10-22 18:38 ` Julien Grall
2020-10-23 6:17 ` Jan Beulich
0 siblings, 2 replies; 10+ messages in thread
From: Elliott Mitchell @ 2020-10-22 17:13 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk
On Thu, Oct 22, 2020 at 09:42:17AM +0200, Jan Beulich wrote:
> On 22.10.2020 00:12, Elliott Mitchell wrote:
> > --- a/xen/arch/arm/acpi/domain_build.c
> > +++ b/xen/arch/arm/acpi/domain_build.c
> > @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d)
> > status = acpi_get_table(ACPI_SIG_SPCR, 0,
> > (struct acpi_table_header **)&spcr);
> >
> > - if ( ACPI_FAILURE(status) )
> > + if ( ACPI_SUCCESS(status) )
> > {
> > - printk("Failed to get SPCR table\n");
> > - return -EINVAL;
> > + mfn = spcr->serial_port.address >> PAGE_SHIFT;
> > + /* Deny MMIO access for UART */
> > + rc = iomem_deny_access(d, mfn, mfn + 1);
> > + if ( rc )
> > + return rc;
> > + }
> > + else
> > + {
> > + printk("Failed to get SPCR table, Xen console may be unavailable\n");
> > }
>
> Nit: While I see you've got Stefano's R-b already, I Xen we typically
> omit the braces here.
Personally, I prefer that myself, but was unsure of the preference here.
I've seen multiple projects which *really* dislike using having brackets
for some clauses, but not others (ie they want either all clauses with or
all clauses without; instead of only if required).
I sent what I thought was the more often used format. (I also like tabs,
and dislike having so many spaces; alas my preferences are apparently
uncommon)
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | ehem+sigmsg@m5p.com PGP 87145445 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-22 17:13 ` Elliott Mitchell
@ 2020-10-22 18:38 ` Julien Grall
2020-10-23 6:17 ` Jan Beulich
1 sibling, 0 replies; 10+ messages in thread
From: Julien Grall @ 2020-10-22 18:38 UTC (permalink / raw)
To: Elliott Mitchell, Jan Beulich
Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk
Hi Elliott,
On 22/10/2020 18:13, Elliott Mitchell wrote:
> On Thu, Oct 22, 2020 at 09:42:17AM +0200, Jan Beulich wrote:
>> On 22.10.2020 00:12, Elliott Mitchell wrote:
>>> --- a/xen/arch/arm/acpi/domain_build.c
>>> +++ b/xen/arch/arm/acpi/domain_build.c
>>> @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>>> status = acpi_get_table(ACPI_SIG_SPCR, 0,
>>> (struct acpi_table_header **)&spcr);
>>>
>>> - if ( ACPI_FAILURE(status) )
>>> + if ( ACPI_SUCCESS(status) )
>>> {
>>> - printk("Failed to get SPCR table\n");
>>> - return -EINVAL;
>>> + mfn = spcr->serial_port.address >> PAGE_SHIFT;
>>> + /* Deny MMIO access for UART */
>>> + rc = iomem_deny_access(d, mfn, mfn + 1);
>>> + if ( rc )
>>> + return rc;
>>> + }
>>> + else
>>> + {
>>> + printk("Failed to get SPCR table, Xen console may be unavailable\n");
>>> }
>>
>> Nit: While I see you've got Stefano's R-b already, I Xen we typically
>> omit the braces here.
>
> Personally, I prefer that myself, but was unsure of the preference here.
I don't think we are very consistent here... I would not add them
myself, but I don't particularly mind them (I know some editors will add
them automatically).
I will keep them while committing. For the patch:
Acked-by: Julien Grall <jgrall@amazon.com>
> I've seen multiple projects which *really* dislike using having brackets
> for some clauses, but not others (ie they want either all clauses with or
> all clauses without; instead of only if required).
>
> I sent what I thought was the more often used format. (I also like tabs,
> and dislike having so many spaces; alas my preferences are apparently
> uncommon)
We have a few files in Xen using tabs (yes, we like mixing coding style!).
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-21 22:12 [PATCH] xen/acpi: Don't fail if SPCR table is absent Elliott Mitchell
2020-10-22 0:20 ` Stefano Stabellini
2020-10-22 7:42 ` Jan Beulich
@ 2020-10-22 18:44 ` Julien Grall
2020-10-22 19:18 ` Elliott Mitchell
2 siblings, 1 reply; 10+ messages in thread
From: Julien Grall @ 2020-10-22 18:44 UTC (permalink / raw)
To: Elliott Mitchell, xen-devel; +Cc: Stefano Stabellini, Volodymyr Babchuk
Hi,
Thank you for the patch. FIY I tweak a bit the commit title before
committing.
The title is now: "xen/arm: acpi: Don't fail it SPCR table is absent".
Cheers,
On 21/10/2020 23:12, Elliott Mitchell wrote:
> Absence of a SPCR table likely means the console is a framebuffer. In
> such case acpi_iomem_deny_access() should NOT fail.
>
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
> ---
> xen/arch/arm/acpi/domain_build.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/xen/arch/arm/acpi/domain_build.c b/xen/arch/arm/acpi/domain_build.c
> index 1b1cfabb00..bbdc90f92c 100644
> --- a/xen/arch/arm/acpi/domain_build.c
> +++ b/xen/arch/arm/acpi/domain_build.c
> @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d)
> status = acpi_get_table(ACPI_SIG_SPCR, 0,
> (struct acpi_table_header **)&spcr);
>
> - if ( ACPI_FAILURE(status) )
> + if ( ACPI_SUCCESS(status) )
> {
> - printk("Failed to get SPCR table\n");
> - return -EINVAL;
> + mfn = spcr->serial_port.address >> PAGE_SHIFT;
> + /* Deny MMIO access for UART */
> + rc = iomem_deny_access(d, mfn, mfn + 1);
> + if ( rc )
> + return rc;
> + }
> + else
> + {
> + printk("Failed to get SPCR table, Xen console may be unavailable\n");
> }
> -
> - mfn = spcr->serial_port.address >> PAGE_SHIFT;
> - /* Deny MMIO access for UART */
> - rc = iomem_deny_access(d, mfn, mfn + 1);
> - if ( rc )
> - return rc;
>
> /* Deny MMIO access for GIC regions */
> return gic_iomem_deny_access(d);
>
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-22 18:44 ` Julien Grall
@ 2020-10-22 19:18 ` Elliott Mitchell
2020-10-23 7:11 ` Jan Beulich
2020-10-23 9:30 ` Julien Grall
0 siblings, 2 replies; 10+ messages in thread
From: Elliott Mitchell @ 2020-10-22 19:18 UTC (permalink / raw)
To: Julien Grall; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk
On Thu, Oct 22, 2020 at 07:38:26PM +0100, Julien Grall wrote:
> I don't think we are very consistent here... I would not add them
> myself, but I don't particularly mind them (I know some editors will add
> them automatically).
>
> I will keep them while committing. For the patch:
I would tend to remove them on commit since I dislike them. Just as
stated, I was unsure.
On default settings, clang-format will object to:
if(thing)
{
foo
}
else
bar
Or
if(thing)
foo
else
{
bar
}
I *like* those formats, but was under the impression most people did not.
The indentation is the more visually obvious indicator, just the compiler
actually uses the brackets. As such I *like* the misleading indentation
warnings as those seemed to have a fairly high true-positive rate.
On Thu, Oct 22, 2020 at 07:44:26PM +0100, Julien Grall wrote:
> Thank you for the patch. FIY I tweak a bit the commit title before
> committing.
>
> The title is now: "xen/arm: acpi: Don't fail it SPCR table is absent".
Perhaps "xen/arm: acpi: Don't fail on absent SPCR table"?
What you're suggesting doesn't read well to me.
--
(\___(\___(\______ --=> 8-) EHM <=-- ______/)___/)___/)
\BS ( | ehem+sigmsg@m5p.com PGP 87145445 | ) /
\_CS\ | _____ -O #include <stddisclaimer.h> O- _____ | / _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-22 17:13 ` Elliott Mitchell
2020-10-22 18:38 ` Julien Grall
@ 2020-10-23 6:17 ` Jan Beulich
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2020-10-23 6:17 UTC (permalink / raw)
To: Elliott Mitchell
Cc: xen-devel, Stefano Stabellini, Julien Grall, Volodymyr Babchuk
On 22.10.2020 19:13, Elliott Mitchell wrote:
> On Thu, Oct 22, 2020 at 09:42:17AM +0200, Jan Beulich wrote:
>> On 22.10.2020 00:12, Elliott Mitchell wrote:
>>> --- a/xen/arch/arm/acpi/domain_build.c
>>> +++ b/xen/arch/arm/acpi/domain_build.c
>>> @@ -42,17 +42,18 @@ static int __init acpi_iomem_deny_access(struct domain *d)
>>> status = acpi_get_table(ACPI_SIG_SPCR, 0,
>>> (struct acpi_table_header **)&spcr);
>>>
>>> - if ( ACPI_FAILURE(status) )
>>> + if ( ACPI_SUCCESS(status) )
>>> {
>>> - printk("Failed to get SPCR table\n");
>>> - return -EINVAL;
>>> + mfn = spcr->serial_port.address >> PAGE_SHIFT;
>>> + /* Deny MMIO access for UART */
>>> + rc = iomem_deny_access(d, mfn, mfn + 1);
>>> + if ( rc )
>>> + return rc;
>>> + }
>>> + else
>>> + {
>>> + printk("Failed to get SPCR table, Xen console may be unavailable\n");
>>> }
>>
>> Nit: While I see you've got Stefano's R-b already, I Xen we typically
>> omit the braces here.
>
> Personally, I prefer that myself, but was unsure of the preference here.
> I've seen multiple projects which *really* dislike using having brackets
> for some clauses, but not others (ie they want either all clauses with or
> all clauses without; instead of only if required).
>
> I sent what I thought was the more often used format. (I also like tabs,
> and dislike having so many spaces; alas my preferences are apparently
> uncommon)
"More often used" doesn't matter when there's an explicit statement
about this in ./CODING_STYLE: "Braces should be omitted for blocks
with a single statement." Yes, there are projects requiring all
if/else-if belonging together to consistently use or not use braces.
But there's explicitly no such statement in our doc. (Along these
lines, dislike of spaces [and favoring tabs] also doesn't matter, as
again the doc is explicit about it.)
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-22 19:18 ` Elliott Mitchell
@ 2020-10-23 7:11 ` Jan Beulich
2020-10-23 9:30 ` Julien Grall
1 sibling, 0 replies; 10+ messages in thread
From: Jan Beulich @ 2020-10-23 7:11 UTC (permalink / raw)
To: Elliott Mitchell
Cc: Julien Grall, xen-devel, Stefano Stabellini, Volodymyr Babchuk
On 22.10.2020 21:18, Elliott Mitchell wrote:
> On Thu, Oct 22, 2020 at 07:44:26PM +0100, Julien Grall wrote:
>> Thank you for the patch. FIY I tweak a bit the commit title before
>> committing.
>>
>> The title is now: "xen/arm: acpi: Don't fail it SPCR table is absent".
>
> Perhaps "xen/arm: acpi: Don't fail on absent SPCR table"?
>
> What you're suggesting doesn't read well to me.
Perhaps Julien meant "if" instead of "it". i.e. a simple typo?
Jan
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
2020-10-22 19:18 ` Elliott Mitchell
2020-10-23 7:11 ` Jan Beulich
@ 2020-10-23 9:30 ` Julien Grall
1 sibling, 0 replies; 10+ messages in thread
From: Julien Grall @ 2020-10-23 9:30 UTC (permalink / raw)
To: Elliott Mitchell; +Cc: xen-devel, Stefano Stabellini, Volodymyr Babchuk
Hi Elliott,
On 22/10/2020 20:18, Elliott Mitchell wrote:
> On Thu, Oct 22, 2020 at 07:38:26PM +0100, Julien Grall wrote:
> On Thu, Oct 22, 2020 at 07:44:26PM +0100, Julien Grall wrote:
>> Thank you for the patch. FIY I tweak a bit the commit title before
>> committing.
>>
>> The title is now: "xen/arm: acpi: Don't fail it SPCR table is absent".
>
> Perhaps "xen/arm: acpi: Don't fail on absent SPCR table"?
>
> What you're suggesting doesn't read well to me.
Sorry, I made a typo when writing the title in the e-mail. Here a direct
copy from the commit:
"xen/arm: acpi: Don't fail if SPCR table is absent"
This is pretty much your original title with "arm: " added to clarify
the subsystem modified.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-23 9:31 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-21 22:12 [PATCH] xen/acpi: Don't fail if SPCR table is absent Elliott Mitchell
2020-10-22 0:20 ` Stefano Stabellini
2020-10-22 7:42 ` Jan Beulich
2020-10-22 17:13 ` Elliott Mitchell
2020-10-22 18:38 ` Julien Grall
2020-10-23 6:17 ` Jan Beulich
2020-10-22 18:44 ` Julien Grall
2020-10-22 19:18 ` Elliott Mitchell
2020-10-23 7:11 ` Jan Beulich
2020-10-23 9:30 ` Julien Grall
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.