Re: [Xen-devel] [PATCH v3 0/11] Rename/remove IS_PRIV

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 04/18/2013 11:12 AM, Jan Beulich wrote:
> > > > On 12.04.13 at 23:04, Daniel De Graaf <dgdegra@xxxxxxxxxxxxx> wrote:
> > Changes since v2:
> >   - Handle XEN_SYSCTL_CPUPOOL_OP_MOVEDOMAIN separately
> >   - Use is_control_domain for CPUID
> >   - Use is_{control,hardware}_domain for TSC
> >   - MAINTAINERS patch included in series
> >
> > ---
> >
> > Following the conversion of most IS_PRIV hooks to XSM, the remaining
> > references to this function generally deal with direct hardware access
> > and not with the type of privilege checks that are best controlled by
> > XSM. To reflect this, the IS_PRIV check is renamed to is_hardware_domain
> > and is used only when dealing with accesses that are both required by
> > dom0 and where it does not make sense to grant access to a domain other
> > than dom0.
> >
> > There are a number of existing places in the hypervisor that check
> > domain_id for equality to zero to make some distinction on dom0; this
> > series replaces these checks with is_hardware_domain to be consistent in
> > how the hypervisor checks a domain's access.
> >
> > Independent changes related to this series:
> >         [PATCH 01/11] MAINTAINERS: Add myself as XSM maintainer
> >         [PATCH 08/11] xen/cpupool: prevent a domain from moving itself
> >
> > Cleanup of IS_PRIV checks that should not be is_hardware_domain:
> >         [PATCH 02/11] xen/arch/x86: remove IS_PRIV access check bypasses
> >         [PATCH 03/11] xen/xsm: add hooks for claim
> >         [PATCH 04/11] hvm: convert access check for nested HVM to XSM
> >         [PATCH 05/11] xen/arch/x86: remove IS_PRIV_FOR references
> >         [PATCH 06/11] xen/arch/arm: remove rcu_lock_target_domain_by_id
> >
> > Replace remaining calls to IS_PRIV:
> >         [PATCH 07/11] xen: rename IS_PRIV to is_hardware_domain
> >
> > Use is_hardware_domain locations where (domid == 0) was used:
> >         [PATCH 09/11] xen: use domid check in is_hardware_domain
> >         [PATCH 10/11] xen/arch/x86: clarify domid == 0 checks
> >         [PATCH 11/11] IOMMU: use is_hardware_domain instead of domid == 0
> While patch 1 went in a few days ago, patch 2 was held up just by
> XSA-46, which went public today. Consequently I also committed
> patch 2 a few minutes ago.
After looking at the XSA-46 patch, I think a the IS_PRIV removal can still
be usefully applied to the XEN_DOMCTL_{bind,unbind}_pt_irq functions - I'll
send the patch in a minute for comment.

> For the rest of the series, however, I would want you two to work
> out the release related aspects, and I'd look into committing parts
> that I'm permitted to commit once I saw George's ack.
George:

Here are my thoughts on the viability of these patches for a freeze
exception; I tried to address them in order from most to least important.

Patch 8 fixes a bug with cpupools, although it is unlikely this bug will
be hit on a normal setup since it requires the cpupool operation to be
invoked from a domain other than dom0. It is also independent of the other
patches in this series, and I see no reason not to include it in 4.3.

Patches 3-5 just change the behavior with XSM enabled, and makes these
access vectors more consistent with the rest of the hypervisor in the use
of XSM hooks when doing access checks related to domain permissions. I
think there is good reason to include these in 4.3, assuming there are no
technical objections.

Patch 6 only changes ARM code, and should only change behavior with XSM
enabled. Since XSM on ARM is not really tested yet, this is less important
for 4.3 - although it allows removing some of the functions that were replaced
by XSM hooks, which may help avoid reintroducing users of these functions.

Patches 7 and 9-11 introduce is_hardware_domain as a wrapper on (domid == 0).
Since the hypervisor does not currently support setting is_privileged on
non-dom0 domains, the test is equivalent to IS_PRIV - just a cosmetic change.
The changes to .c files in patches 9-11 should produce identical code after
preprocessing and so have almost zero chance of introducing a bug.

> Patch 10 may, according to the reply I just sent, need another small
> tweak before getting applied - as long as you agree, I could certainly
> do this while committing.
>
> Jan
Assuming that the is_hardware_domain changes go in, the tweak you mentioned
looks good to me.

--
Daniel De Graaf
National Security Agency

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel