linux-kernel - Re: [PATCH v2 2/2] xen/arm: introduce GNTTABOP_cache

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <542EBB2A.6030104@citrix.com>
Date:	Fri, 3 Oct 2014 16:05:14 +0100
From:	David Vrabel <david.vrabel@...rix.com>
To:	Stefano Stabellini <stefano.stabellini@...citrix.com>,
	<xen-devel@...ts.xensource.com>
CC:	<konrad.wilk@...cle.com>, <Ian.Campbell@...rix.com>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 2/2] xen/arm: introduce GNTTABOP_cache_flush

On 03/10/14 15:53, Stefano Stabellini wrote:
> Introduce support for new hypercall GNTTABOP_cache_flush.
> Use it to perform cache flashing on pages used for dma when necessary.
[..]
>  /* functions called by SWIOTLB */
> @@ -22,16 +25,31 @@ static void dma_cache_maint(dma_addr_t handle, unsigned long offset,
>  		size_t len = left;
>  		void *vaddr;
>  	
> +		if (len + offset > PAGE_SIZE)
> +			len = PAGE_SIZE - offset;
> +
>  		if (!pfn_valid(pfn))
>  		{
> -			/* TODO: cache flush */
> +			struct gnttab_cache_flush cflush;
> +
> +			cflush.op = 0;
> +			cflush.a.dev_bus_addr = pfn << PAGE_SHIFT;
> +			cflush.offset = offset;
> +			cflush.size = len;
> +
> +			if (op == dmac_unmap_area && dir != DMA_TO_DEVICE)
> +				cflush.op = GNTTAB_CACHE_INVAL;
> +			if (op == dmac_map_area) {
> +				cflush.op = GNTTAB_CACHE_CLEAN;
> +				if (dir == DMA_FROM_DEVICE)
> +					cflush.op |= GNTTAB_CACHE_INVAL;
> +			}

Are all these cache operations needed?  You do a clean on map regardless
of the direction and INVAL on map seems unnecessary.

I would have thought it would be:

map && (TO_DEVICE || BOTH)
   op = CLEAN

unmap && (FROM_DEVICE || BOTH)
   op = INVAL

> --- a/include/xen/interface/grant_table.h
> +++ b/include/xen/interface/grant_table.h
> @@ -479,6 +479,24 @@ struct gnttab_get_version {
>  DEFINE_GUEST_HANDLE_STRUCT(gnttab_get_version);
>  
>  /*
> + * Issue one or more cache maintenance operations on a portion of a
> + * page granted to the calling domain by a foreign domain.
> + */
> +#define GNTTABOP_cache_flush          12
> +struct gnttab_cache_flush {
> +    union {
> +        uint64_t dev_bus_addr;
> +        grant_ref_t ref;
> +    } a;
> +    uint32_t offset; /* offset from start of grant */
> +    uint32_t size;   /* size within the grant */
> +#define GNTTAB_CACHE_CLEAN      (1<<0)
> +#define GNTTAB_CACHE_INVAL      (1<<1)
> +    uint32_t op;
> +};
> +DEFINE_GUEST_HANDLE_STRUCT(gnttab_cache_flush);
> +
> +/*
>   * Bitfield values for update_pin_status.flags.
>   */
>   /* Map the grant entry for access by I/O devices. */

If the ARM bits are correct, have a

Reviewed-by: David Vrabel <david.vrabel@...rix.com>

for the common header.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/