Re: [PATCH 1/2] ns16550: reject IRQ above nr_irqs

[Julien Grall <julien@xxxxxxx>]

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()?
Cheers,

--
Julien Grall