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

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

On Tue, Sep 03, 2019 at 03:58:08PM +0200, Jan Beulich wrote:
> The difference between pci_hide_device() and pci_ro_device() is that
> the former only prevents a device from getting assigned to a guest,
> while the latter additionally arranges for Dom0 write attempts to the
> device's config space to be ignored/discarded. Whether we want one or
> the other certainly doesn't depend on whether the device is in our set
> of known devices. All that matters is whether we use a PCI device: Call
> pci_ro_device() in any such case.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

LGTM, just one question below.

> ---
> Resend with To/Cc corrected; thanks to Andrew for pointing out.
> 
> --- 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?

Thanks, Roger.

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