|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v4 1/2] x86/mm: add API for marking only part of a MMIO page read only
On Tue, Jun 11, 2024 at 01:38:35PM +0200, Marek Marczykowski-Górecki wrote:
> On Tue, Jun 11, 2024 at 12:40:49PM +0200, Roger Pau Monné wrote:
> > On Wed, May 22, 2024 at 05:39:03PM +0200, Marek Marczykowski-Górecki wrote:
> > > + if ( !entry )
> > > + {
> > > + /* iter == NULL marks it was a newly allocated entry */
> > > + iter = NULL;
> > > + entry = xzalloc(struct subpage_ro_range);
> > > + if ( !entry )
> > > + return -ENOMEM;
> > > + entry->mfn = mfn;
> > > + }
> > > +
> > > + for ( i = offset_s; i <= offset_e; i += MMIO_RO_SUBPAGE_GRAN )
> > > + {
> > > + bool oldbit = __test_and_set_bit(i / MMIO_RO_SUBPAGE_GRAN,
> > > + entry->ro_elems);
> > > + ASSERT(!oldbit);
> > > + }
> > > +
> > > + if ( !iter )
> > > + list_add(&entry->list, &subpage_ro_ranges);
> > > +
> > > + return iter ? 1 : 0;
> > > +}
> > > +
> > > +/* This needs subpage_ro_lock already taken */
> > > +static void __init subpage_mmio_ro_remove_page(
> > > + mfn_t mfn,
> > > + unsigned int offset_s,
> > > + unsigned int offset_e)
> > > +{
> > > + struct subpage_ro_range *entry = NULL, *iter;
> > > + unsigned int i;
> > > +
> > > + list_for_each_entry(iter, &subpage_ro_ranges, list)
> > > + {
> > > + if ( mfn_eq(iter->mfn, mfn) )
> > > + {
> > > + entry = iter;
> > > + break;
> > > + }
> > > + }
> > > + if ( !entry )
> > > + return;
> > > +
> > > + for ( i = offset_s; i <= offset_e; i += MMIO_RO_SUBPAGE_GRAN )
> > > + __clear_bit(i / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems);
> > > +
> > > + if ( !bitmap_empty(entry->ro_elems, PAGE_SIZE /
> > > MMIO_RO_SUBPAGE_GRAN) )
> > > + return;
> > > +
> > > + list_del(&entry->list);
> > > + if ( entry->mapped )
> > > + iounmap(entry->mapped);
> > > + xfree(entry);
> > > +}
> > > +
> > > +int __init subpage_mmio_ro_add(
> > > + paddr_t start,
> > > + size_t size)
> > > +{
> > > + mfn_t mfn_start = maddr_to_mfn(start);
> > > + paddr_t end = start + size - 1;
> > > + mfn_t mfn_end = maddr_to_mfn(end);
> > > + unsigned int offset_end = 0;
> > > + int rc;
> > > + bool subpage_start, subpage_end;
> > > +
> > > + ASSERT(IS_ALIGNED(start, MMIO_RO_SUBPAGE_GRAN));
> > > + ASSERT(IS_ALIGNED(size, MMIO_RO_SUBPAGE_GRAN));
> > > + if ( !IS_ALIGNED(size, MMIO_RO_SUBPAGE_GRAN) )
> > > + size = ROUNDUP(size, MMIO_RO_SUBPAGE_GRAN);
> > > +
> > > + if ( !size )
> > > + return 0;
> > > +
> > > + if ( mfn_eq(mfn_start, mfn_end) )
> > > + {
> > > + /* Both starting and ending parts handled at once */
> > > + subpage_start = PAGE_OFFSET(start) || PAGE_OFFSET(end) !=
> > > PAGE_SIZE - 1;
> > > + subpage_end = false;
> >
> > Given the intended usage of this, don't we want to limit to only a
> > single page? So that PFN_DOWN(start + size) == PFN_DOWN/(start), as
> > that would simplify the logic here?
>
> I have considered that, but I haven't found anything in the spec
> mandating the XHCI DbC registers to not cross page boundary. Currently
> (on a system I test this on) they don't cross page boundary, but I don't
> want to assume extra constrains - to avoid issues like before (when
> on the older system I tested the DbC registers didn't shared page with
> other registers, but then they shared the page on a newer hardware).
Oh, from our conversation at XenSummit I got the impression debug registers
where always at the same position. Looking at patch 2/2, it seems you
only need to block access to a single register. Are registers in XHCI
size aligned? As this would guarantee it doesn't cross a page
boundary (as long as the register is <= 4096 in size).
> > > + if ( !addr )
> > > + {
> > > + gprintk(XENLOG_ERR,
> > > + "Failed to map page for MMIO write at
> > > 0x%"PRI_mfn"%03x\n",
> > > + mfn_x(mfn), offset);
> > > + return;
> > > + }
> > > +
> > > + switch ( len )
> > > + {
> > > + case 1:
> > > + writeb(*(const uint8_t*)data, addr);
> > > + break;
> > > + case 2:
> > > + writew(*(const uint16_t*)data, addr);
> > > + break;
> > > + case 4:
> > > + writel(*(const uint32_t*)data, addr);
> > > + break;
> > > + case 8:
> > > + writeq(*(const uint64_t*)data, addr);
> > > + break;
> > > + default:
> > > + /* mmio_ro_emulated_write() already validated the size */
> > > + ASSERT_UNREACHABLE();
> > > + goto write_ignored;
> > > + }
> > > + return;
> > > + }
> > > + }
> > > + /* Do not print message for pages without any writable parts. */
> > > +}
> > > +
> > > +#ifdef CONFIG_HVM
> > > +bool subpage_mmio_write_accept(mfn_t mfn, unsigned long gla)
> > > +{
> > > + unsigned int offset = PAGE_OFFSET(gla);
> > > + const struct subpage_ro_range *entry;
> > > +
> > > + list_for_each_entry(entry, &subpage_ro_ranges, list)
> > > + if ( mfn_eq(entry->mfn, mfn) &&
> > > + !test_bit(offset / MMIO_RO_SUBPAGE_GRAN, entry->ro_elems) )
> > > + {
> > > + /*
> > > + * We don't know the write size at this point yet, so it
> > > could be
> > > + * an unaligned write, but accept it here anyway and deal
> > > with it
> > > + * later.
> > > + */
> > > + return true;
> >
> > For accesses that fall into the RO region, I think you need to accept
> > them here and just terminate them? I see no point in propagating
> > them further in hvm_hap_nested_page_fault().
>
> If write hits an R/O region on a page with some writable regions the
> handling should be the same as it would be just on the mmio_ro_ranges.
> This is what the patch does.
> There may be an opportunity to simplify mmio_ro_ranges handling
> somewhere, but I don't think it belongs to this patch.
Maybe worth adding a comment that the logic here intends to deal only
with the RW bits of a page that's otherwise RO, and that by not
handling the RO regions the intention is that those are dealt just
like fully RO pages.
I guess there's some message printed when attempting to write to a RO
page that you would also like to print here?
Thanks, Roger.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |