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
