Re: [Qemu-devel] [PATCH 00/11] pseries: migration and QOM support

[Anthony Liguori <aliguori@us.ibm.com>]

Alexey Kardashevskiy <aik@ozlabs.ru> writes:

> On 07/16/2013 10:48 PM, Alexey Kardashevskiy wrote:
>> On 07/16/2013 10:35 PM, Alexey Kardashevskiy wrote:
>>> On 07/16/2013 10:33 PM, Anthony Liguori wrote:
>>>> Alexey Kardashevskiy <aik@ozlabs.ru> writes:
>>>>
>>>>> On 07/16/2013 01:11 AM, Anthony Liguori wrote:
>>>>>> This series is based on Alexey's series:
>>>>>>
>>>>>>   spapr: migration, pci, msi, power8
>>>>>>
>>>>>> Which in turn was based on work by David Gibson.
>>>>>>
>>>>>> I've removed the bits not related to migration and made the
>>>>>> following changes:
>>>>>>
>>>>>>  1) QOMify TCE tables and XICS
>>>>>>
>>>>>>  2) Do everything in terms of VMStateDescriptions
>>>>>>
>>>>>>  3) Fix endianness problem with TCE table translation
>>>>>>     a) Drop the VMSTATE_DIVIDE thing in the process
>>>>>>
>>>>>> I've tested this with a TCG pseries guest on an x86_64 host.
>>>>>
>>>>>
>>>>> It did not compile (fixed, patch is posted) and it fails to migrate with
>>>>> enabled KVM.
>>>>
>>>> With in-kernel XICS?  That's not in this series..
>>>
>>> No, as is, without any of my patches. I suspect rather HPTE than XICS though.
>> 
>> I was wrong. vmstate_spapr_tce_table is broken :)
>
> Here it is:
>
> diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c
> index 709cc34..3d4a1fc 100644
> --- a/hw/ppc/spapr_iommu.c
> +++ b/hw/ppc/spapr_iommu.c
> @@ -148,6 +148,7 @@ static int spapr_tce_table_realize(DeviceState *dev)
>              * sizeof(uint64_t);
>          tcet->table = g_malloc0(table_size);
>      }
> +    tcet->nb_table = tcet->window_size >> SPAPR_TCE_PAGE_SHIFT;

Ah, right, pre_load isn't enough :-/  Sorry about that.  Thanks for
digging in to this.

>
>  #ifdef DEBUG_TCE
>      fprintf(stderr, "spapr_iommu: New TCE table @ %p, liobn=0x%x, "
>
>
> Honestly, I liked David's approach more when we did not need any extra
> parameter to sync  :(

I resisted the urge to refactor but I'm confident that nb_table will end
up with nicer code.

The 'tcet->window_size >> SPAPR_TCE_PAGE_SHIFT' is used all over the
place.  It's not very clear without carefully thinking about it that the
table size == the number of pages in the window.

Regards,

Anthony Liguori

>
>
>
> -- 
> Alexey