Re: [PATCH 1/2] xen, libxc: init msix addr/data with value from qemu via hypercall

[Zhenzhong Duan <zhenzhong.duan@oracle.com>]

On 2013/5/8 20:03, Jan Beulich wrote:
>>>> On 08.05.13 at 12:00, Zhenzhong Duan <zhenzhong.duan@oracle.com> wrote:
>> On 2013-05-08 17:39, Jan Beulich wrote:
>>>>>> On 08.05.13 at 10:17, Zhenzhong Duan<zhenzhong.duan@oracle.com>  wrote:
>>>> Accelerated msix entry is initialized to zero when msixtbl_pt_register is
>>>> called. This doesn't match the value from qemu side, although pirq may
>>>> already
>>>> be mapped and binded in qemu side. Kernel will get wrong value when reading
>>>> msix info.
>>>>
>>>> Signed-off-by: Zhenzhong Duan<zhenzhong.duan@oracle.com>
>>>> Tested-by: Yuval Shaia<yuval.shaia@oracle.com>
>>> I appreciate this needing to change, but it is a no-go to expose an
>>> implementation detail of the hypervisor (number of accelerated
>>> entries being 3) trough a hypercall interface (and even less so by
>>> scattering around literal 3-s).
>> I presume you mean msi_ad[3]. msi_ad[3] is addr_lo, addr_high and data.
>> Not related to accelerated entries count.
>>
>> or others?
> Oh, right you are. But then nevertheless give this meaningful
> names in the hypercall interface (e.g. addr_lo, addr_hi, and data,
> or just [64-bit] addr and [32-bit] data) rather than following the
> bad practice in vmsi.c.
>
>>> Please work towards a different solution, leaving the tool stack
>>> agnostic to the number of accelerated entries. And if at all
>>> possible, arrange for the patch to be split into tool stack and
>>> hypervisor pieces, such that they can be applied independently
>>> (and in either order).
>> sure, will do it after above question is clear.
> With the above it's going to be difficult to split the two pieces.
so, only change to a meaningful names without split patch, right?
>
> But of course I still don't really understand why all of the sudden
> this needs to be passed in rather than being under the full control
> of the hypervisor at all times. Perhaps this is related to me not
> understanding why the kernel would read these values at all:
> There's no other place in the kernel where the message would
> be read before first getting written (in fact, apart from the
> use of __read_msi_msg() by the Xen code, there's only one
> other user under arch/powerpc/, and there - according to the
> accompanying comment - this is just to save away the data for
> later use during resume).
There is a bug if msi_ad is not passed in.

when driver first load,

kernel.__read_msi_msg()
        (got all zero)
kernel.__write_msi_msg(pirq)
        (ioreq passed to qemu as no msixtbl_entry established yet)
qemu.pt_msi_update_one()
        xc_domain_update_msi_irq()
             (msixtbl_entry dynamicly allocated with msi_ad all zero)

then driver unload,
...
driver load again,

kernel.__read_msi_msg()
        (got all zero from xen as accelerated entry just established with all zero)
qemu.__write_msi_msg(a new pirq)

pirq would exhaust or fail to map and bind.

zduan