Re: [Xen-devel] [PATCH 5/7] public / x86: introduce __HYPERCALL_iommu_op

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 16 March 2018 12:25
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; George Dunlap <George.Dunlap@xxxxxxxxxx>; Ian
> Jackson <Ian.Jackson@xxxxxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx; KonradRzeszutek
> Wilk <konrad.wilk@xxxxxxxxxx>; Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>;
> Tim (Xen.org) <tim@xxxxxxx>
> Subject: Re: [PATCH 5/7] public / x86: introduce __HYPERCALL_iommu_op
> 
> >>> On 12.02.18 at 11:47, <paul.durrant@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -33,6 +33,7 @@ obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o
> >  obj-y += hypercall.o
> >  obj-y += i387.o
> >  obj-y += i8259.o
> > +obj-y += iommu_op.o
> 
> As mentioned in other contexts, I'd prefer if we stopped using
> underscores in places where dashes (or other separators not
> usable in C identifiers) are fine.
> 

I don't see any guidance in CODING_STYLE or elsewhere, and also the majority of 
the codebase seems to prefer using underscores in module names. Personally I'd 
prefer new code remain consistent.

> > --- /dev/null
> > +++ b/xen/arch/x86/iommu_op.c
> > @@ -0,0 +1,169 @@
> >
> +/*********************************************************
> *********************
> > + * x86/iommu_op.c
> > + *
> > + * Paravirtualised IOMMU functionality
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; If not, see <http://www.gnu.org/licenses/>.
> > + *
> > + * Copyright (C) 2018 Citrix Systems Inc
> > + */
> > +
> > +#include <xen/event.h>
> > +#include <xen/guest_access.h>
> > +#include <xen/hypercall.h>
> > +
> > +static bool can_control_iommu(void)
> > +{
> > +    struct domain *currd = current->domain;
> > +
> > +    /*
> > +     * IOMMU mappings cannot be manipulated if:
> > +     * - the IOMMU is not enabled or,
> > +     * - the IOMMU is passed through or,
> 
> "is passed through" isn't really a proper description of what
> iommu_passthrough means, I'm afraid. The description of the
> option says "Control whether to disable DMA remapping for
> Dom0." Perhaps "is bypassed"? But then it would be better
> to qualify the check with is_hardware_domain(), despite you
> restricting things to Dom0 for now anyway.
> 

I think I'm going to add a hypercall for a domain to enable PV IOMMU at start 
of day, so I'll re-work all this in a separate patch.

> > +     * - shared EPT configured or,
> > +     * - Xen is maintaining an identity map.
> 
> Is this meant to describe ...
> 
> > +     */
> > +    if ( !iommu_enabled || iommu_passthrough ||
> > +         iommu_use_hap_pt(currd) || need_iommu(currd) )
> 
> ... need_iommu() here? How is that implying an identity map?
> 
> > +        return false;
> > +
> > +    return true;
> 
> Please make this a singe return statement (with the expression as
> operand).
> 
> > +long do_iommu_op(XEN_GUEST_HANDLE_PARAM(xen_iommu_op_t)
> uops,
> > +                 unsigned int count)
> > +{
> > +    unsigned int i;
> > +    int rc;
> > +
> > +    rc = xsm_iommu_op(XSM_PRIV, current->domain);
> > +    if ( rc )
> > +        return rc;
> > +
> > +    if ( !can_control_iommu() )
> > +        return -EACCES;
> > +
> > +    for ( i = 0; i < count; i++ )
> > +    {
> > +        xen_iommu_op_t op;
> > +
> > +        if ( ((i & 0xff) == 0xff) && hypercall_preempt_check() )
> > +        {
> > +            rc = i;
> 
> For this to be correct for large enough values of "count", rc needs
> to have long type.

Yes, it does indeed.

> 
> > +            break;
> > +        }
> > +
> > +        if ( copy_from_guest_offset(&op, uops, i, 1) )
> > +        {
> > +            rc = -EFAULT;
> > +            break;
> > +        }
> > +
> > +        iommu_op(&op);
> > +
> > +        if ( copy_to_guest_offset(uops, i, &op, 1) )
> 
> __copy_to_guest_offset()
> 
> Also do you really need to copy back other than the status?

At this stage, no. I'll restrict it here and it can expand later if need be.

> 
> > --- /dev/null
> > +++ b/xen/include/public/iommu_op.h
> > @@ -0,0 +1,55 @@
> > +/*
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> copy
> > + * of this software and associated documentation files (the "Software"),
> to
> > + * deal in the Software without restriction, including without limitation 
> > the
> > + * rights to use, copy, modify, merge, publish, distribute, sublicense,
> and/or
> > + * sell copies of the Software, and to permit persons to whom the
> Software is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included
> in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT SHALL THE
> > + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
> OR OTHER
> > + * DEALINGS IN THE SOFTWARE.
> > + *
> > + * Copyright (C) 2018 Citrix Systems Inc
> > + */
> > +
> > +#ifndef __XEN_PUBLIC_IOMMU_OP_H__
> > +#define __XEN_PUBLIC_IOMMU_OP_H__
> 
> Please can you avoid introducing further name space violations
> into the public headers?

I assume you mean the leading '__'? Again, I chose the name based on 
consistency with other code and I'd prefer to remain consistent. Could you 
explain why having a leading '__' is problematic?

  Paul

> 
> > +#include "xen.h"
> > +
> > +struct xen_iommu_op {
> > +    uint16_t op;
> > +    uint16_t flags; /* op specific flags */
> > +    int32_t status; /* op completion status: */
> > +                    /* 0 for success otherwise, negative errno */
> > +};
> 
> Peeking at patch 6, you need to add the union and a large enough
> placeholder here right away, so that the struct size won't change
> with future additions.
> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel