| Message ID | 20250219164840.94803-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/dom0: be less restrictive with the Interrupt Address Range | expand |
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@citrix.com> > --- > 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. > --- 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. Personally I'd prefer if we continued using the subtraction, but either way: Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
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@citrix.com> > > --- > > 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. > > --- 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). I can switch back to PAGE_SHIFT, didn't think it was a big deal since it's a one time only calculation. > Personally I'd prefer if we continued using the subtraction, > but either way: > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, will switch back to PAGE_SHIFT if it doesn't turn out to be too ugly.
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@citrix.com> >>> --- >>> 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
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c index 3b9681dc9134..aec596997d5d 100644 --- 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); rc |= irqs_permit_access(d, 1, nr_irqs_gsi - 1); /* Modify I/O port access permissions. */
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@citrix.com> --- 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. --- Changes since v2: - New in this version. --- xen/arch/x86/dom0_build.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)