Re: [Xen-devel] [PATCH v2 11/23] x86: XENMEM_resource_ioreq_server is HVM only

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Mon, Aug 27, 2018 at 09:13:04AM -0600, Jan Beulich wrote:
> >>> On 26.08.18 at 14:19, <wei.liu2@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/mm.c
> > +++ b/xen/arch/x86/mm.c
> > @@ -4376,12 +4376,17 @@ int arch_acquire_resource(struct domain *d, 
> > unsigned int type,
> >  
> >      switch ( type )
> >      {
> > +#ifdef CONFIG_HVM
> >      case XENMEM_resource_ioreq_server:
> >      {
> >          ioservid_t ioservid = id;
> >          unsigned int i;
> >  
> >          rc = -EINVAL;
> > +        if ( !is_hvm_domain(d) )
> > +            break;
> > +
> > +        rc = -EINVAL;
> >          if ( id != (unsigned int)ioservid )
> >              break;
> 
> Since this is the only caller of hvm_get_ioreq_server_frame(),
> adding an is_hvm_domain() check here means the one inside
> the function becomes redundant - it should be dropped or
> converted to an ASSERT() imo. Furthermore if the check is to
> remain here, please drop the redundant assignment of rc.

A general principle I have been sticking to is the common code should
make sure guest type specific function should only be called when
necessary by using is_hvm/pv_*. Restructuring the code like that makes
DCE work effectively.

That means a lot of the checks in hvm_/pv_ functions should lifted out
and turned into ASSERT.

For this patch, is_hvm_domain in the case branch will remain and the one
in hvm_get_ioreq_server_frame will be turned into ASSERT.

Wei.

> 
> Jan
> 
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel