Re: [PATCH v3 1/3] x86/dom0: correctly set the maximum ->iomem_caps bound for PVH

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.02.2025 09:49, Roger Pau Monné wrote:
> On Thu, Feb 20, 2025 at 09:22:40AM +0100, Jan Beulich wrote:
>> On 19.02.2025 17:48, Roger Pau Monne wrote:
>>> The logic in dom0_setup_permissions() sets the maximum bound in
>>> ->iomem_caps unconditionally using paddr_bits, which is not correct for HVM
>>> based domains.  Instead use domain_max_paddr_bits() to get the correct
>>> maximum paddr bits for each possible domain type.
>>>
>>> Switch to using PFN_DOWN() instead of PAGE_SHIFT, as that's shorter.
>>>
>>> Fixes: 53de839fb409 ('x86: constrain MFN range Dom0 may access')
>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>>> ---
>>> The fixes tag might be dubious, IIRC at that time we had PVHv1 dom0, which
>>> would likely also need such adjustment, but not the current PVHv2.
>>
>> Probably better to omit it then. It would be one of the changes moving to
>> PVHv2 that missed making the adjustment.
> 
> Well, PVHv1 would have needed such adjustment, as it was also limited
> to hap_paddr_bits instead of paddr_bits.

Looks like I mis-interpreted your sentence then.

>>> --- a/xen/arch/x86/dom0_build.c
>>> +++ b/xen/arch/x86/dom0_build.c
>>> @@ -481,7 +481,8 @@ int __init dom0_setup_permissions(struct domain *d)
>>>  
>>>      /* The hardware domain is initially permitted full I/O capabilities. */
>>>      rc = ioports_permit_access(d, 0, 0xFFFF);
>>> -    rc |= iomem_permit_access(d, 0UL, (1UL << (paddr_bits - PAGE_SHIFT)) - 
>>> 1);
>>> +    rc |= iomem_permit_access(d, 0UL,
>>> +                              PFN_DOWN(1UL << domain_max_paddr_bits(d)) - 
>>> 1);
>>
>> Why PFN_DOWN() rather than subtracting PAGE_SHIFT? That's two shifts rather
>> than just one.
> 
> cosmetic: line length (it's mentioned in the commit message).

Oh, I had overlooked that sentence there.

>  I can
> switch back to PAGE_SHIFT, didn't think it was a big deal since it's
> a one time only calculation.

Feel free to keep as is then. I agree it's not a big deal here; my worry with 
such
usually is that people seeing something in one place may then copy/clone the 
same
to use elsewhere.

Jan