From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zhenzhong Duan Subject: Re: [PATCH 1/2] xen, libxc: init msix addr/data with value from qemu via hypercall Date: Thu, 09 May 2013 11:02:37 +0800 Message-ID: <518B11CD.1050308@oracle.com> References: <518A0A15.60301@oracle.com> <518A397F02000078000D44AE@nat28.tlf.novell.com> <518A2237.9060709@oracle.com> <518A5B4A02000078000D4668@nat28.tlf.novell.com> Reply-To: zhenzhong.duan@oracle.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <518A5B4A02000078000D4668@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Chien Yen , Konrad Rzeszutek Wilk , Feng Jin , Yuval Shaia , xen-devel List-Id: xen-devel@lists.xenproject.org On 2013/5/8 20:03, Jan Beulich wrote: >>>> On 08.05.13 at 12:00, Zhenzhong Duan wrote: >> On 2013-05-08 17:39, Jan Beulich wrote: >>>>>> On 08.05.13 at 10:17, Zhenzhong Duan 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 >>>> Tested-by: Yuval Shaia >>> 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