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

[Zhenzhong Duan <zhenzhong.duan@xxxxxxxxxx>]

On 2013/5/8 20:03, Jan Beulich wrote:
> > > > On 08.05.13 at 12:00, Zhenzhong Duan <zhenzhong.duan@xxxxxxxxxx> wrote:
> > On 2013-05-08 17:39, Jan Beulich wrote:
> > > > > > On 08.05.13 at 10:17, Zhenzhong Duan<zhenzhong.duan@xxxxxxxxxx>  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@xxxxxxxxxx>
> > > > Tested-by: Yuval Shaia<yuval.shaia@xxxxxxxxxx>
> > > 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


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel