Re: [Xen-devel] [PATCH] ns16550: make PCI device hiding uniform

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Sep 03, 2019 at 05:13:38PM +0200, Jan Beulich wrote:
> On 03.09.2019 16:15, Roger Pau Monné  wrote:
> > On Tue, Sep 03, 2019 at 03:58:08PM +0200, Jan Beulich wrote:
> >> --- a/xen/drivers/char/ns16550.c
> >> +++ b/xen/drivers/char/ns16550.c
> >> @@ -763,23 +763,16 @@ static void __init ns16550_init_postirq(
> >>  #ifdef CONFIG_HAS_PCI
> >>      if ( uart->bar || uart->ps_bdf_enable )
> >>      {
> >> -        if ( !uart->param )
> >> -            pci_hide_device(0, uart->ps_bdf[0], PCI_DEVFN(uart->ps_bdf[1],
> >> -                            uart->ps_bdf[2]));
> >> -        else
> >> -        {
> >> -            if ( uart->param->mmio &&
> >> -                 rangeset_add_range(mmio_ro_ranges,
> >> -                                    uart->io_base,
> >> -                                    uart->io_base + uart->io_size - 1) )
> >> -                printk(XENLOG_INFO "Error while adding MMIO range of 
> >> device to mmio_ro_ranges\n");
> >> +        if ( uart->param && uart->param->mmio &&
> >> +             rangeset_add_range(mmio_ro_ranges, uart->io_base,
> >> +                                uart->io_base + uart->io_size - 1) )
> >> +            printk(XENLOG_INFO "Error while adding MMIO range of device 
> >> to mmio_ro_ranges\n");
> >>  
> >> -            if ( pci_ro_device(0, uart->ps_bdf[0],
> >> -                               PCI_DEVFN(uart->ps_bdf[1], 
> >> uart->ps_bdf[2])) )
> >> -                printk(XENLOG_INFO "Could not mark config space of 
> >> %02x:%02x.%u read-only.\n",
> >> -                                    uart->ps_bdf[0], uart->ps_bdf[1],
> >> -                                    uart->ps_bdf[2]);
> >> -        }
> >> +        if ( pci_ro_device(0, uart->ps_bdf[0],
> > 
> > Don't you need to gate the call to pci_ro_device with
> > uart->ps_bdf_enable?
> 
> No, we want this for both the parse_pci() and the pci_uart_config()
> case, which is what the surrounding if() (visible in context above)
> checks. (Note also that previously there was no such check either,
> so if anything it would be an orthogonal change anyway.)

Ack, I find it wrong that pci_uart_config sets the ps_bdf but not
ps_bdf_enable. From the description of the ps_bdf_enable field it
seems like ps_bdf is only valid if ps_bdf_enable is true, but that
doesn't seem to match pci_uart_config.

The change looks fine to me based on what was done before, hence:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel