Re: [Xen-devel] [PATCH] x86/hvm: remove default ioreq server

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 07 August 2018 11:37
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-devel <xen-
> devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: Re: [PATCH] x86/hvm: remove default ioreq server
> 
> >>> On 07.08.18 at 12:03, <paul.durrant@xxxxxxxxxx> wrote:
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4098,7 +4098,6 @@ static int hvm_allow_set_param(struct domain
> *d,
> >       * since the domain may need to be paused.
> >       */
> >      case HVM_PARAM_IDENT_PT:
> > -    case HVM_PARAM_DM_DOMAIN:
> >      case HVM_PARAM_ACPI_S_STATE:
> >      /* The remaining parameters should not be set by the guest. */
> >      default:
> 
> How come you remove it here, but not from hvmop_set_param()?
> And how is this related to qemu-trad _only_ anyway? Or wait - is
> this just a leftover (and hence its removal not directly related to the
> purpose of this patch)? If so, half a sentence in the description
> would have helped recognizing this.

I remove it here because it no longer has the semantic of pausing the domain. 
In fact that disappeared a while ago and this should have been cleaned up then. 
So, yes, I should have called it out in the commit comment. Sorry about that.

> 
> > @@ -4373,13 +4372,6 @@ static int hvm_allow_get_param(struct domain
> *d,
> >      case HVM_PARAM_ALTP2M:
> >      case HVM_PARAM_X87_FIP_WIDTH:
> >          break;
> > -    /*
> > -     * The following parameters must not be read by the guest
> > -     * since the domain may need to be paused.
> > -     */
> > -    case HVM_PARAM_IOREQ_PFN:
> > -    case HVM_PARAM_BUFIOREQ_PFN:
> > -    case HVM_PARAM_BUFIOREQ_EVTCHN:
> >      /* The remaining parameters should not be read by the guest. */
> >      default:
> >          if ( d == current->domain )
> 
> Shouldn't you also remove the (or at least some) uses of these
> params from libxc? If all of them can go away, perhaps even
> their definitions should be commented out in the public header
> (or be put inside #ifdef __XEN__, as the values might want
> using in hvm_allow_{g,s}et_param() to uniformly deny access),
> considering that - as the comment you remove says - they
> were not usable from guests themselves. (I don't think they
> should be removed altogether, to keep a record of which
> values they used, as I don't think we want to re-use the values
> going forward.)

Note that the PFN params have to stay because, prior to direct resource 
mapping, upstream QEMU makes use of the PFNs in question and so the 
save/restore code in Xen has to preserve their values. The EVTCHN param can 
probably go away totally though... I don't think the save/restore code touches 
that. I guess the param definition should stay in the header but a comment be 
added that it is deprecated. I can also completely disallow get/set of that 
parameter too (with EOPNOTSUPP or EINVAL?) if you think that is a reasonable 
thing to do.

  Paul


> 
> Jan
> 


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