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

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

On Wed, Mar 29, 2023 at 11:14:52AM +0200, Jan Beulich wrote:
> On 27.03.2023 12:09, 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. And with share=yes, a domU would use them too.
> > Without this patch, PV dom0 would fail to initialize the controller,
> > while HVM would be killed on EPT violation.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  xen/drivers/char/xhci-dbc.c | 38 ++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 36 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
> > index 60b781f87202..df2524b0ca18 100644
> > --- a/xen/drivers/char/xhci-dbc.c
> > +++ b/xen/drivers/char/xhci-dbc.c
> > @@ -1226,9 +1226,43 @@ static void __init cf_check 
> > dbc_uart_init_postirq(struct serial_port *port)
> >                           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
> > +                sizeof(*uart->dbc.dbc_reg)) - 1) ) {
> 
> Nit: No need for a brace here (and certainly not a misplaced one).
> 
> > +        printk(XENLOG_WARNING
> 
> This log level change looks kind of unrelated.
> 
> >                 "Error while adding MMIO range of device to 
> > mmio_ro_ranges\n");
> > +    }
> > +    else
> > +    {
> > +        unsigned long dbc_regs_start = (uart->dbc.bar_val &
> > +                PCI_BASE_ADDRESS_MEM_MASK) + uart->dbc.xhc_dbc_offset;
> > +        unsigned long dbc_regs_end = dbc_regs_start + 
> > sizeof(*uart->dbc.dbc_reg);
> > +
> > +        /* This being smaller than a page simplifies conditions below */
> > +        BUILD_BUG_ON(sizeof(*uart->dbc.dbc_reg) >= PAGE_SIZE - 1);
> 
> Why PAGE_SIZE - 1 (or why >= instead of > )? If there is a reason, then
> the comment wants to be in sync.

Indeed looks like off-by-one.

> > +        if ( dbc_regs_start & (PAGE_SIZE - 1) ||
> 
> Nit: Please parenthesize the & against the || (similarly again below).
> 
> Like asked by Roger for patch 1 (iirc), here and below please use
> PAGE_OFFSET() in favor of (kind of) open-coding it.
> 
> > +                PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end) )
> 
> Nit: Style (indentation).
> 
> > +        {
> > +            if ( subpage_mmio_ro_add(
> > +                        _mfn(PFN_DOWN(dbc_regs_start)),
> > +                        dbc_regs_start & (PAGE_SIZE - 1),
> > +                        PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end)
> > +                        ? dbc_regs_end & (PAGE_SIZE - 1)
> > +                        : PAGE_SIZE - 1,
> > +                        FIX_XHCI_END) )
> 
> Nit: I think this is too deep a level of indentation; it should be a
> single level (4 blanks) from the start of the function name (also
> again another time below).
> 
> > +                printk(XENLOG_WARNING
> > +                        "Error while adding MMIO range of device to 
> > subpage_mmio_ro\n");
> 
> Nit: Style (indentation).
> 
> > +        }
> > +        if ( dbc_regs_end & (PAGE_SIZE - 1) &&
> > +                PFN_DOWN(dbc_regs_start) != PFN_DOWN(dbc_regs_end) )
> > +        {
> > +            if ( subpage_mmio_ro_add(
> > +                        _mfn(PFN_DOWN(dbc_regs_end)),
> > +                        0,
> > +                        dbc_regs_end & (PAGE_SIZE - 1),
> > +                        FIX_XHCI_END + 
> > PFN_DOWN(sizeof(*uart->dbc.dbc_reg))) )
> > +                printk(XENLOG_WARNING
> > +                        "Error while adding MMIO range of device to 
> > subpage_mmio_ro\n");
> > +        }
> > +    }
> 
> Seeing the uses it occurs to me that the interface is somewhat odd: It
> adds a r/o range to a page that is already recorded to be r/o. It would
> imo be more logical the other way around: To add an exception (writable)
> range. The only alternative would be to include the call to
> rangeset_add_range(mmio_ro_ranges, ...) as part of the new function, and
> reduce accordingly the range passed earlier in the function. But I think
> this would needlessly complicate the code there.

I'm trying to make the interface safe against multiple calls making
different parts of the page R/O. If it would mark an exception, then
handling a page where multiple regions need to be protected would be
rather cumbersome. It isn't the case for XHCI driver, but for example it
could be in MSI-X (if I'd actually use this API there).

Making subpage_mmio_ro_add() to call mmio_ro_ranges() on its own,
together with Roger's suggestion of using ioremap() internally instead
of using fixmap would make it a bit nicer (if mapping the same page with
ioremap() in addition to fixmap isn't a problem).

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

Attachment: signature.asc
Description: PGP signature