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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.03.2023 15:27, Marek Marczykowski-Górecki wrote:
> On Wed, Mar 29, 2023 at 02:39:22PM +0200, Jan Beulich wrote:
>> On 29.03.2023 12:51, Marek Marczykowski-Górecki wrote:
>>> On Wed, Mar 29, 2023 at 10:50:20AM +0200, Jan Beulich wrote:
>>>> Taking both remarks together, limiting granularity to 16(?) bytes
>>>> would allow using the advanced EPT functionality down the road, and
>>>> would at the same time limit the suggested bitmap to just 256 bits /
>>>> 32 bytes, which I think gets us below what even an empty rangeset
>>>> would require. Plus lookup would also be quite a bit more lightweight.
>>>
>>> Indeed, in that case it makes sense.
>>
>> Hmm, I've checked the SDM, and I was misremembering: Granularity is
>> 128 bytes, which might be too large for the purposes here.
> 
> Indeed, it seems so. In case of USB3 console, I want to protect 64 bytes
> of registers...
> 
> I guess 16 bytes granularity would work, but it feels kinda arbitrary
> without any good reason to choose this specific number. More logical
> would be 4 bytes (as common register size), but it means 128 bytes for
> the bitmask.

Which is still acceptable, so I'd say 4 bytes it is then.

>>>>> +        }
>>>>> +    }
>>>>> +    gdprintk(XENLOG_WARNING,
>>>>> +             "ignoring write to R/O MMIO mfn %" PRI_mfn " offset %lx len 
>>>>> %u\n",
>>>>> +             mfn_x(mfn), offset, len);
>>>>
>>>> ... make it here. (By implication: I'm not convinced this wants to be a
>>>> gdprintk(), as I think at least one such warning would better surface in
>>>> release builds as well.)
>>>
>>> Right, gprintk() would make more sense indeed.
>>>
>>>> At the same time I don't think any message should be issued for write
>>>> attempts to pages which don't have part of it marked writable. We deal
>>>> with such silently right now, and this shouldn't change.
>>>
>>> At least for HVM domains, it isn't really silent. It's domain_crash()
>>> (before my change, not reaching this function at all).
>>
>> Well, yes, but that's one instance in the lifetime of a domain.
> 
> What I mean is that it isn't a common or normal case that HVM domain
> would exercise in normal operation. It's abnormal situation and as such
> it should IMO get a log message.
> And even for PV domain, such message would save me quite some time
> debugging related issues...

Sure. I don't mind a single message. I also don't mind a few. But I'd
prefer if we could avoid relying on (general) rate limiting to bound
the volume, at least when it's the simple case of dropping writes
which don't overlap the intended to be writable region.

That said - if for HVM previously we crashed the domain in such cases
and this now changes, then this change in behavior would also need
mentioning / justifying.

Jan