[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] ns16550: reject IRQ above nr_irqs
Hi,
On 11/03/2022 10:52, Marek Marczykowski-Górecki wrote:
On Fri, Mar 11, 2022 at 10:23:03AM +0000, Julien Grall wrote:
Hi Marek,
On 10/03/2022 16:37, Marek Marczykowski-Górecki wrote:
On Thu, Mar 10, 2022 at 04:21:50PM +0000, Julien Grall wrote:
Hi,
On 10/03/2022 16:12, Roger Pau Monné wrote:
On Thu, Mar 10, 2022 at 05:08:07PM +0100, Jan Beulich wrote:
On 10.03.2022 16:47, Roger Pau Monné wrote:
On Thu, Mar 10, 2022 at 04:23:00PM +0100, Jan Beulich wrote:
On 10.03.2022 15:34, Marek Marczykowski-Górecki wrote:
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1221,6 +1221,9 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt,
unsigned int idx)
pci_conf_read8(PCI_SBDF(0, b, d, f),
PCI_INTERRUPT_LINE) : 0;
+ if (uart->irq >= nr_irqs)
+ uart->irq = 0;
Don't you mean nr_irqs_gsi here? Also (nit) please add the missing blanks
immediately inside the parentheses.
If we use nr_irqs_gsi we will need to make the check x86 only AFAICT.
Down the road (when Arm wants to select HAS_PCI) - yes. Not necessarily
right away. After all Arm wants to have an equivalent check here then,
not merely checking against nr_irqs instead. So putting a conditional
here right away would hide the need for putting in place an Arm-specific
alternative.
Oh, I always forget Arm doesn't have CONFIG_HAS_PCI enabled just yet.
The PCI code in ns16550.c is gated by CONFIG_HAS_PCI and CONFIG_X86. I am
not sure we will ever see a support for PCI UART card in Xen on Arm.
However, if it evers happens then neither nr_irqs or nr_irqs_gsi would help
here because from the interrupt controller PoV 0xff may be a valid (GICv2
supports up to 1024 interrupts).
Is there any reason we can't explicitely check 0xff?
That's what my v0.1 did, but Roger suggested nr_irqs. And I agree,
because the value is later used (on x86) to access irq_desc array (via
irq_to_desc), which has nr_irqs size.
I think it would be better if that check is closer to who access the
irq_desc. This would be helpful for other users (I am sure this is not the
only potential place where the IRQ may be wrong). So how about moving it in
setup_irq()?
I don't like it, it's rather fragile approach (at least in the current
code base, without some refactor). There are a bunch of places using
uart->irq (even if just checking if its -1 or 0) before setup_irq()
call. This includes smp_intr_init(), which is what was the first thing
crashing with 0xff set there.
Even if the code is gated with !CONFIG_X86, it sounds wrong to me to
have such check in an UART driver. It only prevents us to do an
out-of-bound access. There are no guarantee the interrupt will be usable
(on Arm 256 is a valid interrupt).
As I wrote, I don't expect the code to be used any time soon on Arm. So
I am not going to argue too much on the approach. However, we should at
least clarify in the commit message/title that this is x86 and pci only.
Cheers,
--
Julien Grall
|
