Message-ID: <a963388d-b13e-07c5-c256-c91671b3aa73@linux.ibm.com>
Date:   Tue, 14 Dec 2021 12:54:55 -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:     Pierre Morel <pmorel@linux.ibm.com>, linux-s390@vger.kernel.org
Cc:     alex.williamson@redhat.com, cohuck@redhat.com,
        schnelle@linux.ibm.com, farman@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>
 <d2af697e-bf48-e78b-eed6-766f0790232f@linux.ibm.com>
From:   Matthew Rosato <mjrosato@linux.ibm.com>
In-Reply-To: <d2af697e-bf48-e78b-eed6-766f0790232f@linux.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 12/14/21 11:59 AM, Pierre Morel wrote:
> 
> 
> On 12/7/21 21:57, 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;
>>   }
> 
> ...snip...
> 
>> +
>> +int kvm_s390_pci_refresh_trans(struct kvm_vcpu *vcpu, unsigned long req,
>> +                   unsigned long start, unsigned long size)
>> +{
>> +    struct zpci_dev *zdev;
>> +    u32 fh;
>> +    int rc;
>> +
>> +    /* If the device has a SHM bit on, let userspace take care of 
>> this */
>> +    fh = req >> 32;
>> +    if ((fh & aift.mdd) != 0)
>> +        return -EOPNOTSUPP;
> 
> I think you should make this check in the caller.

OK

> 
>> +
>> +    /* Make sure this is a valid device associated with this guest */
>> +    zdev = get_zdev_by_fh(fh);
>> +    if (!zdev || !zdev->kzdev || zdev->kzdev->kvm != vcpu->kvm)
>> +        return -EINVAL;
>> +
>> +    /* Only proceed if the device is using the assist */
>> +    if (zdev->kzdev->ioat.head[0] == 0)
>> +        return -EOPNOTSUPP;
> 
> Using the assist means using interpretation over using interception and 
> legacy vfio-pci. right?

Right - more specifically that the IOAT assist feature was never set via 
the vfio feature ioctl, so we can't handle the RPCIT for this device and 
so throw to userspace.

The way the QEMU series is being implemented, a device using 
interpretation will always have the IOAT feature set on.

> 
>> +
>> +    rc = dma_table_shadow(vcpu, zdev, start, size);
>> +    if (rc > 0)
>> +        rc = zpci_refresh_trans((u64) zdev->fh << 32, start, size);
> 
> Here you lose the status reported by the hardware.
> You should directly use __rpcit(fn, addr, range, &status);

OK, I can have a look at doing this.

@Niklas thoughts on how you would want this exported.  Renamed to 
zpci_rpcit or so?

> 
> 
>> +    zdev->kzdev->rpcit_count++;
>> +
>> +    return rc;
>> +}
>> +
>>   /* Modify PCI: Register floating adapter interruption forwarding */
>>   static int kvm_zpci_set_airq(struct zpci_dev *zdev)
>>   {
>> @@ -590,4 +779,6 @@ void kvm_s390_pci_init(void)
>>   {
>>       spin_lock_init(&aift.gait_lock);
>>       mutex_init(&aift.lock);
>> +
>> +    WARN_ON(zpci_get_mdd(&aift.mdd));
>>   }
>> diff --git a/arch/s390/kvm/pci.h b/arch/s390/kvm/pci.h
>> index 3c86888fe1b3..d252a631b693 100644
>> --- a/arch/s390/kvm/pci.h
>> +++ b/arch/s390/kvm/pci.h
>> @@ -33,6 +33,7 @@ struct zpci_aift {
>>       struct kvm_zdev **kzdev;
>>       spinlock_t gait_lock; /* Protects the gait, used during AEN 
>> forward */
>>       struct mutex lock; /* Protects the other structures in aift */
>> +    u32 mdd;
>>   };
>>   static inline struct kvm *kvm_s390_pci_si_to_kvm(struct zpci_aift 
>> *aift,
>> @@ -47,7 +48,8 @@ struct zpci_aift *kvm_s390_pci_get_aift(void);
>>   int kvm_s390_pci_aen_init(u8 nisc);
>>   void kvm_s390_pci_aen_exit(void);
>> -
>> +int kvm_s390_pci_refresh_trans(struct kvm_vcpu *vcpu, unsigned long req,
>> +                   unsigned long start, unsigned long end);
>>   void kvm_s390_pci_init(void);
>>   #endif /* __KVM_S390_PCI_H */
>>
>