Re: [Xen-devel] [PATCH v3 12/13] xen/arm: Add the property "protected-devices" in the hypervisor node

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2014-03-18 at 20:09 +0000, Julien Grall wrote:
> Hi Ian,
> 
> On 03/18/2014 04:48 PM, Ian Campbell wrote:
> > On Tue, 2014-03-11 at 15:49 +0000, Julien Grall wrote:
> >> DOM0 is using the swiotlb to bounce DMA. With the IOMMU support in Xen,
> >> protected devices should not use it.
> >>
> >> Only Xen is abled to know if an IOMMU protects the device. The new property
> >> "protected-devices" is a list of device phandles protected by an IOMMU.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> >>
> >> ---
> >>     This patch *MUST NOT* be applied until we agreed on a device binding
> >>     the device tree folks. DOM0 can run safely with swiotlb on protected
> >>     devices while LVM is not used for guest disk.
> > 
> > LVM works these days I think.
> 
> With this patch series applied, LVM will be broken if the hard drive is
> protected by an IOMMU.

How/why?

> It's the case on midway, the platform will crash just after the guest
> begins to boot.

This configuration works today (osstest tests it) so this would be a
regression. Can you sort this please?

> >> @@ -384,6 +387,39 @@ static int make_hypervisor_node(struct domain *d,
> >>      if ( res )
> >>          return res;
> >>  
> >> +    if ( kinfo->num_dev_protected )
> >> +    {
> >> +        /* Don't need to take dtdevs_lock here */
> > 
> > Why not? Please explain in the comment.
> 
> Because, building dom0 is only done with 1 CPU online (e.g CPU0). I
> though it was obvious, I will update the comment.

Locking is never obvious IMHO.

> >> +        cells = xmalloc_array(__be32, kinfo->num_dev_protected *
> >> +                              dt_size_to_cells(sizeof(dt_phandle)));
> >> +        if ( !cells )
> >> +            return -FDT_ERR_XEN(ENOMEM);
> >> +
> >> +        _cells = cells;
> > 
> > Odd numbers of leading _ are reserved for the compiler IIRC. Even
> > numbers are reserved for the libc/environment which is why we can get
> > away with such names in hypervisor context.
> > 
> > But lets just skirt the whole issue and pick a name which doesn't use a
> > leading _. cells_iter or c or something.
> > 
> > Is there no interface to say "make an fdt_property(name, size)"
> > returning the data to be filled in?
> 
> No, every helpers request to have an input data in parameters.

fdt_setprop_inplace isn't helpful because you need to be able to create
the empty prop, which doesn't exist. Oh well.


> I will rename _cells into cells_iter.

Thanks.

Ian.


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