Re: [PATCH v2] x86/PVH: modify permission checking in hwdom_fixup_p2m()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.07.2025 13:04, Roger Pau Monné wrote:
> On Tue, Jul 15, 2025 at 12:47:15PM +0200, Jan Beulich wrote:
>> On 15.07.2025 12:09, Roger Pau Monné wrote:
>>> On Mon, Jul 14, 2025 at 06:09:27PM +0200, Jan Beulich wrote:
>>>> We're generally striving to minimize behavioral differences between PV
>>>> and PVH Dom0. Using is_memory_hole() in the PVH case looks quite a bit
>>>> weaker to me, compared to the page ownership check done in the PV case.
>>>> Change checking accordingly.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>
>>> Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>
>> Thanks.
>>
>>> Kind of unrelated to this specific patch, but what's our opinion on
>>> turning on pf-fixup by default before the release?
>>
>> As far as the patch here goes, the relationship is very tight. I came to
>> make this patch only while investigating whether we couldn't have Dom0
>> report the resource (MMIO) ranges early enough for us to not even need
>> such fixing-up. Sadly, as per [1] that turned out pretty much impossible.
>> Which means that while I'm still pretty hesitant of us doing something
>> like this by default, I can't currently see a way around doing so. Hence
>> perhaps yes, we may want (or even need) to turn this on by default.
> 
> Sorry, wanted to reply to your previous commit alternative approach
> email, but got distracted with something else and forgot about it.
> 
> While I won't be opposed to having a way for dom0 to notify extra MMIO
> regions it wants added to the p2m, I think this is likely too much
> fuzz.  For example for FreeBSD I wouldn't consider adding such logic
> to the kernel, simply because I think it's likely to be too intrusive,
> and would rather rely on pf-fixup.  Overall the amount of p2m fixups
> that Xen ends up doing is always fairly small (I usually see maybe 4
> pages tops), and only as result of ACPI related accesses.  IMO it's an
> acceptable compromise to map those as individual 4K pages.

Yes, and my concern isn't so much what we map, or how many pages there
are, but that we do this behind the back of Dom0 (and also not ahead
of actually launching it).

As to the amount of accesses, these are the ranges that my SKL reports
through the temporary hypercall (as described on the v1 thread):

(XEN) sysmem: fed1c000 (24000 bytes)
(XEN) sysmem: fed45000 (47000 bytes)
(XEN) sysmem: ff000000 (1000000 bytes)
(XEN) sysmem: fed1b000 (1000 bytes)
(XEN) sysmem: fd000000 (ac0000 bytes)
(XEN) sysmem: fdad0000 (10000 bytes)
(XEN) sysmem: fe000000 (10000 bytes)
(XEN) sysmem: fe011000 (f000 bytes)
(XEN) sysmem: fe036000 (6000 bytes)
(XEN) sysmem: fe03d000 (3c3000 bytes)
(XEN) sysmem: fe410000 (3f0000 bytes)
(XEN) sysmem: fdaf0000 (10000 bytes)
(XEN) sysmem: fdae0000 (10000 bytes)
(XEN) sysmem: fdac0000 (10000 bytes)

Some of these ranges are also E820_RESERVED, so would (by default) be
mapped anyway. That's most notably the ff000000 one. The other regions
exceeding 2Mb in size aren't visible in E820, though.

As they're all reported by ACPI, they all could in principle be accessed.
Just requires the right drivers to be loaded, I expect.

> I would only consider the alternative approach of using a hypercall if
> we saw big regions being mapped by pf-fixup, because in that case it
> would better be using p2m superpage(s).
> 
> I think we want to enable pf-fixup by default at some point, the
> question is whether you would consider it appropriate to do now.
> Given it's limited to PVH dom0 only, I think we should enable for this
> release already.

As said, since I see no alternative, we can as well do it for 4.21. No
matter that I'm hesitant about it.

Jan