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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.06.2024 12:40, Roger Pau Monné wrote:
> On Wed, May 22, 2024 at 05:39:03PM +0200, Marek Marczykowski-Górecki wrote:
>> +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?
> 
> Mostly asking because I think for the usage of XHCI the registers that
> need to be marked RO are all inside the same page, and hence would
> like to avoid introducing logic to handle multipage ranges if that's
> not tested at all.
> 
>> +    }
>> +    else
>> +    {
>> +        subpage_start = PAGE_OFFSET(start);
>> +        subpage_end = PAGE_OFFSET(end) != PAGE_SIZE - 1;
>> +    }
>> +
>> +    spin_lock(&subpage_ro_lock);
> 
> Do you really need the lock if modifications can only happen during
> init?  Xen initialization is single threaded, so you can likely avoid
> the lock during boot.

I was wondering the same, but then concluded the locking here is for
the sake of completenese, not because it's strictly needed.

Jan