From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Roger_Pau_Monn=c3=a9?= Subject: Re: [PATCH v3 3/4] xen/hvm: introduce a fpu_uninitialised field to the CPU save record Date: Tue, 24 Nov 2015 15:38:14 +0100 Message-ID: <56547656.2090902@citrix.com> References: <1447864631-4174-1-git-send-email-roger.pau@citrix.com> <1447864631-4174-4-git-send-email-roger.pau@citrix.com> <564F4F3502000078000B747E@prv-mh.provo.novell.com> <565461CE.1070609@citrix.com> <5654758C02000078000B86A1@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a1Ejk-0000s9-6B for xen-devel@lists.xenproject.org; Tue, 24 Nov 2015 14:38:28 +0000 In-Reply-To: <5654758C02000078000B86A1@prv-mh.provo.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: Andrew Cooper , xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org El 24/11/15 a les 14.34, Jan Beulich ha escrit: >>>> On 24.11.15 at 14:10, wrote: >> El 20/11/15 a les 16.49, Jan Beulich ha escrit: >>>>>> On 18.11.15 at 17:37, wrote: >>>> @@ -2091,7 +2092,8 @@ static int hvm_load_cpu_ctxt(struct domain *d, hvm_domain_context_t *h) >>>> struct xsave_struct *xsave_area = v->arch.xsave_area; >>>> >>>> memcpy(v->arch.xsave_area, ctxt.fpu_regs, sizeof(ctxt.fpu_regs)); >>>> - xsave_area->xsave_hdr.xstate_bv = XSTATE_FP_SSE; >>>> + xsave_area->xsave_hdr.xstate_bv = ctxt.fpu_initialised ? >>>> + XSTATE_FP_SSE : 0; >>>> } >>>> else >>>> memcpy(v->arch.fpu_ctxt, ctxt.fpu_regs, sizeof(ctxt.fpu_regs)); >>> >>> Question is - are the memcpy()s here really meaningful/valid >>> when !ctxt.fpu_initialized? I.e. shouldn't this code rather be >>> skipped instead of getting modified? >> >> If !fpu_initialized the fpu context save record is all zeroed out. I >> don't think it matters much (apart from saving a few CPU cycles). I can >> send a new version that doesn't save/restore the fpu context at all if >> !fpu_initialised. > > I'd appreciate that (ideally with if(!fpu_initialised) memset(); else if ...). > >>>> @@ -157,6 +159,8 @@ struct hvm_hw_cpu { >>>> }; >>>> /* error code for pending event */ >>>> uint32_t error_code; >>>> + /* is fpu initialised? */ >>>> + uint32_t fpu_initialised; >>> >>> A whole uint32_t for just one bit? Didn't we talk about making this >>> new field a flags one, consuming just one bit from it? >> >> AFAIK we agreed on adding this field to the tail and making it a >> uint32_t so that when new fields are added they can be detected by >> looking at the size of the structure: >> >> http://marc.info/?l=xen-devel&m=144490321208291 > > Admittedly it's a little implicit, but that mail has, in its quoting parts, > > "... (and validate unused tail bits are zero, so they can be used for > something later on)" > > going back to that intention of using just a single bit here afaict. Ack. I have to admit I've misunderstood that part. Then I guess the field should also have a more generic name, like "flags", and fpu_initialised should be defined as (1U << 0). Or do you want me to use the MSB in order to store the fpu_initialised bit? Roger.