RE: [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

So sorry for taking so long to respond. Just being back from the long National 
Day holidays. 😉

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Thursday, September 23, 2021 6:36 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>
> Subject: Re: [PATCH 03/11] xen/arm: introduce 1:1 direct-map for domUs
> 
> 
> 
> On 23/09/2021 08:11, Penny Zheng wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> >
> > Cases where domU needs 1:1 direct-map memory map:
> 
> "1:1" and "direct-map" means pretty much the same. Given that the property
> is name "direct-map", then I would drop "1:1".
> 
> >    * IOMMU not present in the system.
> >    * IOMMU disabled if it doesn't cover a specific device and all the
> > guests are trusted. Thinking a mixed scenario, where a few devices
> > with IOMMU and a few without, then guest DMA security still could not be
> totally guaranteed.
> > So users may want to disable the IOMMU, to at least gain some
> > performance improvement from IOMMU disabled.
> >    * IOMMU disabled as a workaround when it doesn't have enough
> bandwidth.
> > To be specific, in a few extreme situation, when multiple devices do
> > DMA concurrently, these requests may exceed IOMMU's transmission
> capacity.
> >    * IOMMU disabled when it adds too much latency on DMA. For example,
> > TLB may be missing in some IOMMU hardware, which may bring latency in
> > DMA progress, so users may want to disable it in some realtime scenario.
> >
> > *WARNING:
> > Users should be aware that it is not always secure to assign a device
> > without IOMMU protection.
> > When the device is not protected by the IOMMU, the administrator
> > should make sure that:
> >   1. The device is assigned to a trusted guest.
> >   2. Users have additional security mechanism on the platform.
> 
> The two requirements are only necessary for device that are DMA-capable.
> For device that can't do DMA, it will likely be fine to assign to non-trusted
> guest.
> 
> >
> > Given that with direct-map, the IOMMU could be used but it is not required.
> 
> I can't parse it.
> 

Now when doing device assignments, IOMMU is forced to be enabled. And since
we are introducing direct-map here, I think that maybe even if IOMMU is 
missing/disabled,
direct-map on trust guests could also make it work.

Maybe I should rephrase it to
"
When doing device assignments and IOMMU is missing or disabled, direct-map
shall be used on trust guests.
"

> > This commit avoids setting XEN_DOMCTL_CDF_iommu when the IOMMU is
> > disabled and direct_map is requested.
> >
> > Since, for now, 1:1 direct-map is only supported when domain on Static
> 
> I think "Since" seems unnecessary.
> 
> > Allocation, that is, "xen.static-mem" is defined in the domain 
> > configuration.
> >
> > This commit also re-implements allocate_static_memory to allocate
> > static memory as guest RAM for 1:1 direct-map domain.
> >
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>
> > Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> > ---
> >   docs/misc/arm/device-tree/booting.txt |   9 ++
> >   xen/arch/arm/domain_build.c           | 117 +++++++++++++++++---------
> >   2 files changed, 85 insertions(+), 41 deletions(-)
> >
> > diff --git a/docs/misc/arm/device-tree/booting.txt
> > b/docs/misc/arm/device-tree/booting.txt
> > index 44cd9e1a9a..3d637c747e 100644
> > --- a/docs/misc/arm/device-tree/booting.txt
> > +++ b/docs/misc/arm/device-tree/booting.txt
> > @@ -164,6 +164,15 @@ with the following properties:
> >       Both #address-cells and #size-cells need to be specified because
> >       both sub-nodes (described shortly) have reg properties.
> >
> > +- direct-map
> > +
> > +    Optional for Domain on Static Allocation.
> > +    An empty property to request the memory of the domain to be 1:1
> > +    direct-map(guest physical address == physical address).
> > +    WARNING: Users must be aware of this risk, that guests having access
> > +    to hardware with DMA capacity must be trusted, or it could use the
> > +    DMA engine to access any other memory area.
> 
> The WARNING is only applicable if the device is not protected by an IOMMU.
> So this should be clarified because one may want still want to use the direct-
> map (e.g. because the OS relies on the host layout) and have IOMMU enabled.
> 
> > +
> >   Under the "xen,domain" compatible node, one or more sub-nodes are
> present
> >   for the DomU kernel and ramdisk.
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 21d8a559af..213ad017dc 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -488,8 +488,14 @@ static bool __init
> append_static_memory_to_bank(struct domain *d,
> >   {
> >       int res;
> >       unsigned int nr_pages = PFN_DOWN(size);
> > -    /* Infer next GFN. */
> > -    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> > +    gfn_t sgfn;
> > +
> > +    if ( !is_domain_direct_mapped(d) )
> > +        /* Infer next GFN when GFN != MFN. */
> > +        sgfn = gaddr_to_gfn(bank->start + bank->size);
> > +    else
> > +        sgfn = gaddr_to_gfn(mfn_to_maddr(smfn));
> > +
> >
> >       res = guest_physmap_add_pages(d, sgfn, smfn, nr_pages);
> >       if ( res )
> > @@ -537,14 +543,17 @@ static void __init allocate_static_memory(struct
> domain *d,
> >       }
> 
> This function was already pretty difficult to read. So I would rather not add
> more complexity in it. Instead, I would look to split the common code in a
> separate helper or possibly duplicate it.
> 
> >       reg_cells = addr_cells + size_cells;
> >
> > -    /*
> > -     * The static memory will be mapped in the guest at the usual guest
> memory
> > -     * addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE) defined by
> > -     * xen/include/public/arch-arm.h.
> > -     */
> > -    gbank = 0;
> > -    gsize = ramsize[gbank];
> > -    kinfo->mem.bank[gbank].start = rambase[gbank];
> > +    if ( !is_domain_direct_mapped(d) )
> > +    {
> > +        /*
> > +         * The static memory will be mapped in the guest at the usual guest
> > +         * memory addresses (GUEST_RAM0_BASE, GUEST_RAM1_BASE)
> defined by
> > +         * xen/include/public/arch-arm.h.
> > +         */
> > +        gbank = 0;
> > +        gsize = ramsize[gbank];
> > +        kinfo->mem.bank[gbank].start = rambase[gbank];
> > +    } >
> >       cell = (const __be32 *)prop->value;
> >       nr_banks = (prop->length) / (reg_cells * sizeof (u32)); @@
> > -572,42 +581,58 @@ static void __init allocate_static_memory(struct
> domain *d,
> >           printk(XENLOG_INFO "%pd: STATIC BANK[%u] %#"PRIpaddr"-
> %#"PRIpaddr"\n",
> >                  d, bank, pbase, pbase + psize);
> >
> > -        while ( 1 )
> > +        if ( !is_domain_direct_mapped(d) )
> >           {
> > -            /* Map as much as possible the static range to the guest bank 
> > */
> > -            if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> smfn,
> > -                                               min(psize, gsize)) )
> > -                goto fail;
> > -
> > -            /*
> > -             * The current physical bank is fully mapped.
> > -             * Handle the next physical bank.
> > -             */
> > -            if ( gsize >= psize )
> > +            while ( 1 )
> >               {
> > -                gsize = gsize - psize;
> > -                break;
> > +                /* Map as much as possible the static range to the guest 
> > bank */
> > +                if ( !append_static_memory_to_bank(d, 
> > &kinfo->mem.bank[gbank],
> > +                                                   smfn, min(psize, 
> > gsize)) )
> > +                    goto fail;
> > +
> > +                /*
> > +                 * The current physical bank is fully mapped.
> > +                 * Handle the next physical bank.
> > +                 */
> > +                if ( gsize >= psize )
> > +                {
> > +                    gsize = gsize - psize;
> > +                    break;
> > +                }
> > +                /*
> > +                 * When current guest bank is not enough to map, exhaust
> > +                 * the current one and seek to the next.
> > +                 * Before seeking to the next, check if we still have 
> > available
> > +                 * guest bank.
> > +                 */
> > +                else if ( (gbank + 1) >= GUEST_RAM_BANKS )
> > +                {
> > +                    printk(XENLOG_ERR "Exhausted all possible guest 
> > banks.\n");
> > +                    goto fail;
> > +                }
> > +                else
> > +                {
> > +                    psize = psize - gsize;
> > +                    smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> > +                    /* Update to the next guest bank. */
> > +                    gbank++;
> > +                    gsize = ramsize[gbank];
> > +                    kinfo->mem.bank[gbank].start = rambase[gbank];
> > +                }
> >               }
> > +        }
> > +        else /* 1:1 direct-map. */
> > +        {
> >               /*
> > -             * When current guest bank is not enough to map, exhaust
> > -             * the current one and seek to the next.
> > -             * Before seeking to the next, check if we still have available
> > -             * guest bank.
> > +             * One guest memory bank is matched with one physical
> > +             * memory bank.
> >                */
> > -            else if ( (gbank + 1) >= GUEST_RAM_BANKS )
> > -            {
> > -                printk(XENLOG_ERR "Exhausted all possible guest banks.\n");
> > +            gbank = bank;
> > +            kinfo->mem.bank[gbank].start = pbase;
> > +
> > +            if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> > +                                               smfn, psize) )
> >                   goto fail;
> > -            }
> > -            else
> > -            {
> > -                psize = psize - gsize;
> > -                smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> > -                /* Update to the next guest bank. */
> > -                gbank++;
> > -                gsize = ramsize[gbank];
> > -                kinfo->mem.bank[gbank].start = rambase[gbank];
> > -            }
> >           }
> >
> >           tot_size += psize;
> > @@ -2638,6 +2663,7 @@ void __init create_domUs(void)
> >   {
> >       struct dt_device_node *node;
> >       const struct dt_device_node *chosen =
> > dt_find_node_by_path("/chosen");
> > +    bool direct_map = false;
> 
> This is a bit redundant for just a couple of use. Instead, you could directly 
> use
> d_cfg.flags & XEN_DOMCTL_INTERNAL_directmap.
> 
> >
> >       BUG_ON(chosen == NULL);
> >       dt_for_each_child_node(chosen, node) @@ -2658,8 +2684,17 @@ void
> > __init create_domUs(void)
> >               panic("Missing property 'cpus' for domain %s\n",
> >                     dt_node_name(node));
> >
> > +        direct_map = dt_property_read_bool(node, "direct-map");
> > +        d_cfg.flags |= direct_map ? XEN_DOMCTL_INTERNAL_directmap :
> > + 0;
> >           if ( dt_find_compatible_node(node, NULL, "multiboot,device-tree") 
> > )
> > -            d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> > +        {
> > +            if ( iommu_enabled )
> > +                d_cfg.flags |= XEN_DOMCTL_CDF_iommu;
> > +            else if ( !direct_map )
> > +                panic("Assign a device but IOMMU and direct-map are all
> disabled\n");
> > +            else
> > +                warning_add("Please be sure of having trusted guests, when 
> > doing
> device assignment without IOMMU protection\n");
> > +        }
> >
> >           if ( !dt_property_read_u32(node, "nr_spis", &d_cfg.arch.nr_spis) )
> >           {
> >
> 
> Cheers,
> 
> --
> Julien Grall

Cheers,

--
Penny Zheng