[PATCH] xen/acpi: Don't fail if SPCR table is absent

Elliott Mitchell posted 1 patch 3 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20201021221253.GA73207@mattapan.m5p.com
Maintainers: Julien Grall <julien@xen.org>, Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>, Stefano Stabellini <sstabellini@kernel.org>
xen/arch/arm/acpi/domain_build.c | 19 ++++++++++---------
1 file changed, 10 insertions(+), 9 deletions(-)
[PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Elliott Mitchell 3 years, 5 months ago
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



Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Jan Beulich 3 years, 5 months ago
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

Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Elliott Mitchell 3 years, 5 months ago
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



Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Julien Grall 3 years, 5 months ago
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

Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Jan Beulich 3 years, 5 months ago
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

Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Julien Grall 3 years, 5 months ago
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

Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Elliott Mitchell 3 years, 5 months ago
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



Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Jan Beulich 3 years, 5 months ago
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

Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Julien Grall 3 years, 5 months ago
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

Re: [PATCH] xen/acpi: Don't fail if SPCR table is absent
Posted by Stefano Stabellini 3 years, 5 months ago
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);