[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/2] memory: XENMEM_add_to_physmap (almost) wrapping checks


On 14/10/2021 15:10, Jan Beulich wrote:
On 14.10.2021 13:29, Julien Grall wrote:
On 13/09/2021 07:42, Jan Beulich wrote:
Determining that behavior is correct (i.e. results in failure) for a
passed in GFN equaling INVALID_GFN is non-trivial. Make this quite a
bit more obvious by checking input in generic code - both for singular
requests to not match the value and for range ones to not pass / wrap
through it.

For Arm similarly make more obvious that no wrapping of MFNs passed
for XENMAPSPACE_dev_mmio and thus to map_dev_mmio_region() can occur:
Drop the "nr" parameter of the function to avoid future callers
appearing which might not themselves check for wrapping. Otherwise
the respective ASSERT() in rangeset_contains_range() could trigger.

Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
I find it odd that map_dev_mmio_region() returns success upon
iomem_access_permitted() indicating failure - is this really intended?

AFAIR yes. The hypercall is not used as "Map the region" but instead
"Make sure the region is mapped if the IOMEM region is accessible".

It is necessary to return 0 because dom0 OS cannot distinguished between
emulated and non-emulated. So we may report error when there is none.

Odd, but I clearly don't understand all the aspects here.

As per commit 102984bb1987 introducing it this also was added for ACPI
only - any reason XENMAPSPACE_dev_mmio isn't restricted to CONFIG_ACPI

There is nothing specific to ACPI in the implementation. So I don't
really see the reason to restrict to CONFIG_ACPI.

However, it is still possible to boot using DT when Xen is built with
CONFIG_ACPI. So if the restriction was desirable, then I think it should
be using !acpi_disabled.

My point was rather about this potentially being dead code in non-ACPI
builds (i.e. in particular uniformly on 32-bit).

The hypercall is already wired and a dom0 OS can use it today even on non-ACPI. Whether a dom0 OS will use it is a different question. I know that Linux will limit it to ACPI. It is likely not used by other OS, but I can't guarantee it.

In this case, the hypercall is only a few lines and already restricted to dom0 only (see xapt_permission_check()). So to me, the #ifdef here is not worth it.

@@ -841,6 +844,15 @@ int xenmem_add_to_physmap(struct domain
       if ( xatp->size < start )
           return -EILSEQ;
+ if ( xatp->gpfn + xatp->size < xatp->gpfn ||
+         xatp->idx + xatp->size < xatp->idx )
+    {
+#define _gfn(x) (x)

AFAICT, _gfn() will already be defined. So some compiler may complain
because will be defined differently on debug build.

No - _gfn() is an inline function as per typesafe.h. (Or else it
wouldn't be just "some" compiler, but gcc at least would have
complained to me.)

Ah. somehow I thought it was a macro. But looking at the implementation, it makes sense to be an inline funciton.

Sorry for the noise.



... I might be missing something... but why can't use gfn_x(INVALID_GFN)
+ 1 here?

Because gfn_x() also is an inline function, and that's not suitable
for a compile-time constant expression.

Right. How about introduce INVALID_GFN_RAW in mm-frame.h? This could also be used to replace the open-code value in INVALID_GFN and INVALID_GFN_INITIALIZER?

In fact, I am not entirely sure what's the purpose of this
BUILD_BUG_ON(). Could you give more details?

The expression in the surrounding if() relies on INVALID_GFN being the
largest representable value, i.e. this ensures that INVALID_GFN doesn't
sit anywhere in [xatp->gpfn, xatp->gpfn + xatp->size).

Thanks the explanation. Can you add the rationale in a comment on top of BUILD_BUG_ON()?


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.