[PATCH] ns16550: limit mapped MMIO size

[PATCH] ns16550: limit mapped MMIO size

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]

There's no point in mapping more than the memory we actually may need
to touch, and in fact the too large region could actually extend into
another device's one (which currently is benign on x86 since only a
single page gets mapped anyway, but which is a latent bug on ARM
whenever PCI support gets enabled there).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -931,6 +931,8 @@ pci_uart_config (struct ns16550 *uart, i
                         uart->io_base += bar_idx * uart_param[p].uart_offset;
                         if ( uart_param[p].base_baud )
                             uart->clock_hz = uart_param[p].base_baud * 16;
+                        size = max(8U << uart_param[p].reg_shift,
+                                   uart_param[p].uart_offset);
                         /* Set device and MMIO region read only to Dom0 */
                         uart->enable_ro = 1;
                         break;




[-- Attachment #2: ns16550-limit-size.patch --]
[-- Type: text/plain, Size: 1041 bytes --]

ns16550: limit mapped MMIO size

There's no point in mapping more than the memory we actually may need
to touch, and in fact the too large region could actually extend into
another device's one (which currently is benign on x86 since only a
single page gets mapped anyway, but which is a latent bug on ARM
whenever PCI support gets enabled there).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -931,6 +931,8 @@ pci_uart_config (struct ns16550 *uart, i
                         uart->io_base += bar_idx * uart_param[p].uart_offset;
                         if ( uart_param[p].base_baud )
                             uart->clock_hz = uart_param[p].base_baud * 16;
+                        size = max(8U << uart_param[p].reg_shift,
+                                   uart_param[p].uart_offset);
                         /* Set device and MMIO region read only to Dom0 */
                         uart->enable_ro = 1;
                         break;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] ns16550: limit mapped MMIO size

[Andrew Cooper]

On 12/11/15 15:52, Jan Beulich wrote:
> There's no point in mapping more than the memory we actually may need
> to touch, and in fact the too large region could actually extend into
> another device's one (which currently is benign on x86 since only a
> single page gets mapped anyway, but which is a latent bug on ARM
> whenever PCI support gets enabled there).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH] ns16550: limit mapped MMIO size

[Ian Campbell]

On Thu, 2015-11-12 at 08:52 -0700, Jan Beulich wrote:
> There's no point in mapping more than the memory we actually may need
> to touch, and in fact the too large region could actually extend into
> another device's one (which currently is benign on x86 since only a
> single page gets mapped anyway, but which is a latent bug on ARM
> whenever PCI support gets enabled there).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -931,6 +931,8 @@ pci_uart_config (struct ns16550 *uart, i
>                          uart->io_base += bar_idx *
> uart_param[p].uart_offset;
>                          if ( uart_param[p].base_baud )
>                              uart->clock_hz = uart_param[p].base_baud *
> 16;
> +                        size = max(8U << uart_param[p].reg_shift,
> +                                   uart_param[p].uart_offset);

I assume 8 bytes (suitably shifted as above) corresponds to the "need to
touch" set of registers, but I can't fathom the link to uart_offset, rather
than uart_offset + those 8 bytes or something like that.

Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] ns16550: limit mapped MMIO size

[Jan Beulich]

>>> On 16.11.15 at 17:52, <ian.campbell@citrix.com> wrote:
> On Thu, 2015-11-12 at 08:52 -0700, Jan Beulich wrote:
>> There's no point in mapping more than the memory we actually may need
>> to touch, and in fact the too large region could actually extend into
>> another device's one (which currently is benign on x86 since only a
>> single page gets mapped anyway, but which is a latent bug on ARM
>> whenever PCI support gets enabled there).
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/xen/drivers/char/ns16550.c
>> +++ b/xen/drivers/char/ns16550.c
>> @@ -931,6 +931,8 @@ pci_uart_config (struct ns16550 *uart, i
>>                          uart->io_base += bar_idx *
>> uart_param[p].uart_offset;
>>                          if ( uart_param[p].base_baud )
>>                              uart->clock_hz = uart_param[p].base_baud *
>> 16;
>> +                        size = max(8U << uart_param[p].reg_shift,
>> +                                   uart_param[p].uart_offset);
> 
> I assume 8 bytes (suitably shifted as above) corresponds to the "need to
> touch" set of registers, but I can't fathom the link to uart_offset, rather
> than uart_offset + those 8 bytes or something like that.

uart_offset represents the range of I/O-ports or MMIO that individual
ports are apart. Since advanced functionality may require access to
more than the legacy 8 registers range, and since everything is part of
a single BAR, it seems appropriate to use the maximum of the two
values.

Jan

Re: [PATCH] ns16550: limit mapped MMIO size

[Ian Campbell]

On Tue, 2015-11-17 at 03:19 -0700, Jan Beulich wrote:
> > > > On 16.11.15 at 17:52, <ian.campbell@citrix.com> wrote:
> > On Thu, 2015-11-12 at 08:52 -0700, Jan Beulich wrote:
> > > There's no point in mapping more than the memory we actually may need
> > > to touch, and in fact the too large region could actually extend into
> > > another device's one (which currently is benign on x86 since only a
> > > single page gets mapped anyway, but which is a latent bug on ARM
> > > whenever PCI support gets enabled there).
> > > 
> > > Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > > 
> > > --- a/xen/drivers/char/ns16550.c
> > > +++ b/xen/drivers/char/ns16550.c
> > > @@ -931,6 +931,8 @@ pci_uart_config (struct ns16550 *uart, i
> > >                          uart->io_base += bar_idx *
> > > uart_param[p].uart_offset;
> > >                          if ( uart_param[p].base_baud )
> > >                              uart->clock_hz = uart_param[p].base_baud
> > > *
> > > 16;
> > > +                        size = max(8U << uart_param[p].reg_shift,
> > > +                                   uart_param[p].uart_offset);
> > 
> > I assume 8 bytes (suitably shifted as above) corresponds to the "need
> > to
> > touch" set of registers, but I can't fathom the link to uart_offset,
> > rather
> > than uart_offset + those 8 bytes or something like that.
> 
> uart_offset represents the range of I/O-ports or MMIO that individual
> ports are apart. Since advanced functionality may require access to
> more than the legacy 8 registers range, and since everything is part of
> a single BAR, it seems appropriate to use the maximum of the two
> values.

Ah, so uart_offset might be more accurately called uart_stride or some
such.

No need to rename though, for the original patch:
Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel