Hi Jan,
On 2020/2/21 23:07, Jan Beulich wrote:
> On 21.02.2020 15:57, Julien Grall wrote:
>> On 21/02/2020 14:02, Jan Beulich wrote:
>>> On 21.02.2020 03:22, Wei Xu wrote:
>>>> --- a/xen/drivers/char/ns16550.c
>>>> +++ b/xen/drivers/char/ns16550.c
>>>> @@ -1620,6 +1620,85 @@ DT_DEVICE_START(ns16550, "NS16550 UART",
>>>> DEVICE_SERIAL)
>>>> DT_DEVICE_END
>>>>
>>>> #endif /* HAS_DEVICE_TREE */
>>>> +
>>>> +#if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
>>>> +#include <xen/acpi.h>
>>>> +
>>>> +static int __init ns16550_acpi_uart_init(const void *data)
>>>> +{
>>>> + struct acpi_table_header *table;
>>>> + struct acpi_table_spcr *spcr;
>>>> + acpi_status status;
>>>> + /*
>>>> + * Same as the DT part.
>>>> + * Only support one UART on ARM which happen to be ns16550_com[0].
>>>> + */
>>>> + struct ns16550 *uart = &ns16550_com[0];
>>>> +
>>>> + status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
>>>> + if ( ACPI_FAILURE(status) )
>>>> + {
>>>> + printk("ns16550: Failed to get SPCR table\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + spcr = container_of(table, struct acpi_table_spcr, header);
>>>> +
>>>> + /*
>>>> + * The serial port address may be 0 for example
>>>> + * if the console redirection is disabled.
>>>> + */
>>>> + if ( unlikely(!spcr->serial_port.address) )
>>>> + {
>>>> + printk("ns16550: Console redirection is disabled\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + if ( unlikely(spcr->serial_port.space_id !=
>>>> ACPI_ADR_SPACE_SYSTEM_MEMORY) )
>>>> + {
>>>> + printk("ns16550: Address space type is not mmio\n");
>>>> + return -EINVAL;
>>>> + }
>>>
>>> The space_id field qualifies the address one, i.e. whether a value of
>>> zero can sensibly mean "disabled" depends on the address space. Hence
>>> logically the address space check should come first.
>>>
>>> This is the last thing I'd like to see changed. I won't give the
>>> patch my ack though, as I think it should be an Arm maintainer to ack
>>> it.
>>
>> Acked-by: Julien Grall <jul...@xen.org>
>>
>> Although, a reviewed-by tag from you would be nice as you did most of
>> the review for this patch.
>
> Well, to clarify - this is one of the very few (afaict) cases where our
> R-b implying A-b (when people are entitled to ack the respective code)
> gets in the way. If this wasn't the case, I'd have given the former,
> making it clear (also later from just looking at the resulting commit)
> that the (only) ack came from an Arm person.
I will check the address space in the v5 and add your R-b.
Thanks!
Best Regards,
Wei
>
> Jan
>
> .
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
