Re: [PATCH v2 2/2] drivers/char: Use sub-page ro API to make just xhci dbc cap RO

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, May 30, 2023 at 02:04:26PM +0200, Jan Beulich wrote:
> On 05.05.2023 23:25, Marek Marczykowski-Górecki wrote:
> > Not the whole page, which may contain other registers too. In fact
> > on Tiger Lake and newer (at least), this page do contain other registers
> > that Linux tries to use.
> 
> Please can you clarify whether this is with spec or an erratum? 

It is in spec. The spec is written with assumption that different driver
may drive the debug console than the rest of the controller. But the
spec (apparently) does not anticipate those drivers living in different
kernels, so does not mandate isolating registers to separate pages.

> I ask
> not the least because I continue to wonder whether we really want/need
> the non-negligible amount of new code added by path 1.
> 
> > And with share=yes, a domU would use them too.
> 
> And gain yet more access to the emulator, as mentioned in patch 1. The
> security implications may (will?) want mentioning.
> 
> > --- a/xen/drivers/char/xhci-dbc.c
> > +++ b/xen/drivers/char/xhci-dbc.c
> > @@ -1221,14 +1221,12 @@ static void __init cf_check 
> > dbc_uart_init_postirq(struct serial_port *port)
> >       * Linux's XHCI driver (as of 5.18) works without writting to the whole
> >       * page, so keep it simple.
> >       */
> > -    if ( rangeset_add_range(mmio_ro_ranges,
> > -                PFN_DOWN((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
> > -                         uart->dbc.xhc_dbc_offset),
> > -                PFN_UP((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
> > -                       uart->dbc.xhc_dbc_offset +
> > -                sizeof(*uart->dbc.dbc_reg)) - 1) )
> > -        printk(XENLOG_INFO
> > -               "Error while adding MMIO range of device to 
> > mmio_ro_ranges\n");
> > +    if ( subpage_mmio_ro_add(
> > +            (uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
> > +             uart->dbc.xhc_dbc_offset,
> > +            sizeof(*uart->dbc.dbc_reg)) )
> > +        printk(XENLOG_WARNING
> > +               "Error while marking MMIO range of XHCI console as R/O\n");
> 
> So how about falling back to just rangeset_add_range(mmio_ro_ranges, ...)
> in this failure case? (I did mention an alternative to doing it here in
> the comments on patch 1.)

Or fallback to XHCI_SHARE_NONE (IOW, pci_ro_device()).

> Also, doesn't the comment ahead of the construct become stale?

Indeed.

> Finally I think indentation of the function call arguments is off by one.

How is it supposed to be? Currently it's 8 spaces over the "if", should
it be 4 spaces over the function name?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature