Re: [Qemu-devel] [PATCH qemu v18 5/5] spapr_pci/spapr_pci_vfio: Support Dynamic DMA Windows (DDW)

[Alexey Kardashevskiy]

On 22/06/16 19:44, Thomas Huth wrote:
> On 22.06.2016 05:23, Alexey Kardashevskiy wrote:
>> On 22/06/16 12:35, David Gibson wrote:
>>> On Tue, Jun 21, 2016 at 11:14:05AM +1000, Alexey Kardashevskiy wrote:
>>>> This adds support for Dynamic DMA Windows (DDW) option defined by
>>>> the SPAPR specification which allows to have additional DMA window(s)
>>>>
>>>> The "ddw" property is enabled by default on a PHB but for compatibility
>>>> the pseries-2.6 machine and older disable it.
>>>> This also creates a single DMA window for the older machines to
>>>> maintain backward migration.
>>>>
>>>> This implements DDW for PHB with emulated and VFIO devices. The host
>>>> kernel support is required. The advertised IOMMU page sizes are 4K and
>>>> 64K; 16M pages are supported but not advertised by default, in order to
>>>> enable them, the user has to specify "pgsz" property for PHB and
>>>> enable huge pages for RAM.
>>>>
>>>> The existing linux guests try creating one additional huge DMA window
>>>> with 64K or 16MB pages and map the entire guest RAM to. If succeeded,
>>>> the guest switches to dma_direct_ops and never calls TCE hypercalls
>>>> (H_PUT_TCE,...) again. This enables VFIO devices to use the entire RAM
>>>> and not waste time on map/unmap later. This adds a "dma64_win_addr"
>>>> property which is a bus address for the 64bit window and by default
>>>> set to 0x800.0000.0000.0000 as this is what the modern POWER8 hardware
>>>> uses and this allows having emulated and VFIO devices on the same bus.
>>>>
>>>> This adds 4 RTAS handlers:
>>>> * ibm,query-pe-dma-window
>>>> * ibm,create-pe-dma-window
>>>> * ibm,remove-pe-dma-window
>>>> * ibm,reset-pe-dma-window
>>>> These are registered from type_init() callback.
>>>>
>>>> These RTAS handlers are implemented in a separate file to avoid polluting
>>>> spapr_iommu.c with PCI.
>>>>
>>>> This changes sPAPRPHBState::dma_liobn to an array to allow 2 LIOBNs.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> [...]
>>>> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
>>>> index 9f28fb3..0cb51dd 100644
>>>> --- a/hw/ppc/spapr_pci.c
>>>> +++ b/hw/ppc/spapr_pci.c
> [...]
>>>> @@ -1515,7 +1531,8 @@ static void spapr_phb_reset(DeviceState *qdev)
>>>>  static Property spapr_phb_properties[] = {
>>>>      DEFINE_PROP_UINT32("index", sPAPRPHBState, index, -1),
>>>>      DEFINE_PROP_UINT64("buid", sPAPRPHBState, buid, -1),
>>>> -    DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn, -1),
>>>> +    DEFINE_PROP_UINT32("liobn", sPAPRPHBState, dma_liobn[0], -1),
>>>> +    DEFINE_PROP_UINT32("liobn64", sPAPRPHBState, dma_liobn[1], -1),
>>>>      DEFINE_PROP_UINT64("mem_win_addr", sPAPRPHBState, mem_win_addr, -1),
>>>>      DEFINE_PROP_UINT64("mem_win_size", sPAPRPHBState, mem_win_size,
>>>>                         SPAPR_PCI_MMIO_WIN_SIZE),
>>>> @@ -1527,6 +1544,11 @@ static Property spapr_phb_properties[] = {
>>>>      /* Default DMA window is 0..1GB */
>>>>      DEFINE_PROP_UINT64("dma_win_addr", sPAPRPHBState, dma_win_addr, 0),
>>>>      DEFINE_PROP_UINT64("dma_win_size", sPAPRPHBState, dma_win_size, 
>>>> 0x40000000),
>>>> +    DEFINE_PROP_UINT64("dma64_win_addr", sPAPRPHBState, dma64_win_addr,
>>>> +                       0x800000000000000ULL),
>>>> +    DEFINE_PROP_BOOL("ddw", sPAPRPHBState, ddw_enabled, true),
>>>> +    DEFINE_PROP_UINT64("pgsz", sPAPRPHBState, page_size_mask,
>>>> +                       (1ULL << 12) | (1ULL << 16)),
>>>>      DEFINE_PROP_END_OF_LIST(),
>>>>  };
>>>>  
>>>> @@ -1603,7 +1625,7 @@ static const VMStateDescription vmstate_spapr_pci = {
>>>>      .post_load = spapr_pci_post_load,
>>>>      .fields = (VMStateField[]) {
>>>>          VMSTATE_UINT64_EQUAL(buid, sPAPRPHBState),
>>>> -        VMSTATE_UINT32_EQUAL(dma_liobn, sPAPRPHBState),
>>>> +        VMSTATE_UNUSED(4), /* dma_liobn */
>>>
>>> It's not obvious to me why this change is necessary.
>>
>> It is not. But I was touching liobn and this is a proper cleanup which
>> needs to be done anyway as _EQUAL() macros are sort of deprecated and
>> rather pointless.
> 
> Not sure, but if you mark this field as unused now, is migration
> backwards to an older version of QEMU still working? If not, you might
> need to bump the version number, too?

Oh. Correct, it will fail. So I still need this field here. Ok, will fix
when resend.



-- 
Alexey

signature.asc
Description: OpenPGP digital signature