Message-ID: <e54bd244-06eb-6217-8511-4867df085ff5@linux.ibm.com>
Date:   Wed, 8 Dec 2021 13:04:20 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH 23/32] KVM: s390: pci: handle refresh of PCI translations
Content-Language: en-US
To:     Niklas Schnelle <schnelle@linux.ibm.com>,
        linux-s390@vger.kernel.org
Cc:     alex.williamson@redhat.com, cohuck@redhat.com,
        farman@linux.ibm.com, pmorel@linux.ibm.com,
        borntraeger@linux.ibm.com, hca@linux.ibm.com, gor@linux.ibm.com,
        gerald.schaefer@linux.ibm.com, agordeev@linux.ibm.com,
        frankja@linux.ibm.com, david@redhat.com, imbrenda@linux.ibm.com,
        vneethv@linux.ibm.com, oberpar@linux.ibm.com, freude@linux.ibm.com,
        thuth@redhat.com, pasic@linux.ibm.com, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20211207205743.150299-1-mjrosato@linux.ibm.com>
 <20211207205743.150299-24-mjrosato@linux.ibm.com>
 <fc3c836ccb697a7e7123ab70015bd2a40b7cb5d4.camel@linux.ibm.com>
From:   Matthew Rosato <mjrosato@linux.ibm.com>
In-Reply-To: <fc3c836ccb697a7e7123ab70015bd2a40b7cb5d4.camel@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/8/21 5:30 AM, Niklas Schnelle wrote:
> On Tue, 2021-12-07 at 15:57 -0500, Matthew Rosato wrote:
>> Add a routine that will perform a shadow operation between a guest
>> and host IOAT.  A subsequent patch will invoke this in response to
>> an 04 RPCIT instruction intercept.
>>
>> Signed-off-by: Matthew Rosato <mjrosato@linux.ibm.com>
>> ---
>>   arch/s390/include/asm/kvm_pci.h |   1 +
>>   arch/s390/include/asm/pci_dma.h |   1 +
>>   arch/s390/kvm/pci.c             | 191 ++++++++++++++++++++++++++++++++
>>   arch/s390/kvm/pci.h             |   4 +-
>>   4 files changed, 196 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/s390/include/asm/kvm_pci.h b/arch/s390/include/asm/kvm_pci.h
>> index 254275399f21..97e3a369135d 100644
>> --- a/arch/s390/include/asm/kvm_pci.h
>> +++ b/arch/s390/include/asm/kvm_pci.h
>> @@ -30,6 +30,7 @@ struct kvm_zdev_ioat {
>>   struct kvm_zdev {
>>   	struct zpci_dev *zdev;
>>   	struct kvm *kvm;
>> +	u64 rpcit_count;
>>   	struct kvm_zdev_ioat ioat;
>>   	struct zpci_fib fib;
>>   };
>> diff --git a/arch/s390/include/asm/pci_dma.h b/arch/s390/include/asm/pci_dma.h
>> index e1d3c1d3fc8a..0ca15e5db3d9 100644
>> --- a/arch/s390/include/asm/pci_dma.h
>> +++ b/arch/s390/include/asm/pci_dma.h
>> @@ -52,6 +52,7 @@ enum zpci_ioat_dtype {
>>   #define ZPCI_TABLE_ENTRIES		(ZPCI_TABLE_SIZE / ZPCI_TABLE_ENTRY_SIZE)
>>   #define ZPCI_TABLE_PAGES		(ZPCI_TABLE_SIZE >> PAGE_SHIFT)
>>   #define ZPCI_TABLE_ENTRIES_PAGES	(ZPCI_TABLE_ENTRIES * ZPCI_TABLE_PAGES)
>> +#define ZPCI_TABLE_ENTRIES_PER_PAGE	(ZPCI_TABLE_ENTRIES / ZPCI_TABLE_PAGES)
>>   
>>   #define ZPCI_TABLE_BITS			11
>>   #define ZPCI_PT_BITS			8
>> diff --git a/arch/s390/kvm/pci.c b/arch/s390/kvm/pci.c
>> index a1c0c0881332..858c5ecdc8b9 100644
>> --- a/arch/s390/kvm/pci.c
>> +++ b/arch/s390/kvm/pci.c
>> @@ -123,6 +123,195 @@ int kvm_s390_pci_aen_init(u8 nisc)
>>   	return rc;
>>   }
>>   
>> +static int dma_shadow_cpu_trans(struct kvm_vcpu *vcpu, unsigned long *entry,
>> +				unsigned long *gentry)
>> +{
>> +	unsigned long idx;
>> +	struct page *page;
>> +	void *gaddr = NULL;
>> +	kvm_pfn_t pfn;
>> +	gpa_t addr;
>> +	int rc = 0;
>> +
>> +	if (pt_entry_isvalid(*gentry)) {
>> +		/* pin and validate */
>> +		addr = *gentry & ZPCI_PTE_ADDR_MASK;
>> +		idx = srcu_read_lock(&vcpu->kvm->srcu);
>> +		page = gfn_to_page(vcpu->kvm, gpa_to_gfn(addr));
>> +		srcu_read_unlock(&vcpu->kvm->srcu, idx);
>> +		if (is_error_page(page))
>> +			return -EIO;
>> +		gaddr = page_to_virt(page) + (addr & ~PAGE_MASK);
> 
> Hmm, this looks like a virtual vs physical address mixup to me that is
> currently not a problem because kernel virtual addresses are equal to
> their physical address. Here page_to_virt(page) gives us a virtual
> address but the entries in the I/O translation table have to be
> physical (aka absolute) addresses.
> 
> With my commit "s390/pci: use physical addresses in DMA tables"
> currently in the s390 feature branch this is also reflected in the
> argument types taken by set_pt_pfaa() below so gaddr should have type
> phys_addr_t not void *. That should also remove the need for the cast
> to unsigned long for the duplicate check.

Right...  Like the other comment re: virtual vs physical address I will 
take a look and fix for v2.