* [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.
2020-10-26 17:17 [PATCH v1 0/4] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
@ 2020-10-26 17:17 ` Rahul Singh
2020-10-27 23:32 ` Stefano Stabellini
2020-10-26 17:17 ` [PATCH v1 2/4] xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS functionality Rahul Singh
` (3 subsequent siblings)
4 siblings, 1 reply; 26+ messages in thread
From: Rahul Singh @ 2020-10-26 17:17 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, Andrew Cooper, George Dunlap, Ian Jackson,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
ARM platforms does not support ns16550 PCI support. When CONFIG_HAS_PCI
is enabled for ARM a compilation error is observed.
Fixed compilation error after introducing new kconfig option
CONFIG_HAS_NS16550_PCI for x86 platforms to support ns16550 PCI.
No functional change.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
xen/drivers/char/Kconfig | 7 +++++++
xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
2 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index b572305657..8887e86afe 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -4,6 +4,13 @@ config HAS_NS16550
help
This selects the 16550-series UART support. For most systems, say Y.
+config HAS_NS16550_PCI
+ bool "NS16550 UART PCI support" if X86
+ default y
+ depends on X86 && HAS_NS16550 && HAS_PCI
+ help
+ This selects the 16550-series UART PCI support. For most systems, say Y.
+
config HAS_CADENCE_UART
bool "Xilinx Cadence UART driver"
default y
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index d8b52eb813..bd1c2af956 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -16,7 +16,7 @@
#include <xen/timer.h>
#include <xen/serial.h>
#include <xen/iocap.h>
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
#include <xen/pci.h>
#include <xen/pci_regs.h>
#include <xen/pci_ids.h>
@@ -54,7 +54,7 @@ enum serial_param_type {
reg_shift,
reg_width,
stop_bits,
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
bridge_bdf,
device,
port_bdf,
@@ -83,7 +83,7 @@ static struct ns16550 {
unsigned int timeout_ms;
bool_t intr_works;
bool_t dw_usr_bsy;
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
/* PCI card parameters. */
bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
bool_t ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
@@ -117,14 +117,14 @@ static const struct serial_param_var __initconst sp_vars[] = {
{"reg-shift", reg_shift},
{"reg-width", reg_width},
{"stop-bits", stop_bits},
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
{"bridge", bridge_bdf},
{"dev", device},
{"port", port_bdf},
#endif
};
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
struct ns16550_config {
u16 vendor_id;
u16 dev_id;
@@ -620,7 +620,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
static void pci_serial_early_init(struct ns16550 *uart)
{
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
return;
@@ -719,7 +719,7 @@ static void __init ns16550_init_preirq(struct serial_port *port)
static void __init ns16550_init_irq(struct serial_port *port)
{
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
struct ns16550 *uart = port->uart;
if ( uart->msi )
@@ -761,7 +761,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
uart->timeout_ms = max_t(
unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
if ( uart->bar || uart->ps_bdf_enable )
{
if ( uart->param && uart->param->mmio &&
@@ -841,7 +841,7 @@ static void ns16550_suspend(struct serial_port *port)
stop_timer(&uart->timer);
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
if ( uart->bar )
uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
uart->ps_bdf[2]), PCI_COMMAND);
@@ -850,7 +850,7 @@ static void ns16550_suspend(struct serial_port *port)
static void _ns16550_resume(struct serial_port *port)
{
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
struct ns16550 *uart = port->uart;
if ( uart->bar )
@@ -1013,7 +1013,7 @@ static int __init check_existence(struct ns16550 *uart)
return 1; /* Everything is MMIO */
#endif
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
pci_serial_early_init(uart);
#endif
@@ -1044,7 +1044,7 @@ static int __init check_existence(struct ns16550 *uart)
return (status == 0x90);
}
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
static int __init
pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
{
@@ -1305,7 +1305,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
if ( *conf == ',' && *++conf != ',' )
{
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
if ( strncmp(conf, "pci", 3) == 0 )
{
if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
@@ -1327,7 +1327,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
if ( *conf == ',' && *++conf != ',' )
{
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
if ( strncmp(conf, "msi", 3) == 0 )
{
conf += 3;
@@ -1339,7 +1339,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
uart->irq = simple_strtol(conf, &conf, 10);
}
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
if ( *conf == ',' && *++conf != ',' )
{
conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
@@ -1419,7 +1419,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
uart->reg_width = simple_strtoul(param_value, NULL, 0);
break;
-#ifdef CONFIG_HAS_PCI
+#ifdef CONFIG_HAS_NS16550_PCI
case bridge_bdf:
if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
&uart->ps_bdf[1], &uart->ps_bdf[2]) )
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.
2020-10-26 17:17 ` [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled Rahul Singh
@ 2020-10-27 23:32 ` Stefano Stabellini
2020-10-28 7:18 ` Jan Beulich
2020-10-28 10:38 ` Rahul Singh
0 siblings, 2 replies; 26+ messages in thread
From: Stefano Stabellini @ 2020-10-27 23:32 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, bertrand.marquis, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu
On Mon, 26 Oct 2020, Rahul Singh wrote:
> ARM platforms does not support ns16550 PCI support. When CONFIG_HAS_PCI
^ do
> is enabled for ARM a compilation error is observed.
>
> Fixed compilation error after introducing new kconfig option
> CONFIG_HAS_NS16550_PCI for x86 platforms to support ns16550 PCI.
>
> No functional change.
Written like that it would seem that ARM platforms do not support
NS16550 on the PCI bus, but actually, it would be theoretically possible
to have an NS16550 card slotted in a PCI bus on ARM, right?
The problem is the current limitation in terms of Xen internal
plumbings, such as lack of MSI support. Is that correct?
If so, I'd update the commit message to reflect the situation a bit
better.
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/drivers/char/Kconfig | 7 +++++++
> xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
> 2 files changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index b572305657..8887e86afe 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -4,6 +4,13 @@ config HAS_NS16550
> help
> This selects the 16550-series UART support. For most systems, say Y.
>
> +config HAS_NS16550_PCI
> + bool "NS16550 UART PCI support" if X86
> + default y
> + depends on X86 && HAS_NS16550 && HAS_PCI
> + help
> + This selects the 16550-series UART PCI support. For most systems, say Y.
I think that this should be a silent option:
if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
otherwise -> automatically disable
No need to show it to the user.
The rest of course is fine.
> config HAS_CADENCE_UART
> bool "Xilinx Cadence UART driver"
> default y
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index d8b52eb813..bd1c2af956 100644
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -16,7 +16,7 @@
> #include <xen/timer.h>
> #include <xen/serial.h>
> #include <xen/iocap.h>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> #include <xen/pci.h>
> #include <xen/pci_regs.h>
> #include <xen/pci_ids.h>
> @@ -54,7 +54,7 @@ enum serial_param_type {
> reg_shift,
> reg_width,
> stop_bits,
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> bridge_bdf,
> device,
> port_bdf,
> @@ -83,7 +83,7 @@ static struct ns16550 {
> unsigned int timeout_ms;
> bool_t intr_works;
> bool_t dw_usr_bsy;
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> /* PCI card parameters. */
> bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
> bool_t ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
> @@ -117,14 +117,14 @@ static const struct serial_param_var __initconst sp_vars[] = {
> {"reg-shift", reg_shift},
> {"reg-width", reg_width},
> {"stop-bits", stop_bits},
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> {"bridge", bridge_bdf},
> {"dev", device},
> {"port", port_bdf},
> #endif
> };
>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> struct ns16550_config {
> u16 vendor_id;
> u16 dev_id;
> @@ -620,7 +620,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
>
> static void pci_serial_early_init(struct ns16550 *uart)
> {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
> return;
>
> @@ -719,7 +719,7 @@ static void __init ns16550_init_preirq(struct serial_port *port)
>
> static void __init ns16550_init_irq(struct serial_port *port)
> {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> struct ns16550 *uart = port->uart;
>
> if ( uart->msi )
> @@ -761,7 +761,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
> uart->timeout_ms = max_t(
> unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> if ( uart->bar || uart->ps_bdf_enable )
> {
> if ( uart->param && uart->param->mmio &&
> @@ -841,7 +841,7 @@ static void ns16550_suspend(struct serial_port *port)
>
> stop_timer(&uart->timer);
>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> if ( uart->bar )
> uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
> uart->ps_bdf[2]), PCI_COMMAND);
> @@ -850,7 +850,7 @@ static void ns16550_suspend(struct serial_port *port)
>
> static void _ns16550_resume(struct serial_port *port)
> {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> struct ns16550 *uart = port->uart;
>
> if ( uart->bar )
> @@ -1013,7 +1013,7 @@ static int __init check_existence(struct ns16550 *uart)
> return 1; /* Everything is MMIO */
> #endif
>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> pci_serial_early_init(uart);
> #endif
>
> @@ -1044,7 +1044,7 @@ static int __init check_existence(struct ns16550 *uart)
> return (status == 0x90);
> }
>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> static int __init
> pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
> {
> @@ -1305,7 +1305,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>
> if ( *conf == ',' && *++conf != ',' )
> {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> if ( strncmp(conf, "pci", 3) == 0 )
> {
> if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
> @@ -1327,7 +1327,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>
> if ( *conf == ',' && *++conf != ',' )
> {
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> if ( strncmp(conf, "msi", 3) == 0 )
> {
> conf += 3;
> @@ -1339,7 +1339,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
> uart->irq = simple_strtol(conf, &conf, 10);
> }
>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> if ( *conf == ',' && *++conf != ',' )
> {
> conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
> @@ -1419,7 +1419,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
> uart->reg_width = simple_strtoul(param_value, NULL, 0);
> break;
>
> -#ifdef CONFIG_HAS_PCI
> +#ifdef CONFIG_HAS_NS16550_PCI
> case bridge_bdf:
> if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
> &uart->ps_bdf[1], &uart->ps_bdf[2]) )
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.
2020-10-27 23:32 ` Stefano Stabellini
@ 2020-10-28 7:18 ` Jan Beulich
2020-10-28 10:41 ` Rahul Singh
2020-10-28 10:38 ` Rahul Singh
1 sibling, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2020-10-28 7:18 UTC (permalink / raw)
To: Stefano Stabellini, Rahul Singh
Cc: xen-devel, bertrand.marquis, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Wei Liu
On 28.10.2020 00:32, Stefano Stabellini wrote:
> On Mon, 26 Oct 2020, Rahul Singh wrote:
>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -4,6 +4,13 @@ config HAS_NS16550
>> help
>> This selects the 16550-series UART support. For most systems, say Y.
>>
>> +config HAS_NS16550_PCI
>> + bool "NS16550 UART PCI support" if X86
>> + default y
>> + depends on X86 && HAS_NS16550 && HAS_PCI
>> + help
>> + This selects the 16550-series UART PCI support. For most systems, say Y.
>
> I think that this should be a silent option:
> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
> otherwise -> automatically disable
>
> No need to show it to the user.
I agree in principle, but I don't see why an X86 dependency gets
added here. HAS_PCI really should be all that's needed.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.
2020-10-28 7:18 ` Jan Beulich
@ 2020-10-28 10:41 ` Rahul Singh
2020-10-28 11:32 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Rahul Singh @ 2020-10-28 10:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, xen-devel, Bertrand Marquis, Andrew Cooper,
George Dunlap, Ian Jackson, Julien Grall, Wei Liu
Hello Jan,
> On 28 Oct 2020, at 7:18 am, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 28.10.2020 00:32, Stefano Stabellini wrote:
>> On Mon, 26 Oct 2020, Rahul Singh wrote:
>>> --- a/xen/drivers/char/Kconfig
>>> +++ b/xen/drivers/char/Kconfig
>>> @@ -4,6 +4,13 @@ config HAS_NS16550
>>> help
>>> This selects the 16550-series UART support. For most systems, say Y.
>>>
>>> +config HAS_NS16550_PCI
>>> + bool "NS16550 UART PCI support" if X86
>>> + default y
>>> + depends on X86 && HAS_NS16550 && HAS_PCI
>>> + help
>>> + This selects the 16550-series UART PCI support. For most systems, say Y.
>>
>> I think that this should be a silent option:
>> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
>> otherwise -> automatically disable
>>
>> No need to show it to the user.
>
> I agree in principle, but I don't see why an X86 dependency gets
> added here. HAS_PCI really should be all that's needed.
>
Yes you are right . I will remove the X86 and make HAS_NS16550_PCI depend on "HAS_NS16550 && HAS_PCI"
> Jan
Regards,
Rahul
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.
2020-10-28 10:41 ` Rahul Singh
@ 2020-10-28 11:32 ` Julien Grall
2020-10-28 15:47 ` Rahul Singh
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2020-10-28 11:32 UTC (permalink / raw)
To: Rahul Singh, Jan Beulich
Cc: Stefano Stabellini, xen-devel, Bertrand Marquis, Andrew Cooper,
George Dunlap, Ian Jackson, Wei Liu
Hi,
On 28/10/2020 10:41, Rahul Singh wrote:
>> On 28 Oct 2020, at 7:18 am, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 28.10.2020 00:32, Stefano Stabellini wrote:
>>> On Mon, 26 Oct 2020, Rahul Singh wrote:
>>>> --- a/xen/drivers/char/Kconfig
>>>> +++ b/xen/drivers/char/Kconfig
>>>> @@ -4,6 +4,13 @@ config HAS_NS16550
>>>> help
>>>> This selects the 16550-series UART support. For most systems, say Y.
>>>>
>>>> +config HAS_NS16550_PCI
>>>> + bool "NS16550 UART PCI support" if X86
>>>> + default y
>>>> + depends on X86 && HAS_NS16550 && HAS_PCI
>>>> + help
>>>> + This selects the 16550-series UART PCI support. For most systems, say Y.
>>>
>>> I think that this should be a silent option:
>>> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
>>> otherwise -> automatically disable
>>>
>>> No need to show it to the user.
>>
>> I agree in principle, but I don't see why an X86 dependency gets
>> added here. HAS_PCI really should be all that's needed.
>>
>
> Yes you are right . I will remove the X86 and make HAS_NS16550_PCI depend on "HAS_NS16550 && HAS_PCI"
There are quite a bit of work to make the PCI part of the implementation
build on Arm because the code refer to x86 functions.
While in theory, an NS16550 PCI card could be used on Arm, there is only
a slim chance for an actual users. So I am not convinced the effort is
worth it here.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.
2020-10-28 11:32 ` Julien Grall
@ 2020-10-28 15:47 ` Rahul Singh
2020-10-28 15:49 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Rahul Singh @ 2020-10-28 15:47 UTC (permalink / raw)
To: Julien Grall
Cc: Jan Beulich, Stefano Stabellini, xen-devel, Bertrand Marquis,
Andrew Cooper, George Dunlap, Ian Jackson, Wei Liu
Hello Julen,
> On 28 Oct 2020, at 11:32 am, Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 28/10/2020 10:41, Rahul Singh wrote:
>>> On 28 Oct 2020, at 7:18 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>
>>> On 28.10.2020 00:32, Stefano Stabellini wrote:
>>>> On Mon, 26 Oct 2020, Rahul Singh wrote:
>>>>> --- a/xen/drivers/char/Kconfig
>>>>> +++ b/xen/drivers/char/Kconfig
>>>>> @@ -4,6 +4,13 @@ config HAS_NS16550
>>>>> help
>>>>> This selects the 16550-series UART support. For most systems, say Y.
>>>>>
>>>>> +config HAS_NS16550_PCI
>>>>> + bool "NS16550 UART PCI support" if X86
>>>>> + default y
>>>>> + depends on X86 && HAS_NS16550 && HAS_PCI
>>>>> + help
>>>>> + This selects the 16550-series UART PCI support. For most systems, say Y.
>>>>
>>>> I think that this should be a silent option:
>>>> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
>>>> otherwise -> automatically disable
>>>>
>>>> No need to show it to the user.
>>>
>>> I agree in principle, but I don't see why an X86 dependency gets
>>> added here. HAS_PCI really should be all that's needed.
>>>
>> Yes you are right . I will remove the X86 and make HAS_NS16550_PCI depend on "HAS_NS16550 && HAS_PCI"
>
> There are quite a bit of work to make the PCI part of the implementation build on Arm because the code refer to x86 functions.
>
> While in theory, an NS16550 PCI card could be used on Arm, there is only a slim chance for an actual users. So I am not convinced the effort is worth it here.
>
> Cheers,
Ok. I will enable by default HAS_NS16550_PCI on X86 only. Once we have proper support for NS16550 PCI on ARM we can enable it at that time.
I will modify as follows:
config HAS_NS16550_PCI
bool
default y
depends on X86 && HAS_NS16550 && HAS_PCI
help
This selects the 16550-series UART PCI support. For most systems, say Y.
>
> --
> Julien Grall
Thanks,
Rahul
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.
2020-10-28 15:47 ` Rahul Singh
@ 2020-10-28 15:49 ` Jan Beulich
0 siblings, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2020-10-28 15:49 UTC (permalink / raw)
To: Rahul Singh
Cc: Stefano Stabellini, xen-devel, Bertrand Marquis, Andrew Cooper,
George Dunlap, Ian Jackson, Wei Liu, Julien Grall
On 28.10.2020 16:47, Rahul Singh wrote:
> Ok. I will enable by default HAS_NS16550_PCI on X86 only. Once we have proper support for NS16550 PCI on ARM we can enable it at that time.
> I will modify as follows:
>
> config HAS_NS16550_PCI
> bool
> default y
def_bool y
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled.
2020-10-27 23:32 ` Stefano Stabellini
2020-10-28 7:18 ` Jan Beulich
@ 2020-10-28 10:38 ` Rahul Singh
1 sibling, 0 replies; 26+ messages in thread
From: Rahul Singh @ 2020-10-28 10:38 UTC (permalink / raw)
To: Stefano Stabellini
Cc: xen-devel, Bertrand Marquis, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall, Wei Liu
Hello Stefano,
> On 27 Oct 2020, at 11:32 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Mon, 26 Oct 2020, Rahul Singh wrote:
>> ARM platforms does not support ns16550 PCI support. When CONFIG_HAS_PCI
> ^ do
Ok I will fix that in next version.
>
>> is enabled for ARM a compilation error is observed.
>>
>> Fixed compilation error after introducing new kconfig option
>> CONFIG_HAS_NS16550_PCI for x86 platforms to support ns16550 PCI.
>>
>> No functional change.
>
> Written like that it would seem that ARM platforms do not support
> NS16550 on the PCI bus, but actually, it would be theoretically possible
> to have an NS16550 card slotted in a PCI bus on ARM, right?
>
> The problem is the current limitation in terms of Xen internal
> plumbings, such as lack of MSI support. Is that correct?
>
> If so, I'd update the commit message to reflect the situation a bit
> better.
>
Yes you are right on ARM platforms PCI is not fully supported because of that
I decided to disable it for ARM. Once we have full support for PCI device we can enable
it for ARM also. I will modify the commit msg to reflect the same.
>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> xen/drivers/char/Kconfig | 7 +++++++
>> xen/drivers/char/ns16550.c | 32 ++++++++++++++++----------------
>> 2 files changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
>> index b572305657..8887e86afe 100644
>> --- a/xen/drivers/char/Kconfig
>> +++ b/xen/drivers/char/Kconfig
>> @@ -4,6 +4,13 @@ config HAS_NS16550
>> help
>> This selects the 16550-series UART support. For most systems, say Y.
>>
>> +config HAS_NS16550_PCI
>> + bool "NS16550 UART PCI support" if X86
>> + default y
>> + depends on X86 && HAS_NS16550 && HAS_PCI
>> + help
>> + This selects the 16550-series UART PCI support. For most systems, say Y.
>
> I think that this should be a silent option:
> if HAS_NS16550 && HAS_PCI && X86 -> automatically enable
> otherwise -> automatically disable
>
> No need to show it to the user.
>
> The rest of course is fine.
Ok I will fix that. I will do the same as done for HAS_NS16550 ,
x86: silent option depend on HAS_NS16550 && HAS_PCI
ARM: user can decide to enable or disable via menuconfig depend on HAS_NS16550 && HAS_PCI.
>
>
>> config HAS_CADENCE_UART
>> bool "Xilinx Cadence UART driver"
>> default y
>> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
>> index d8b52eb813..bd1c2af956 100644
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -16,7 +16,7 @@
>> #include <xen/timer.h>
>> #include <xen/serial.h>
>> #include <xen/iocap.h>
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> #include <xen/pci.h>
>> #include <xen/pci_regs.h>
>> #include <xen/pci_ids.h>
>> @@ -54,7 +54,7 @@ enum serial_param_type {
>> reg_shift,
>> reg_width,
>> stop_bits,
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> bridge_bdf,
>> device,
>> port_bdf,
>> @@ -83,7 +83,7 @@ static struct ns16550 {
>> unsigned int timeout_ms;
>> bool_t intr_works;
>> bool_t dw_usr_bsy;
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> /* PCI card parameters. */
>> bool_t pb_bdf_enable; /* if =1, pb-bdf effective, port behind bridge */
>> bool_t ps_bdf_enable; /* if =1, ps_bdf effective, port on pci card */
>> @@ -117,14 +117,14 @@ static const struct serial_param_var __initconst sp_vars[] = {
>> {"reg-shift", reg_shift},
>> {"reg-width", reg_width},
>> {"stop-bits", stop_bits},
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> {"bridge", bridge_bdf},
>> {"dev", device},
>> {"port", port_bdf},
>> #endif
>> };
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> struct ns16550_config {
>> u16 vendor_id;
>> u16 dev_id;
>> @@ -620,7 +620,7 @@ static int ns16550_getc(struct serial_port *port, char *pc)
>>
>> static void pci_serial_early_init(struct ns16550 *uart)
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
>> return;
>>
>> @@ -719,7 +719,7 @@ static void __init ns16550_init_preirq(struct serial_port *port)
>>
>> static void __init ns16550_init_irq(struct serial_port *port)
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> struct ns16550 *uart = port->uart;
>>
>> if ( uart->msi )
>> @@ -761,7 +761,7 @@ static void __init ns16550_init_postirq(struct serial_port *port)
>> uart->timeout_ms = max_t(
>> unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> if ( uart->bar || uart->ps_bdf_enable )
>> {
>> if ( uart->param && uart->param->mmio &&
>> @@ -841,7 +841,7 @@ static void ns16550_suspend(struct serial_port *port)
>>
>> stop_timer(&uart->timer);
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> if ( uart->bar )
>> uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>> uart->ps_bdf[2]), PCI_COMMAND);
>> @@ -850,7 +850,7 @@ static void ns16550_suspend(struct serial_port *port)
>>
>> static void _ns16550_resume(struct serial_port *port)
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> struct ns16550 *uart = port->uart;
>>
>> if ( uart->bar )
>> @@ -1013,7 +1013,7 @@ static int __init check_existence(struct ns16550 *uart)
>> return 1; /* Everything is MMIO */
>> #endif
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> pci_serial_early_init(uart);
>> #endif
>>
>> @@ -1044,7 +1044,7 @@ static int __init check_existence(struct ns16550 *uart)
>> return (status == 0x90);
>> }
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> static int __init
>> pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx)
>> {
>> @@ -1305,7 +1305,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>>
>> if ( *conf == ',' && *++conf != ',' )
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> if ( strncmp(conf, "pci", 3) == 0 )
>> {
>> if ( pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com) )
>> @@ -1327,7 +1327,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>>
>> if ( *conf == ',' && *++conf != ',' )
>> {
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> if ( strncmp(conf, "msi", 3) == 0 )
>> {
>> conf += 3;
>> @@ -1339,7 +1339,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>> uart->irq = simple_strtol(conf, &conf, 10);
>> }
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> if ( *conf == ',' && *++conf != ',' )
>> {
>> conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
>> @@ -1419,7 +1419,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
>> uart->reg_width = simple_strtoul(param_value, NULL, 0);
>> break;
>>
>> -#ifdef CONFIG_HAS_PCI
>> +#ifdef CONFIG_HAS_NS16550_PCI
>> case bridge_bdf:
>> if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
>> &uart->ps_bdf[1], &uart->ps_bdf[2]) )
>> --
>> 2.17.1
Regards,
Rahul
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 2/4] xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS functionality.
2020-10-26 17:17 [PATCH v1 0/4] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
2020-10-26 17:17 ` [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled Rahul Singh
@ 2020-10-26 17:17 ` Rahul Singh
2020-10-27 23:55 ` Stefano Stabellini
2020-10-28 7:25 ` Jan Beulich
2020-10-26 17:17 ` [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory Rahul Singh
` (2 subsequent siblings)
4 siblings, 2 replies; 26+ messages in thread
From: Rahul Singh @ 2020-10-26 17:17 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Paul Durrant, Kevin Tian
PCI ATS functionality is not enabled and tested for ARM architecture
but it is enabled for x86 and referenced in common passthrough/pci.c
code.
Therefore introducing the new flag to enable the ATS functionality for
x86 only to avoid issues for ARM architecture.
No functional change.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
xen/arch/x86/Kconfig | 1 +
xen/drivers/passthrough/ats.h | 24 ++++++++++++++++++++++++
xen/drivers/passthrough/vtd/x86/Makefile | 2 +-
xen/drivers/passthrough/x86/Makefile | 2 +-
xen/drivers/pci/Kconfig | 3 +++
5 files changed, 30 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 24868aa6ad..31906e9c97 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -20,6 +20,7 @@ config X86
select HAS_NS16550
select HAS_PASSTHROUGH
select HAS_PCI
+ select HAS_PCI_ATS
select HAS_PDX
select HAS_SCHED_GRANULARITY
select HAS_UBSAN
diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h
index 22ae209b37..a0af07b287 100644
--- a/xen/drivers/passthrough/ats.h
+++ b/xen/drivers/passthrough/ats.h
@@ -17,6 +17,8 @@
#include <xen/pci_regs.h>
+#ifdef CONFIG_HAS_PCI_ATS
+
#define ATS_REG_CAP 4
#define ATS_REG_CTL 6
#define ATS_QUEUE_DEPTH_MASK 0x1f
@@ -48,5 +50,27 @@ static inline int pci_ats_device(int seg, int bus, int devfn)
return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
}
+#else
+
+static inline int enable_ats_device(struct pci_dev *pdev,
+ struct list_head *ats_list)
+{
+ return -ENODEV;
+}
+
+static inline void disable_ats_device(struct pci_dev *pdev) { }
+
+static inline int pci_ats_enabled(int seg, int bus, int devfn)
+{
+ return -ENODEV;
+}
+
+static inline int pci_ats_device(int seg, int bus, int devfn)
+{
+ return -ENODEV;
+}
+
+#endif /* CONFIG_HAS_PCI_ATS */
+
#endif /* _ATS_H_ */
diff --git a/xen/drivers/passthrough/vtd/x86/Makefile b/xen/drivers/passthrough/vtd/x86/Makefile
index 4ef00a4c5b..60f79fe263 100644
--- a/xen/drivers/passthrough/vtd/x86/Makefile
+++ b/xen/drivers/passthrough/vtd/x86/Makefile
@@ -1,3 +1,3 @@
-obj-y += ats.o
+obj-$(CONFIG_HAS_PCI_ATS) += ats.o
obj-$(CONFIG_HVM) += hvm.o
obj-y += vtd.o
diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
index a70cf9460d..05e6f51f25 100644
--- a/xen/drivers/passthrough/x86/Makefile
+++ b/xen/drivers/passthrough/x86/Makefile
@@ -1,2 +1,2 @@
-obj-y += ats.o
+obj-$(CONFIG_HAS_PCI_ATS) += ats.o
obj-y += iommu.o
diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
index 7da03fa13b..1588d4a91e 100644
--- a/xen/drivers/pci/Kconfig
+++ b/xen/drivers/pci/Kconfig
@@ -1,3 +1,6 @@
config HAS_PCI
bool
+
+config HAS_PCI_ATS
+ bool
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/4] xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS functionality.
2020-10-26 17:17 ` [PATCH v1 2/4] xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS functionality Rahul Singh
@ 2020-10-27 23:55 ` Stefano Stabellini
2020-10-28 7:22 ` Jan Beulich
2020-10-28 7:25 ` Jan Beulich
1 sibling, 1 reply; 26+ messages in thread
From: Stefano Stabellini @ 2020-10-27 23:55 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, bertrand.marquis, Jan Beulich, Andrew Cooper,
Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Paul Durrant, Kevin Tian
On Mon, 26 Oct 2020, Rahul Singh wrote:
> PCI ATS functionality is not enabled and tested for ARM architecture
> but it is enabled for x86 and referenced in common passthrough/pci.c
> code.
>
> Therefore introducing the new flag to enable the ATS functionality for
> x86 only to avoid issues for ARM architecture.
>
> No functional change.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/arch/x86/Kconfig | 1 +
> xen/drivers/passthrough/ats.h | 24 ++++++++++++++++++++++++
> xen/drivers/passthrough/vtd/x86/Makefile | 2 +-
> xen/drivers/passthrough/x86/Makefile | 2 +-
> xen/drivers/pci/Kconfig | 3 +++
> 5 files changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index 24868aa6ad..31906e9c97 100644
> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -20,6 +20,7 @@ config X86
> select HAS_NS16550
> select HAS_PASSTHROUGH
> select HAS_PCI
> + select HAS_PCI_ATS
> select HAS_PDX
> select HAS_SCHED_GRANULARITY
> select HAS_UBSAN
> diff --git a/xen/drivers/passthrough/ats.h b/xen/drivers/passthrough/ats.h
> index 22ae209b37..a0af07b287 100644
> --- a/xen/drivers/passthrough/ats.h
> +++ b/xen/drivers/passthrough/ats.h
> @@ -17,6 +17,8 @@
>
> #include <xen/pci_regs.h>
>
> +#ifdef CONFIG_HAS_PCI_ATS
> +
> #define ATS_REG_CAP 4
> #define ATS_REG_CTL 6
> #define ATS_QUEUE_DEPTH_MASK 0x1f
> @@ -48,5 +50,27 @@ static inline int pci_ats_device(int seg, int bus, int devfn)
> return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
> }
>
> +#else
> +
> +static inline int enable_ats_device(struct pci_dev *pdev,
> + struct list_head *ats_list)
> +{
> + return -ENODEV;
> +}
> +
> +static inline void disable_ats_device(struct pci_dev *pdev) { }
> +
> +static inline int pci_ats_enabled(int seg, int bus, int devfn)
> +{
> + return -ENODEV;
> +}
pci_ats_enabled returns 0 when ATS is not enabled
> +static inline int pci_ats_device(int seg, int bus, int devfn)
> +{
> + return -ENODEV;
also returns 0 when ATS is not enabled
> +}
> +
> +#endif /* CONFIG_HAS_PCI_ATS */
> +
> #endif /* _ATS_H_ */
>
> diff --git a/xen/drivers/passthrough/vtd/x86/Makefile b/xen/drivers/passthrough/vtd/x86/Makefile
> index 4ef00a4c5b..60f79fe263 100644
> --- a/xen/drivers/passthrough/vtd/x86/Makefile
> +++ b/xen/drivers/passthrough/vtd/x86/Makefile
> @@ -1,3 +1,3 @@
> -obj-y += ats.o
> +obj-$(CONFIG_HAS_PCI_ATS) += ats.o
> obj-$(CONFIG_HVM) += hvm.o
> obj-y += vtd.o
> diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
> index a70cf9460d..05e6f51f25 100644
> --- a/xen/drivers/passthrough/x86/Makefile
> +++ b/xen/drivers/passthrough/x86/Makefile
> @@ -1,2 +1,2 @@
> -obj-y += ats.o
> +obj-$(CONFIG_HAS_PCI_ATS) += ats.o
> obj-y += iommu.o
> diff --git a/xen/drivers/pci/Kconfig b/xen/drivers/pci/Kconfig
> index 7da03fa13b..1588d4a91e 100644
> --- a/xen/drivers/pci/Kconfig
> +++ b/xen/drivers/pci/Kconfig
> @@ -1,3 +1,6 @@
>
> config HAS_PCI
> bool
> +
> +config HAS_PCI_ATS
> + bool
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 2/4] xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS functionality.
2020-10-27 23:55 ` Stefano Stabellini
@ 2020-10-28 7:22 ` Jan Beulich
0 siblings, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2020-10-28 7:22 UTC (permalink / raw)
To: Stefano Stabellini, Rahul Singh
Cc: xen-devel, bertrand.marquis, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall, Paul Durrant,
Kevin Tian
On 28.10.2020 00:55, Stefano Stabellini wrote:
> On Mon, 26 Oct 2020, Rahul Singh wrote:
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -20,6 +20,7 @@ config X86
>> select HAS_NS16550
>> select HAS_PASSTHROUGH
>> select HAS_PCI
>> + select HAS_PCI_ATS
From an abstract perspective - is there a way to have a PCI
subsystem where it is impossible for an ATS-capable devices to
be present? I think this shouldn't be a HAS_* option, but an
ordinary user selectable one. We may then even want to consider
having it off by default on x86 (the command line option
defaults to off as well, after all).
>> @@ -48,5 +50,27 @@ static inline int pci_ats_device(int seg, int bus, int devfn)
>> return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
>> }
>>
>> +#else
>> +
>> +static inline int enable_ats_device(struct pci_dev *pdev,
>> + struct list_head *ats_list)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> +static inline void disable_ats_device(struct pci_dev *pdev) { }
>> +
>> +static inline int pci_ats_enabled(int seg, int bus, int devfn)
>> +{
>> + return -ENODEV;
>> +}
>
> pci_ats_enabled returns 0 when ATS is not enabled
>
>
>> +static inline int pci_ats_device(int seg, int bus, int devfn)
>> +{
>> + return -ENODEV;
>
> also returns 0 when ATS is not enabled
And really both ought to be returning bool.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 2/4] xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS functionality.
2020-10-26 17:17 ` [PATCH v1 2/4] xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS functionality Rahul Singh
2020-10-27 23:55 ` Stefano Stabellini
@ 2020-10-28 7:25 ` Jan Beulich
1 sibling, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2020-10-28 7:25 UTC (permalink / raw)
To: Rahul Singh
Cc: bertrand.marquis, Andrew Cooper, Roger Pau Monné,
Wei Liu, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Paul Durrant, Kevin Tian, xen-devel
On 26.10.2020 18:17, Rahul Singh wrote:
> @@ -48,5 +50,27 @@ static inline int pci_ats_device(int seg, int bus, int devfn)
> return pci_find_ext_capability(seg, bus, devfn, PCI_EXT_CAP_ID_ATS);
> }
>
> +#else
> +
> +static inline int enable_ats_device(struct pci_dev *pdev,
> + struct list_head *ats_list)
> +{
> + return -ENODEV;
In addition to what Stefano has said, I don't think this is an
appropriate return value here either - -EOPNOTSUPP would seem
quite a bit closer to reality.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory.
2020-10-26 17:17 [PATCH v1 0/4] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
2020-10-26 17:17 ` [PATCH v1 1/4] xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI enabled Rahul Singh
2020-10-26 17:17 ` [PATCH v1 2/4] xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS functionality Rahul Singh
@ 2020-10-26 17:17 ` Rahul Singh
2020-10-28 0:15 ` Stefano Stabellini
2020-10-28 11:51 ` Jan Beulich
2020-10-26 17:17 ` [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled Rahul Singh
2020-10-28 0:04 ` [PATCH v1 0/4] xen/arm: Make PCI passthrough code non-x86 specific Stefano Stabellini
4 siblings, 2 replies; 26+ messages in thread
From: Rahul Singh @ 2020-10-26 17:17 UTC (permalink / raw)
To: xen-devel
Cc: bertrand.marquis, Jan Beulich, Paul Durrant, Andrew Cooper,
George Dunlap, Ian Jackson, Julien Grall, Stefano Stabellini,
Wei Liu
passthrough/pci.c file is common for all architecture, but there is x86
sepcific code in this file.
Move x86 specific code to the x86 directory to avoid compilation error
for other architecture.
No functional change.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
xen/drivers/passthrough/pci.c | 75 +--------------------
xen/drivers/passthrough/x86/Makefile | 1 +
xen/drivers/passthrough/x86/iommu.c | 7 ++
xen/drivers/passthrough/x86/pci.c | 97 ++++++++++++++++++++++++++++
xen/include/xen/pci.h | 2 +
5 files changed, 108 insertions(+), 74 deletions(-)
create mode 100644 xen/drivers/passthrough/x86/pci.c
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 2a3bce1462..c6fbb7172c 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -24,7 +24,6 @@
#include <xen/irq.h>
#include <xen/param.h>
#include <xen/vm_event.h>
-#include <asm/hvm/irq.h>
#include <xen/delay.h>
#include <xen/keyhandler.h>
#include <xen/event.h>
@@ -847,71 +846,6 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
return ret;
}
-static int pci_clean_dpci_irq(struct domain *d,
- struct hvm_pirq_dpci *pirq_dpci, void *arg)
-{
- struct dev_intx_gsi_link *digl, *tmp;
-
- pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
-
- if ( pt_irq_need_timer(pirq_dpci->flags) )
- kill_timer(&pirq_dpci->timer);
-
- list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
- {
- list_del(&digl->list);
- xfree(digl);
- }
-
- radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
-
- if ( !pt_pirq_softirq_active(pirq_dpci) )
- return 0;
-
- domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
-
- return -ERESTART;
-}
-
-static int pci_clean_dpci_irqs(struct domain *d)
-{
- struct hvm_irq_dpci *hvm_irq_dpci = NULL;
-
- if ( !is_iommu_enabled(d) )
- return 0;
-
- if ( !is_hvm_domain(d) )
- return 0;
-
- spin_lock(&d->event_lock);
- hvm_irq_dpci = domain_get_irq_dpci(d);
- if ( hvm_irq_dpci != NULL )
- {
- int ret = 0;
-
- if ( hvm_irq_dpci->pending_pirq_dpci )
- {
- if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
- ret = -ERESTART;
- else
- hvm_irq_dpci->pending_pirq_dpci = NULL;
- }
-
- if ( !ret )
- ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
- if ( ret )
- {
- spin_unlock(&d->event_lock);
- return ret;
- }
-
- hvm_domain_irq(d)->dpci = NULL;
- free_hvm_irq_dpci(hvm_irq_dpci);
- }
- spin_unlock(&d->event_lock);
- return 0;
-}
-
/* Caller should hold the pcidevs_lock */
static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
uint8_t devfn)
@@ -971,7 +905,7 @@ int pci_release_devices(struct domain *d)
int ret;
pcidevs_lock();
- ret = pci_clean_dpci_irqs(d);
+ ret = arch_pci_release_devices(d);
if ( ret )
{
pcidevs_unlock();
@@ -1375,13 +1309,6 @@ static int __init setup_dump_pcidevs(void)
}
__initcall(setup_dump_pcidevs);
-int iommu_update_ire_from_msi(
- struct msi_desc *msi_desc, struct msi_msg *msg)
-{
- return iommu_intremap
- ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
-}
-
static int iommu_add_device(struct pci_dev *pdev)
{
const struct domain_iommu *hd;
diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
index 05e6f51f25..642f673ed2 100644
--- a/xen/drivers/passthrough/x86/Makefile
+++ b/xen/drivers/passthrough/x86/Makefile
@@ -1,2 +1,3 @@
obj-$(CONFIG_HAS_PCI_ATS) += ats.o
obj-y += iommu.o
+obj-y += pci.o
diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
index f17b1820f4..875e67b53b 100644
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -308,6 +308,13 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
return pg;
}
+int iommu_update_ire_from_msi(
+ struct msi_desc *msi_desc, struct msi_msg *msg)
+{
+ return iommu_intremap
+ ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/drivers/passthrough/x86/pci.c b/xen/drivers/passthrough/x86/pci.c
new file mode 100644
index 0000000000..443712aa22
--- /dev/null
+++ b/xen/drivers/passthrough/x86/pci.c
@@ -0,0 +1,97 @@
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/param.h>
+#include <xen/sched.h>
+#include <xen/pci.h>
+#include <xen/pci_regs.h>
+
+static int pci_clean_dpci_irq(struct domain *d,
+ struct hvm_pirq_dpci *pirq_dpci, void *arg)
+{
+ struct dev_intx_gsi_link *digl, *tmp;
+
+ pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
+
+ if ( pt_irq_need_timer(pirq_dpci->flags) )
+ kill_timer(&pirq_dpci->timer);
+
+ list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
+ {
+ list_del(&digl->list);
+ xfree(digl);
+ }
+
+ radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
+
+ if ( !pt_pirq_softirq_active(pirq_dpci) )
+ return 0;
+
+ domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
+
+ return -ERESTART;
+}
+
+static int pci_clean_dpci_irqs(struct domain *d)
+{
+ struct hvm_irq_dpci *hvm_irq_dpci = NULL;
+
+ if ( !is_iommu_enabled(d) )
+ return 0;
+
+ if ( !is_hvm_domain(d) )
+ return 0;
+
+ spin_lock(&d->event_lock);
+ hvm_irq_dpci = domain_get_irq_dpci(d);
+ if ( hvm_irq_dpci != NULL )
+ {
+ int ret = 0;
+
+ if ( hvm_irq_dpci->pending_pirq_dpci )
+ {
+ if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
+ ret = -ERESTART;
+ else
+ hvm_irq_dpci->pending_pirq_dpci = NULL;
+ }
+
+ if ( !ret )
+ ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
+ if ( ret )
+ {
+ spin_unlock(&d->event_lock);
+ return ret;
+ }
+
+ hvm_domain_irq(d)->dpci = NULL;
+ free_hvm_irq_dpci(hvm_irq_dpci);
+ }
+ spin_unlock(&d->event_lock);
+ return 0;
+}
+
+int arch_pci_release_devices(struct domain *d)
+{
+ return pci_clean_dpci_irqs(d);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 2bc4aaf453..13ae7cc2a5 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -212,4 +212,6 @@ int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
void msixtbl_pt_unregister(struct domain *, struct pirq *);
void msixtbl_pt_cleanup(struct domain *d);
+int arch_pci_release_devices(struct domain *d);
+
#endif /* __XEN_PCI_H__ */
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory.
2020-10-26 17:17 ` [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory Rahul Singh
@ 2020-10-28 0:15 ` Stefano Stabellini
2020-10-28 11:51 ` Jan Beulich
1 sibling, 0 replies; 26+ messages in thread
From: Stefano Stabellini @ 2020-10-28 0:15 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, bertrand.marquis, Jan Beulich, Paul Durrant,
Andrew Cooper, George Dunlap, Ian Jackson, Julien Grall,
Stefano Stabellini, Wei Liu
On Mon, 26 Oct 2020, Rahul Singh wrote:
> passthrough/pci.c file is common for all architecture, but there is x86
> sepcific code in this file.
^ specific
> Move x86 specific code to the x86 directory to avoid compilation error
> for other architecture.
>
> No functional change.
>
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> xen/drivers/passthrough/pci.c | 75 +--------------------
> xen/drivers/passthrough/x86/Makefile | 1 +
> xen/drivers/passthrough/x86/iommu.c | 7 ++
> xen/drivers/passthrough/x86/pci.c | 97 ++++++++++++++++++++++++++++
> xen/include/xen/pci.h | 2 +
> 5 files changed, 108 insertions(+), 74 deletions(-)
> create mode 100644 xen/drivers/passthrough/x86/pci.c
>
> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
> index 2a3bce1462..c6fbb7172c 100644
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -24,7 +24,6 @@
> #include <xen/irq.h>
> #include <xen/param.h>
> #include <xen/vm_event.h>
> -#include <asm/hvm/irq.h>
> #include <xen/delay.h>
> #include <xen/keyhandler.h>
> #include <xen/event.h>
sched.h is not needed anymore among the #include
> @@ -847,71 +846,6 @@ int pci_remove_device(u16 seg, u8 bus, u8 devfn)
> return ret;
> }
>
> -static int pci_clean_dpci_irq(struct domain *d,
> - struct hvm_pirq_dpci *pirq_dpci, void *arg)
> -{
> - struct dev_intx_gsi_link *digl, *tmp;
> -
> - pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
> -
> - if ( pt_irq_need_timer(pirq_dpci->flags) )
> - kill_timer(&pirq_dpci->timer);
> -
> - list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> - {
> - list_del(&digl->list);
> - xfree(digl);
> - }
> -
> - radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
> -
> - if ( !pt_pirq_softirq_active(pirq_dpci) )
> - return 0;
> -
> - domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
> -
> - return -ERESTART;
> -}
> -
> -static int pci_clean_dpci_irqs(struct domain *d)
> -{
> - struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> -
> - if ( !is_iommu_enabled(d) )
> - return 0;
> -
> - if ( !is_hvm_domain(d) )
> - return 0;
> -
> - spin_lock(&d->event_lock);
> - hvm_irq_dpci = domain_get_irq_dpci(d);
> - if ( hvm_irq_dpci != NULL )
> - {
> - int ret = 0;
> -
> - if ( hvm_irq_dpci->pending_pirq_dpci )
> - {
> - if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
> - ret = -ERESTART;
> - else
> - hvm_irq_dpci->pending_pirq_dpci = NULL;
> - }
> -
> - if ( !ret )
> - ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
> - if ( ret )
> - {
> - spin_unlock(&d->event_lock);
> - return ret;
> - }
> -
> - hvm_domain_irq(d)->dpci = NULL;
> - free_hvm_irq_dpci(hvm_irq_dpci);
> - }
> - spin_unlock(&d->event_lock);
> - return 0;
> -}
> -
> /* Caller should hold the pcidevs_lock */
> static int deassign_device(struct domain *d, uint16_t seg, uint8_t bus,
> uint8_t devfn)
> @@ -971,7 +905,7 @@ int pci_release_devices(struct domain *d)
> int ret;
>
> pcidevs_lock();
> - ret = pci_clean_dpci_irqs(d);
> + ret = arch_pci_release_devices(d);
> if ( ret )
> {
> pcidevs_unlock();
> @@ -1375,13 +1309,6 @@ static int __init setup_dump_pcidevs(void)
> }
> __initcall(setup_dump_pcidevs);
>
> -int iommu_update_ire_from_msi(
> - struct msi_desc *msi_desc, struct msi_msg *msg)
> -{
> - return iommu_intremap
> - ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> -}
> -
> static int iommu_add_device(struct pci_dev *pdev)
> {
> const struct domain_iommu *hd;
> diff --git a/xen/drivers/passthrough/x86/Makefile b/xen/drivers/passthrough/x86/Makefile
> index 05e6f51f25..642f673ed2 100644
> --- a/xen/drivers/passthrough/x86/Makefile
> +++ b/xen/drivers/passthrough/x86/Makefile
> @@ -1,2 +1,3 @@
> obj-$(CONFIG_HAS_PCI_ATS) += ats.o
> obj-y += iommu.o
> +obj-y += pci.o
> diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c
> index f17b1820f4..875e67b53b 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -308,6 +308,13 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
> return pg;
> }
>
> +int iommu_update_ire_from_msi(
> + struct msi_desc *msi_desc, struct msi_msg *msg)
> +{
> + return iommu_intremap
> + ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/drivers/passthrough/x86/pci.c b/xen/drivers/passthrough/x86/pci.c
> new file mode 100644
> index 0000000000..443712aa22
> --- /dev/null
> +++ b/xen/drivers/passthrough/x86/pci.c
> @@ -0,0 +1,97 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/param.h>
not needed
> +#include <xen/sched.h>
> +#include <xen/pci.h>
> +#include <xen/pci_regs.h>
not needed
> +static int pci_clean_dpci_irq(struct domain *d,
> + struct hvm_pirq_dpci *pirq_dpci, void *arg)
> +{
> + struct dev_intx_gsi_link *digl, *tmp;
> +
> + pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
> +
> + if ( pt_irq_need_timer(pirq_dpci->flags) )
> + kill_timer(&pirq_dpci->timer);
> +
> + list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> + {
> + list_del(&digl->list);
> + xfree(digl);
> + }
> +
> + radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
> +
> + if ( !pt_pirq_softirq_active(pirq_dpci) )
> + return 0;
> +
> + domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
> +
> + return -ERESTART;
> +}
> +
> +static int pci_clean_dpci_irqs(struct domain *d)
> +{
> + struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +
> + if ( !is_iommu_enabled(d) )
> + return 0;
> +
> + if ( !is_hvm_domain(d) )
> + return 0;
> +
> + spin_lock(&d->event_lock);
> + hvm_irq_dpci = domain_get_irq_dpci(d);
> + if ( hvm_irq_dpci != NULL )
> + {
> + int ret = 0;
> +
> + if ( hvm_irq_dpci->pending_pirq_dpci )
> + {
> + if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
> + ret = -ERESTART;
> + else
> + hvm_irq_dpci->pending_pirq_dpci = NULL;
> + }
> +
> + if ( !ret )
> + ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
> + if ( ret )
> + {
> + spin_unlock(&d->event_lock);
> + return ret;
> + }
> +
> + hvm_domain_irq(d)->dpci = NULL;
> + free_hvm_irq_dpci(hvm_irq_dpci);
> + }
> + spin_unlock(&d->event_lock);
> + return 0;
> +}
> +
> +int arch_pci_release_devices(struct domain *d)
> +{
> + return pci_clean_dpci_irqs(d);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
> index 2bc4aaf453..13ae7cc2a5 100644
> --- a/xen/include/xen/pci.h
> +++ b/xen/include/xen/pci.h
> @@ -212,4 +212,6 @@ int msixtbl_pt_register(struct domain *, struct pirq *, uint64_t gtable);
> void msixtbl_pt_unregister(struct domain *, struct pirq *);
> void msixtbl_pt_cleanup(struct domain *d);
>
> +int arch_pci_release_devices(struct domain *d);
> +
> #endif /* __XEN_PCI_H__ */
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory.
2020-10-26 17:17 ` [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory Rahul Singh
2020-10-28 0:15 ` Stefano Stabellini
@ 2020-10-28 11:51 ` Jan Beulich
2020-10-28 11:58 ` Julien Grall
2020-10-28 15:20 ` Rahul Singh
1 sibling, 2 replies; 26+ messages in thread
From: Jan Beulich @ 2020-10-28 11:51 UTC (permalink / raw)
To: Rahul Singh
Cc: bertrand.marquis, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel
On 26.10.2020 18:17, Rahul Singh wrote:
> passthrough/pci.c file is common for all architecture, but there is x86
> sepcific code in this file.
The code you move doesn't look to be x86 specific in the sense that
it makes no sense on other architectures, but just because certain
pieces are missing on Arm. With this I question whether is it really
appropriate to move this code. I do realize that in similar earlier
cases my questioning was mostly ignored ...
> --- /dev/null
> +++ b/xen/drivers/passthrough/x86/pci.c
> @@ -0,0 +1,97 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/param.h>
> +#include <xen/sched.h>
> +#include <xen/pci.h>
> +#include <xen/pci_regs.h>
> +
> +static int pci_clean_dpci_irq(struct domain *d,
> + struct hvm_pirq_dpci *pirq_dpci, void *arg)
> +{
> + struct dev_intx_gsi_link *digl, *tmp;
> +
> + pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
> +
> + if ( pt_irq_need_timer(pirq_dpci->flags) )
> + kill_timer(&pirq_dpci->timer);
> +
> + list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
> + {
> + list_del(&digl->list);
> + xfree(digl);
> + }
> +
> + radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
> +
> + if ( !pt_pirq_softirq_active(pirq_dpci) )
> + return 0;
> +
> + domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
> +
> + return -ERESTART;
> +}
> +
> +static int pci_clean_dpci_irqs(struct domain *d)
> +{
> + struct hvm_irq_dpci *hvm_irq_dpci = NULL;
> +
> + if ( !is_iommu_enabled(d) )
> + return 0;
> +
> + if ( !is_hvm_domain(d) )
> + return 0;
> +
> + spin_lock(&d->event_lock);
> + hvm_irq_dpci = domain_get_irq_dpci(d);
> + if ( hvm_irq_dpci != NULL )
> + {
> + int ret = 0;
> +
> + if ( hvm_irq_dpci->pending_pirq_dpci )
> + {
> + if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
> + ret = -ERESTART;
> + else
> + hvm_irq_dpci->pending_pirq_dpci = NULL;
> + }
> +
> + if ( !ret )
> + ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
> + if ( ret )
> + {
> + spin_unlock(&d->event_lock);
> + return ret;
> + }
> +
> + hvm_domain_irq(d)->dpci = NULL;
> + free_hvm_irq_dpci(hvm_irq_dpci);
> + }
> + spin_unlock(&d->event_lock);
> + return 0;
While moving please add the missing blank line before the main
return statement of the function.
> +}
> +
> +int arch_pci_release_devices(struct domain *d)
> +{
> + return pci_clean_dpci_irqs(d);
> +}
Why the extra function layer?
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory.
2020-10-28 11:51 ` Jan Beulich
@ 2020-10-28 11:58 ` Julien Grall
2020-10-28 15:20 ` Rahul Singh
1 sibling, 0 replies; 26+ messages in thread
From: Julien Grall @ 2020-10-28 11:58 UTC (permalink / raw)
To: Jan Beulich, Rahul Singh
Cc: bertrand.marquis, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Stefano Stabellini, Wei Liu, xen-devel
On 28/10/2020 11:51, Jan Beulich wrote:
> On 26.10.2020 18:17, Rahul Singh wrote:
>> passthrough/pci.c file is common for all architecture, but there is x86
>> sepcific code in this file.
>
> The code you move doesn't look to be x86 specific in the sense that
> it makes no sense on other architectures, but just because certain
> pieces are missing on Arm. With this I question whether is it really
> appropriate to move this code. I do realize that in similar earlier
> cases my questioning was mostly ignored ...
There are no plan to get support for PIRQ on Arm. All the interrupts
will be properly sent to the guest using a virtual interrupt controller.
Regarding the code itself, there are still a few bits that are x86
specific (see struct dev_intx_gsi_link). So I think the right action
for now is to move the code to an x86 directory.
This can be adjusted in the future if there is another architecture that
would require to use PIRQ.
Cheers,
--
Julien Grall
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory.
2020-10-28 11:51 ` Jan Beulich
2020-10-28 11:58 ` Julien Grall
@ 2020-10-28 15:20 ` Rahul Singh
2020-10-28 15:37 ` Jan Beulich
1 sibling, 1 reply; 26+ messages in thread
From: Rahul Singh @ 2020-10-28 15:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Bertrand Marquis, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel
Hello Jan,
> On 28 Oct 2020, at 11:51 am, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 26.10.2020 18:17, Rahul Singh wrote:
>> passthrough/pci.c file is common for all architecture, but there is x86
>> sepcific code in this file.
>
> The code you move doesn't look to be x86 specific in the sense that
> it makes no sense on other architectures, but just because certain
> pieces are missing on Arm. With this I question whether is it really
> appropriate to move this code. I do realize that in similar earlier
> cases my questioning was mostly ignored ...
>
>> --- /dev/null
>> +++ b/xen/drivers/passthrough/x86/pci.c
>> @@ -0,0 +1,97 @@
>> +/*
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program; If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/param.h>
>> +#include <xen/sched.h>
>> +#include <xen/pci.h>
>> +#include <xen/pci_regs.h>
>> +
>> +static int pci_clean_dpci_irq(struct domain *d,
>> + struct hvm_pirq_dpci *pirq_dpci, void *arg)
>> +{
>> + struct dev_intx_gsi_link *digl, *tmp;
>> +
>> + pirq_guest_unbind(d, dpci_pirq(pirq_dpci));
>> +
>> + if ( pt_irq_need_timer(pirq_dpci->flags) )
>> + kill_timer(&pirq_dpci->timer);
>> +
>> + list_for_each_entry_safe ( digl, tmp, &pirq_dpci->digl_list, list )
>> + {
>> + list_del(&digl->list);
>> + xfree(digl);
>> + }
>> +
>> + radix_tree_delete(&d->pirq_tree, dpci_pirq(pirq_dpci)->pirq);
>> +
>> + if ( !pt_pirq_softirq_active(pirq_dpci) )
>> + return 0;
>> +
>> + domain_get_irq_dpci(d)->pending_pirq_dpci = pirq_dpci;
>> +
>> + return -ERESTART;
>> +}
>> +
>> +static int pci_clean_dpci_irqs(struct domain *d)
>> +{
>> + struct hvm_irq_dpci *hvm_irq_dpci = NULL;
>> +
>> + if ( !is_iommu_enabled(d) )
>> + return 0;
>> +
>> + if ( !is_hvm_domain(d) )
>> + return 0;
>> +
>> + spin_lock(&d->event_lock);
>> + hvm_irq_dpci = domain_get_irq_dpci(d);
>> + if ( hvm_irq_dpci != NULL )
>> + {
>> + int ret = 0;
>> +
>> + if ( hvm_irq_dpci->pending_pirq_dpci )
>> + {
>> + if ( pt_pirq_softirq_active(hvm_irq_dpci->pending_pirq_dpci) )
>> + ret = -ERESTART;
>> + else
>> + hvm_irq_dpci->pending_pirq_dpci = NULL;
>> + }
>> +
>> + if ( !ret )
>> + ret = pt_pirq_iterate(d, pci_clean_dpci_irq, NULL);
>> + if ( ret )
>> + {
>> + spin_unlock(&d->event_lock);
>> + return ret;
>> + }
>> +
>> + hvm_domain_irq(d)->dpci = NULL;
>> + free_hvm_irq_dpci(hvm_irq_dpci);
>> + }
>> + spin_unlock(&d->event_lock);
>> + return 0;
>
> While moving please add the missing blank line before the main
> return statement of the function.
Ok I will fix that in next version.
>
>> +}
>> +
>> +int arch_pci_release_devices(struct domain *d)
>> +{
>> + return pci_clean_dpci_irqs(d);
>> +}
>
> Why the extra function layer?
Is that ok if I rename the function pci_clean_dpci_irqs() to arch_pci_clean_pirqs() ?
>
> Jan
>
Regards,
Rahul
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory.
2020-10-28 15:20 ` Rahul Singh
@ 2020-10-28 15:37 ` Jan Beulich
0 siblings, 0 replies; 26+ messages in thread
From: Jan Beulich @ 2020-10-28 15:37 UTC (permalink / raw)
To: Rahul Singh
Cc: Bertrand Marquis, Paul Durrant, Andrew Cooper, George Dunlap,
Ian Jackson, Julien Grall, Stefano Stabellini, Wei Liu,
xen-devel
On 28.10.2020 16:20, Rahul Singh wrote:
>> On 28 Oct 2020, at 11:51 am, Jan Beulich <jbeulich@suse.com> wrote:
>> On 26.10.2020 18:17, Rahul Singh wrote:
>>> +int arch_pci_release_devices(struct domain *d)
>>> +{
>>> + return pci_clean_dpci_irqs(d);
>>> +}
>>
>> Why the extra function layer?
>
> Is that ok if I rename the function pci_clean_dpci_irqs() to arch_pci_clean_pirqs() ?
I see no problem with you doing so.
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled.
2020-10-26 17:17 [PATCH v1 0/4] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
` (2 preceding siblings ...)
2020-10-26 17:17 ` [PATCH v1 3/4] xen/pci: Move x86 specific code to x86 directory Rahul Singh
@ 2020-10-26 17:17 ` Rahul Singh
2020-10-28 11:56 ` Jan Beulich
2020-10-28 0:04 ` [PATCH v1 0/4] xen/arm: Make PCI passthrough code non-x86 specific Stefano Stabellini
4 siblings, 1 reply; 26+ messages in thread
From: Rahul Singh @ 2020-10-26 17:17 UTC (permalink / raw)
To: xen-devel; +Cc: bertrand.marquis, Jan Beulich, Paul Durrant
d->vm_event_paging struct is defined under CONFIG_HAS_MEM_PAGING in
sched.h but referenced in passthrough/pci.c directly.
If CONFIG_HAS_MEM_PAGING is not enabled for architecture, compiler will
throws an error.
No functional change.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
xen/drivers/passthrough/pci.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index c6fbb7172c..3125c23e87 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1419,13 +1419,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
if ( !is_iommu_enabled(d) )
return 0;
- /* Prevent device assign if mem paging or mem sharing have been
+#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING)
+ /* Prevent device assign if mem paging or mem sharing have been
* enabled for this domain */
if ( d != dom_io &&
unlikely(mem_sharing_enabled(d) ||
vm_event_check_ring(d->vm_event_paging) ||
p2m_get_hostp2m(d)->global_logdirty) )
return -EXDEV;
+#endif
/* device_assigned() should already have cleared the device for assignment */
ASSERT(pcidevs_locked());
--
2.17.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled.
2020-10-26 17:17 ` [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled Rahul Singh
@ 2020-10-28 11:56 ` Jan Beulich
2020-10-28 15:13 ` Rahul Singh
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2020-10-28 11:56 UTC (permalink / raw)
To: Rahul Singh; +Cc: bertrand.marquis, Paul Durrant, xen-devel
On 26.10.2020 18:17, Rahul Singh wrote:
> --- a/xen/drivers/passthrough/pci.c
> +++ b/xen/drivers/passthrough/pci.c
> @@ -1419,13 +1419,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
> if ( !is_iommu_enabled(d) )
> return 0;
>
> - /* Prevent device assign if mem paging or mem sharing have been
> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING)
> + /* Prevent device assign if mem paging or mem sharing have been
> * enabled for this domain */
> if ( d != dom_io &&
> unlikely(mem_sharing_enabled(d) ||
> vm_event_check_ring(d->vm_event_paging) ||
> p2m_get_hostp2m(d)->global_logdirty) )
> return -EXDEV;
> +#endif
Besides this also disabling mem-sharing and log-dirty related
logic, I don't think the change is correct: Each item being
checked needs individually disabling depending on its associated
CONFIG_*. For this, perhaps you want to introduce something
like mem_paging_enabled(d), to avoid the need for #ifdef here?
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled.
2020-10-28 11:56 ` Jan Beulich
@ 2020-10-28 15:13 ` Rahul Singh
2020-10-29 16:58 ` Rahul Singh
0 siblings, 1 reply; 26+ messages in thread
From: Rahul Singh @ 2020-10-28 15:13 UTC (permalink / raw)
To: Jan Beulich; +Cc: Bertrand Marquis, Paul Durrant, xen-devel
Hello Jan,
> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 26.10.2020 18:17, Rahul Singh wrote:
>> --- a/xen/drivers/passthrough/pci.c
>> +++ b/xen/drivers/passthrough/pci.c
>> @@ -1419,13 +1419,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>> if ( !is_iommu_enabled(d) )
>> return 0;
>>
>> - /* Prevent device assign if mem paging or mem sharing have been
>> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING)
>> + /* Prevent device assign if mem paging or mem sharing have been
>> * enabled for this domain */
>> if ( d != dom_io &&
>> unlikely(mem_sharing_enabled(d) ||
>> vm_event_check_ring(d->vm_event_paging) ||
>> p2m_get_hostp2m(d)->global_logdirty) )
>> return -EXDEV;
>> +#endif
>
> Besides this also disabling mem-sharing and log-dirty related
> logic, I don't think the change is correct: Each item being
> checked needs individually disabling depending on its associated
> CONFIG_*. For this, perhaps you want to introduce something
> like mem_paging_enabled(d), to avoid the need for #ifdef here?
>
Ok I will fix that in next version.
> Jan
Regards,
Rahul
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled.
2020-10-28 15:13 ` Rahul Singh
@ 2020-10-29 16:58 ` Rahul Singh
2020-10-29 17:16 ` Jan Beulich
0 siblings, 1 reply; 26+ messages in thread
From: Rahul Singh @ 2020-10-29 16:58 UTC (permalink / raw)
To: Jan Beulich; +Cc: Bertrand Marquis, Paul Durrant, xen-devel
Hello Jan,
> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote:
>
> Hello Jan,
>
>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 26.10.2020 18:17, Rahul Singh wrote:
>>> --- a/xen/drivers/passthrough/pci.c
>>> +++ b/xen/drivers/passthrough/pci.c
>>> @@ -1419,13 +1419,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>> if ( !is_iommu_enabled(d) )
>>> return 0;
>>>
>>> - /* Prevent device assign if mem paging or mem sharing have been
>>> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING)
>>> + /* Prevent device assign if mem paging or mem sharing have been
>>> * enabled for this domain */
>>> if ( d != dom_io &&
>>> unlikely(mem_sharing_enabled(d) ||
>>> vm_event_check_ring(d->vm_event_paging) ||
>>> p2m_get_hostp2m(d)->global_logdirty) )
>>> return -EXDEV;
>>> +#endif
>>
>> Besides this also disabling mem-sharing and log-dirty related
>> logic, I don't think the change is correct: Each item being
>> checked needs individually disabling depending on its associated
>> CONFIG_*. For this, perhaps you want to introduce something
>> like mem_paging_enabled(d), to avoid the need for #ifdef here?
>>
>
> Ok I will fix that in next version.
I just check and found out that mem-sharing , men-paging and log-dirty is x86 specific and not implemented for ARM.
Is that will be ok if I move above code to x86 specific directory and introduce new function arch_pcidev_is_assignable() that will test if pcidev is assignable or not ?
>
>> Jan
Regards,
Rahul
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled.
2020-10-29 16:58 ` Rahul Singh
@ 2020-10-29 17:16 ` Jan Beulich
2020-10-30 13:59 ` Rahul Singh
0 siblings, 1 reply; 26+ messages in thread
From: Jan Beulich @ 2020-10-29 17:16 UTC (permalink / raw)
To: Rahul Singh; +Cc: Bertrand Marquis, Paul Durrant, xen-devel
On 29.10.2020 17:58, Rahul Singh wrote:
>> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote:
>>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> wrote:
>>> On 26.10.2020 18:17, Rahul Singh wrote:
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -1419,13 +1419,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>>> if ( !is_iommu_enabled(d) )
>>>> return 0;
>>>>
>>>> - /* Prevent device assign if mem paging or mem sharing have been
>>>> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING)
>>>> + /* Prevent device assign if mem paging or mem sharing have been
>>>> * enabled for this domain */
>>>> if ( d != dom_io &&
>>>> unlikely(mem_sharing_enabled(d) ||
>>>> vm_event_check_ring(d->vm_event_paging) ||
>>>> p2m_get_hostp2m(d)->global_logdirty) )
>>>> return -EXDEV;
>>>> +#endif
>>>
>>> Besides this also disabling mem-sharing and log-dirty related
>>> logic, I don't think the change is correct: Each item being
>>> checked needs individually disabling depending on its associated
>>> CONFIG_*. For this, perhaps you want to introduce something
>>> like mem_paging_enabled(d), to avoid the need for #ifdef here?
>>>
>>
>> Ok I will fix that in next version.
>
> I just check and found out that mem-sharing , men-paging and log-dirty is x86 specific and not implemented for ARM.
> Is that will be ok if I move above code to x86 specific directory and introduce new function arch_pcidev_is_assignable() that will test if pcidev is assignable or not ?
As an immediate workaround - perhaps (long term the individual
pieces should still be individually checked here, as they're
not inherently x86-specific). Since there's no device property
involved here, the suggested name looks misleading. Perhaps
arch_iommu_usable(d)?
Jan
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled.
2020-10-29 17:16 ` Jan Beulich
@ 2020-10-30 13:59 ` Rahul Singh
0 siblings, 0 replies; 26+ messages in thread
From: Rahul Singh @ 2020-10-30 13:59 UTC (permalink / raw)
To: Jan Beulich; +Cc: Bertrand Marquis, Paul Durrant, xen-devel
Hello Jan,
> On 29 Oct 2020, at 5:16 pm, Jan Beulich <jbeulich@suse.com> wrote:
>
> On 29.10.2020 17:58, Rahul Singh wrote:
>>> On 28 Oct 2020, at 3:13 pm, Rahul Singh <Rahul.Singh@arm.com> wrote:
>>>> On 28 Oct 2020, at 11:56 am, Jan Beulich <jbeulich@suse.com> wrote:
>>>> On 26.10.2020 18:17, Rahul Singh wrote:
>>>>> --- a/xen/drivers/passthrough/pci.c
>>>>> +++ b/xen/drivers/passthrough/pci.c
>>>>> @@ -1419,13 +1419,15 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
>>>>> if ( !is_iommu_enabled(d) )
>>>>> return 0;
>>>>>
>>>>> - /* Prevent device assign if mem paging or mem sharing have been
>>>>> +#if defined(CONFIG_HAS_MEM_PAGING) || defined(CONFIG_MEM_SHARING)
>>>>> + /* Prevent device assign if mem paging or mem sharing have been
>>>>> * enabled for this domain */
>>>>> if ( d != dom_io &&
>>>>> unlikely(mem_sharing_enabled(d) ||
>>>>> vm_event_check_ring(d->vm_event_paging) ||
>>>>> p2m_get_hostp2m(d)->global_logdirty) )
>>>>> return -EXDEV;
>>>>> +#endif
>>>>
>>>> Besides this also disabling mem-sharing and log-dirty related
>>>> logic, I don't think the change is correct: Each item being
>>>> checked needs individually disabling depending on its associated
>>>> CONFIG_*. For this, perhaps you want to introduce something
>>>> like mem_paging_enabled(d), to avoid the need for #ifdef here?
>>>>
>>>
>>> Ok I will fix that in next version.
>>
>> I just check and found out that mem-sharing , men-paging and log-dirty is x86 specific and not implemented for ARM.
>> Is that will be ok if I move above code to x86 specific directory and introduce new function arch_pcidev_is_assignable() that will test if pcidev is assignable or not ?
>
> As an immediate workaround - perhaps (long term the individual
> pieces should still be individually checked here, as they're
> not inherently x86-specific). Since there's no device property
> involved here, the suggested name looks misleading. Perhaps
> arch_iommu_usable(d)?
Thanks. I will modify the code as per suggestion and will share the version v2 for review.
>
> Jan
Regards,
Rahul
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v1 0/4] xen/arm: Make PCI passthrough code non-x86 specific
2020-10-26 17:17 [PATCH v1 0/4] xen/arm: Make PCI passthrough code non-x86 specific Rahul Singh
` (3 preceding siblings ...)
2020-10-26 17:17 ` [PATCH v1 4/4] xen/pci: solve compilation error when memory paging is not enabled Rahul Singh
@ 2020-10-28 0:04 ` Stefano Stabellini
4 siblings, 0 replies; 26+ messages in thread
From: Stefano Stabellini @ 2020-10-28 0:04 UTC (permalink / raw)
To: Rahul Singh
Cc: xen-devel, bertrand.marquis, Andrew Cooper, George Dunlap,
Ian Jackson, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
Paul Durrant, Kevin Tian
On Mon, 26 Oct 2020, Rahul Singh wrote:
> This patch series is preparatory work to make PCI passthrough code non-x86
> specific.
Do you have a simple patch I could use to test the compilation on ARM?
Even just a hack to help me review the series?
Right now I can only test that the compilation on x86 is unbroken.
Thanks!
- Stefano
> Rahul Singh (4):
> xen/ns16550: solve compilation error on ARM with CONFIG_HAS_PCI
> enabled.
> xen/pci: Introduce new CONFIG_HAS_PCI_ATS flag for PCI ATS
> functionality.
> xen/pci: Move x86 specific code to x86 directory.
> xen/pci: solve compilation error when memory paging is not enabled.
>
> xen/arch/x86/Kconfig | 1 +
> xen/drivers/char/Kconfig | 7 ++
> xen/drivers/char/ns16550.c | 32 ++++----
> xen/drivers/passthrough/ats.h | 24 ++++++
> xen/drivers/passthrough/pci.c | 79 +------------------
> xen/drivers/passthrough/vtd/x86/Makefile | 2 +-
> xen/drivers/passthrough/x86/Makefile | 3 +-
> xen/drivers/passthrough/x86/iommu.c | 7 ++
> xen/drivers/passthrough/x86/pci.c | 97 ++++++++++++++++++++++++
> xen/drivers/pci/Kconfig | 3 +
> xen/include/xen/pci.h | 2 +
> 11 files changed, 164 insertions(+), 93 deletions(-)
> create mode 100644 xen/drivers/passthrough/x86/pci.c
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread