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

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

[Jan Beulich]

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@suse.com>
---
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],
+                           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 ( uart->msi )
         {

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

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

[Roger Pau Monné]

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@suse.com>

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

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

[Jan Beulich]

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.)

Jan

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

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

[Roger Pau Monné]

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@citrix.com>

Thanks, Roger.

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

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

[Julien Grall]

Hi,

On 9/3/19 4:55 PM, Roger Pau Monné wrote:
> 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@citrix.com>

Acked-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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