From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCH v4 11/15] x86/altp2m: define and implement alternate p2m HVMOP types. Date: Mon, 13 Jul 2015 08:25:41 +0100 Message-ID: <55A38415020000780008FF20@mail.emea.novell.com> References: <1436489553-6300-1-git-send-email-edmund.h.white@intel.com> <1436489553-6300-12-git-send-email-edmund.h.white@intel.com> <559FB3FD020000780008F5E3@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ravi Sahita Cc: Tim Deegan , Wei Liu , George Dunlap , Andrew Cooper , Ian Jackson , Edmund H White , "xen-devel@lists.xen.org" , "tlengyel@novetta.com" , Daniel De Graaf List-Id: xen-devel@lists.xenproject.org >>> On 11.07.15 at 00:03, wrote: >> From: Jan Beulich [mailto:JBeulich@suse.com] >>Sent: Friday, July 10, 2015 3:01 AM >>>>> On 10.07.15 at 02:52, wrote: >>> + default: >>> + return -ENOSYS; >>> + >>> + break; >> >>Bogus (unreachable) break. > > Wanted to keep this so that if someone removes the error code then they > don't cause an invalid fall through. > But ok with removing it if you think so. We don't (intentionally) do this anywhere else, so it should be removed. >>> + if ( !(d ? d : current->domain)->arch.altp2m_active ) >> >>This is bogus: d is NULL if and only if altp2m_vcpu_enable_notify, i.e. I > don't >>see why you can't just use current->domain inside that case (and you really >>do). That would then also eliminate the need for this redundant and >>obfuscating switch() nesting you use. >> > > We need to check if the target domain is in altp2m mode for all the > following sub-ops. > If we removed this check, we would need to check for target domain being in > altp2m for all the following cases. > Andrew wanted to refactor and pull common code up, and that's what this is > one case of for hvm_op. I'd be fine with such refactoring if it didn't result in nested switch()-es using the same control expression. >>> + >>> +struct xen_hvm_altp2m_set_mem_access { >>> + /* view */ >>> + uint16_t view; >>> + /* Memory type */ >>> + uint16_t hvmmem_access; /* xenmem_access_t */ >>> + uint8_t pad[4]; >>> + /* gfn */ >>> + uint64_t gfn; >>> +}; >>> +typedef struct xen_hvm_altp2m_set_mem_access >>> xen_hvm_altp2m_set_mem_access_t; >>> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t); >>> + >>> +struct xen_hvm_altp2m_change_gfn { >>> + /* view */ >>> + uint16_t view; >>> + uint8_t pad[6]; >>> + /* old gfn */ >>> + uint64_t old_gfn; >>> + /* new gfn, INVALID_GFN (~0UL) means revert */ >>> + uint64_t new_gfn; >>> +}; >>> +typedef struct xen_hvm_altp2m_change_gfn >>xen_hvm_altp2m_change_gfn_t; >>> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_change_gfn_t); >>> + >>> +struct xen_hvm_altp2m_op { >>> + uint32_t cmd; >>> +/* Get/set the altp2m state for a domain */ >>> +#define HVMOP_altp2m_get_domain_state 1 >>> +#define HVMOP_altp2m_set_domain_state 2 >>> +/* Set the current VCPU to receive altp2m event notifications */ >>> +#define HVMOP_altp2m_vcpu_enable_notify 3 >>> +/* Create a new view */ >>> +#define HVMOP_altp2m_create_p2m 4 >>> +/* Destroy a view */ >>> +#define HVMOP_altp2m_destroy_p2m 5 >>> +/* Switch view for an entire domain */ >>> +#define HVMOP_altp2m_switch_p2m 6 >>> +/* Notify that a page of memory is to have specific access types */ >>> +#define HVMOP_altp2m_set_mem_access 7 >>> +/* Change a p2m entry to have a different gfn->mfn mapping */ >>> +#define HVMOP_altp2m_change_gfn 8 >>> + domid_t domain; >>> + uint8_t pad[2]; >> >>While you added padding fields as asked for, you still don't verify them to > be >>zero on input. > > Specifically what were you thinking we need to do here - also would be good > if you can explain what was the underlying concern? (thanks) I'm pretty sure I said so before - future extensibility. I.e. a means to make use of the now unused (padding) fields, which can only be done if the fields are being checked to be zero while unused. Jan