Re: [PATCH qemu] spapr/ddw: Reset DMA when the last non-default window is removed

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 6/21/22 23:54, Daniel Henrique Barboza wrote:
> 
> 
> On 6/20/22 05:01, Alexey Kardashevskiy wrote:
>> PAPR+/LoPAPR says:
>> ===
>> The platform must restore the default DMA window for the PE on a call
>> to the ibm,remove-pe-dma-window RTAS call when all of the following
>> are true:
>>   a. The call removes the last DMA window remaining for the PE.
>>   b. The DMA window being removed is not the default window
>>
>> ===
>>
>> This resets DMA as PAPR mandates.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
> 
> Looks good. One comment below:
> 
> 
>>   include/hw/ppc/spapr.h  |  3 ++-
>>   hw/ppc/spapr_iommu.c    |  8 +++++---
>>   hw/ppc/spapr_pci.c      |  2 +-
>>   hw/ppc/spapr_rtas_ddw.c | 17 ++++++++++++++++-
>>   hw/ppc/spapr_vio.c      |  3 ++-
>>   5 files changed, 26 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>> index 072dda2c7265..0adbe1566d40 100644
>> --- a/include/hw/ppc/spapr.h
>> +++ b/include/hw/ppc/spapr.h
>> @@ -902,6 +902,7 @@ struct SpaprTceTable {
>>       bool bypass;
>>       bool need_vfio;
>>       bool skipping_replay;
>> +    bool def_win;
>>       int fd;
>>       MemoryRegion root;
>>       IOMMUMemoryRegion iommu;
>> @@ -928,7 +929,7 @@ void spapr_check_mmu_mode(bool guest_radix);
>>   SpaprTceTable *spapr_tce_new_table(DeviceState *owner, uint32_t liobn);
>>   void spapr_tce_table_enable(SpaprTceTable *tcet,
>>                               uint32_t page_shift, uint64_t bus_offset,
>> -                            uint32_t nb_table);
>> +                            uint32_t nb_table, bool def_win);
> 
> Do we need to add 'def_win' to spapr_tce_table_enable()? I see that 
> you're using
> the 'def_win' var to assign
> 
> +    tcet->def_win = def_win;
> 
> but the only case where def_win will be other than 'false' is:
> 
> - the function is called by spapr_tce_post_load(), but in that case it's 
> being
> callied like this:
> 
>      spapr_tce_table_enable(tcet, old_page_shift, old_bus_offset,
>                             tcet->mig_nb_table, tcet->def_win);
> 
> which results in the function doing tcet->def_win = tcet->def_win, which is
> uneeded.
> 
> 
> - the function is called by spapr_phb_dma_reset(). In this case I believe
> we can just do "tcet->def_win = true" before calling 
> spapr_tce_table_enable():
> 
> 
>>       /* Register default 32bit DMA window */
>>       tcet = spapr_tce_find_by_liobn(sphb->dma_liobn[0]);
>> +     tcet->def_win = true;
>>       spapr_tce_table_enable(tcet, SPAPR_TCE_PAGE_SHIFT, 
>> sphb->dma_win_addr,
>>                              sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT);
>>   }
> 
> All other calls to spapr_tce_table_enable() are passing 'false' to 
> def_win and
> can be left alone.
> 
> Assuming that there's no way a created DMA window becomes the default 
> window, or
> the default DMA window is demoted to non-default, I think we can leave
> spapr_tce_table_enable() untouched and just set tcet->def_win to true 
> inside
> spapr_phb_dma_reset().


I can definitely do this. Thanks for the review, I'll repost.

> 
> 
> Thanks,
> 
> 
> Daniel
> 
> 
> 
>>   void spapr_tce_table_disable(SpaprTceTable *tcet);
>>   void spapr_tce_set_need_vfio(SpaprTceTable *tcet, bool need_vfio);
>> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
>> index 81e5a1aea3a6..f8c1627d0782 100644
>> --- a/hw/ppc/spapr_iommu.c
>> +++ b/hw/ppc/spapr_iommu.c
>> @@ -242,7 +242,7 @@ static int spapr_tce_table_post_load(void *opaque, 
>> int version_id)
>>       if (tcet->mig_nb_table) {
>>           if (!tcet->nb_table) {
>>               spapr_tce_table_enable(tcet, old_page_shift, 
>> old_bus_offset,
>> -                                   tcet->mig_nb_table);
>> +                                   tcet->mig_nb_table, tcet->def_win);
>>           }
>>           memcpy(tcet->table, tcet->mig_table,
>> @@ -279,7 +279,7 @@ static const VMStateDescription 
>> vmstate_spapr_tce_table_ex = {
>>   static const VMStateDescription vmstate_spapr_tce_table = {
>>       .name = "spapr_iommu",
>> -    .version_id = 2,
>> +    .version_id = 3,
>>       .minimum_version_id = 2,
>>       .pre_save = spapr_tce_table_pre_save,
>>       .post_load = spapr_tce_table_post_load,
>> @@ -292,6 +292,7 @@ static const VMStateDescription 
>> vmstate_spapr_tce_table = {
>>           VMSTATE_BOOL(bypass, SpaprTceTable),
>>           VMSTATE_VARRAY_UINT32_ALLOC(mig_table, SpaprTceTable, 
>> mig_nb_table, 0,
>>                                       vmstate_info_uint64, uint64_t),
>> +        VMSTATE_BOOL_V(def_win, SpaprTceTable, 3),
>>           VMSTATE_END_OF_LIST()
>>       },
>> @@ -380,7 +381,7 @@ SpaprTceTable *spapr_tce_new_table(DeviceState 
>> *owner, uint32_t liobn)
>>   void spapr_tce_table_enable(SpaprTceTable *tcet,
>>                               uint32_t page_shift, uint64_t bus_offset,
>> -                            uint32_t nb_table)
>> +                            uint32_t nb_table, bool def_win)
>>   {
>>       if (tcet->nb_table) {
>>           warn_report("trying to enable already enabled TCE table");
>> @@ -390,6 +391,7 @@ void spapr_tce_table_enable(SpaprTceTable *tcet,
>>       tcet->bus_offset = bus_offset;
>>       tcet->page_shift = page_shift;
>>       tcet->nb_table = nb_table;
>> +    tcet->def_win = def_win;
>>       tcet->table = spapr_tce_alloc_table(tcet->liobn,
>>                                           tcet->page_shift,
>>                                           tcet->bus_offset,
>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>> index b2f5fbef0c83..e1dbccfc7547 100644
>> --- a/hw/ppc/spapr_pci.c
>> +++ b/hw/ppc/spapr_pci.c
>> @@ -2066,7 +2066,7 @@ void spapr_phb_dma_reset(SpaprPhbState *sphb)
>>       /* Register default 32bit DMA window */
>>       tcet = spapr_tce_find_by_liobn(sphb->dma_liobn[0]);
>>       spapr_tce_table_enable(tcet, SPAPR_TCE_PAGE_SHIFT, 
>> sphb->dma_win_addr,
>> -                           sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT);
>> +                           sphb->dma_win_size >> 
>> SPAPR_TCE_PAGE_SHIFT, true);
>>   }
>>   static void spapr_phb_reset(DeviceState *qdev)
>> diff --git a/hw/ppc/spapr_rtas_ddw.c b/hw/ppc/spapr_rtas_ddw.c
>> index 13d339c807c1..4fe41b0c4539 100644
>> --- a/hw/ppc/spapr_rtas_ddw.c
>> +++ b/hw/ppc/spapr_rtas_ddw.c
>> @@ -182,7 +182,7 @@ static void 
>> rtas_ibm_create_pe_dma_window(PowerPCCPU *cpu,
>>        */
>>       tcet->skipping_replay = true;
>>       spapr_tce_table_enable(tcet, page_shift, win_addr,
>> -                           1ULL << (window_shift - page_shift));
>> +                           1ULL << (window_shift - page_shift), false);
>>       tcet->skipping_replay = false;
>>       if (!tcet->nb_table) {
>>           goto hw_error_exit;
>> @@ -215,6 +215,7 @@ static void 
>> rtas_ibm_remove_pe_dma_window(PowerPCCPU *cpu,
>>       SpaprPhbState *sphb;
>>       SpaprTceTable *tcet;
>>       uint32_t liobn;
>> +    bool def_win_removed;
>>       if ((nargs != 1) || (nret != 1)) {
>>           goto param_error_exit;
>> @@ -231,9 +232,23 @@ static void 
>> rtas_ibm_remove_pe_dma_window(PowerPCCPU *cpu,
>>           goto param_error_exit;
>>       }
>> +    def_win_removed = tcet->def_win;
>>       spapr_tce_table_disable(tcet);
>>       trace_spapr_iommu_ddw_remove(liobn);
>> +    /*
>> +     * PAPR+/LoPAPR says:
>> +     * The platform must restore the default DMA window for the PE on 
>> a call
>> +     * to the ibm,remove-pe-dma-window RTAS call when all of the 
>> following
>> +     * are true:
>> +     * a. The call removes the last DMA window remaining for the PE.
>> +     * b. The DMA window being removed is not the default window
>> +     */
>> +    if (spapr_phb_get_active_win_num(sphb) == 0 && !def_win_removed) {
>> +        spapr_phb_dma_reset(sphb);
>> +        trace_spapr_iommu_ddw_reset(sphb->buid, 0);
>> +    }
>> +
>>       rtas_st(rets, 0, RTAS_OUT_SUCCESS);
>>       return;
>> diff --git a/hw/ppc/spapr_vio.c b/hw/ppc/spapr_vio.c
>> index 9d4fec2c04d8..14506df19d62 100644
>> --- a/hw/ppc/spapr_vio.c
>> +++ b/hw/ppc/spapr_vio.c
>> @@ -533,7 +533,8 @@ static void spapr_vio_busdev_realize(DeviceState 
>> *qdev, Error **errp)
>>           dev->tcet = spapr_tce_new_table(qdev, liobn);
>>           spapr_tce_table_enable(dev->tcet, SPAPR_TCE_PAGE_SHIFT, 0,
>> -                               pc->rtce_window_size >> 
>> SPAPR_TCE_PAGE_SHIFT);
>> +                               pc->rtce_window_size >> 
>> SPAPR_TCE_PAGE_SHIFT,
>> +                               false);
>>           dev->tcet->vdev = dev;
>>           memory_region_add_subregion_overlap(&dev->mrroot, 0,
>>                                               
>> spapr_tce_get_iommu(dev->tcet), 2);

-- 
Alexey