Re: [PATCH v5 2/3] x86/mm: add API for marking only part of a MMIO page read only

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

On Mon, Jul 22, 2024 at 02:09:15PM +0200, Jan Beulich wrote:
> On 19.07.2024 04:33, Marek Marczykowski-Górecki wrote:
> > @@ -4910,6 +4921,254 @@ long arch_memory_op(unsigned long cmd, 
> > XEN_GUEST_HANDLE_PARAM(void) arg)
> >      return rc;
> >  }
> >  
> > +static void __iomem *subpage_mmio_find_page(mfn_t mfn)
> > +{
> > +    struct subpage_ro_range *entry;
> 
> With the function returning void*, my first reaction was to ask why this
> isn't pointer-to-const. Yet then ...
> 
> > +    list_for_each_entry(entry, &subpage_ro_ranges, list)
> > +        if ( mfn_eq(entry->mfn, mfn) )
> > +            return entry;
> 
> ... you're actually returning entry here, just with its type zapped for
> no apparent reason. I also question the __iomem in the return type.

Right, a leftover from some earlier version.

> > +static int __init subpage_mmio_ro_add_page(
> > +    mfn_t mfn,
> > +    unsigned int offset_s,
> > +    unsigned int offset_e)
> > +{
> > +    struct subpage_ro_range *entry = NULL, *iter;
> > +    unsigned int i;
> > +
> > +    entry = subpage_mmio_find_page(mfn);
> > +    if ( !entry )
> > +    {
> > +        /* iter == NULL marks it was a newly allocated entry */
> > +        iter = NULL;
> 
> Yet you don't use "iter" for other purposes anymore. I think the variable
> wants renaming and shrinking to e.g. a simple bool.

+1

> > +        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);
> 
> Nit: Indentation looks to be off by 1 here.
> 
> > +        ASSERT(!oldbit);
> > +    }
> > +
> > +    if ( !iter )
> > +        list_add(&entry->list, &subpage_ro_ranges);
> 
> What's wrong with doing this right in the earlier conditional?
> 
> > +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) )
> > +        return -EINVAL;
> 
> I think I had asked before: Why is misaligned size something that wants a
> release build fallback to the assertion, but not misaligned start?

Misaligned start will lead to protecting larger area, not smaller, so it
is not unsafe thing to do. But I can also make it return an error, it
shouldn't happen after all.

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

Attachment: signature.asc
Description: PGP signature