Re: [PATCH v7 16/22] dma-direct: make dma_direct_map_phys() honor DMA_ATTR_CC_SHARED

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

Jason Gunthorpe <jgg@ziepe.ca> writes:

> On Fri, Jul 10, 2026 at 10:52:49AM +0530, Aneesh Kumar K.V wrote:
>> >> > 	/*
>> >> > 	 * For host memory encryption and device requiring unencrypted DMA,
>> >> > 	 * MMIO memory is treated as shared by default.
>> >> > 	 */
>> >> > 	if (attrs & DMA_ATTR_MMIO) {
>> >> > 		if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) || force_dma_unencrypted(dev))
>> >> > 			attrs |= DMA_ATTR_CC_SHARED;
>> >> > 	}
>> >> 
>> >> Yes, I think it does the trick, preserves the current semantics for AMD.
>> >> I guess you could use a single 'if' for all checks (up to you).
>> >
>> > Please don't change it, MMIO P2P is broken on CC systems today and it
>> > should stay broken. Passing DMA_ATTR_MMIO with DMA_ATTR_CC_SHARED is
>> > an error that we need to correct in the drivers not make work in the
>> > core code.
>> >
>> 
>> But the above changes are intended to handle HOST_MEM_ENCRYPT. In v7, we
>> had the following diff:
>
> To follow how the rest of the decrypted/encrypted stuff works the MMIO
> has to be flaged with CC_SHARED for HOST_MEM_ENCRYPT too, just like
> the PTEs.
>
>> @@ -88,37 +88,40 @@ static inline dma_addr_t dma_direct_map_phys(struct device *dev,
>>  {
>>  	dma_addr_t dma_addr;
>>  
>> +	/*
>> +	 * For a device requiring unencrypted DMA, MMIO memory is treated
>> +	 * as shared by default.
>> +	 */
>> +	if (force_dma_unencrypted(dev) && (attrs & DMA_ATTR_MMIO))
>> +		attrs |= DMA_ATTR_CC_SHARED;
>
> force_dma_unencrypted() says nothing about the properties of the
> address passed in, this was nonsense :\
>
>> As we discussed [1], that can come in a later patch. In the meantime, adding
>> the HOST_MEM_ENCRYPT check preserves the previous behavior for SME.
>
> It never worked. When we added ATTR_MMIO it started to have a chance
> to work but prior to that it was always broken anyhow. I don't see
> there is much merit in preserving the narrow window when we
> inadvertantly had a half working ATTR_MMIO.
>
> But if you really want to it should be
> cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) *only* and get rid of the wrong
> force_dma_unencrypted().
>

Ok, I will update the changes to so we preserve the previous behaviour
for SME?

	if (attrs & DMA_ATTR_MMIO) {
		/*
		 * For host memory encryption treat MMIO memory as shared
		 */
		if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
			attrs |= DMA_ATTR_CC_SHARED;
	}

Do let me know if you feel strongly that the above change should not be
included and that the SME P2P case should instead be handled in a future
patch.


>
> But IMHO, I'd rather this series treat ATTR_MMIO as private MMIO and
> ATTR_MMIO|CC_SHARED as shared MMIO and that's the right and correct
> thing for the DMA API.
>

If you think the rest of the series is ready for upstream, could you
please ack it so it can be picked up for the next merge window? I'll
repost v8, rebased on top of the pKVM topic branch.

-aneesh